search

midenok / mariadb

MariaDB server is a community developed fork of MySQL server. Started by core members of the original MySQL team, MariaDB actively works with outside developers to deliver the most featureful, stable, and sanely licensed open SQL server in the industry.
GNU General Public License v2.0
0 stars 0 forks source link

MDEV-25803 Inplace ALTER breaks MyISAM/Aria table when order of keys is changed #98

Open midenok opened 3 years ago

midenok commented 3 years ago

Reproduce

create table t1 (x int, y int, unique (y), unique (x), primary key (x)) engine myisam;
alter table t1 rename index x to xx, algorithm=inplace;
check table t1;
# cleanup
drop table t1;

Result

mtr --mysqld=--debug=d,error main.x

check table t1;
Table   Op      Msg_type        Msg_text
test.t1 check   Error   Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM
test.t1 check   error   Corrupt
check_definition: error: Key segment 0 (key 1) has different definition
check_definition: error: t1_type=4, t1_language=63, t1_null_bit=0, t1_length=4
check_definition: error: t2_type=4, t2_language=63, t2_null_bit=2, t2_length=4
handler::ha_open: error: error: 190  errno: 0

Notes

Does not reproduce with algorithm=copy or with other engines.

MySQL 8.0 fails as well, but it has earlier error check:

mysqltest: At line 4: Query 'alter table t1 rename index x to xx, algorithm=inplace' failed.
ERROR 1034 (HY000): Incorrect key file for table 't1'; try to repair it

Though it seems it does not recover from that correctly as this is not covered by atomic DDL:

static bool mysql_inplace_alter_table(
...
  if (!(db_type->flags & HTON_SUPPORTS_ATOMIC_DDL)) {
    Open_table_context ot_ctx(thd, MYSQL_OPEN_REOPEN);

    table_list->mdl_request.ticket = mdl_ticket;
    if (open_table(thd, table_list, &ot_ctx)) goto cleanup2;
midenok commented 3 years ago

Error thrown

#0  check_definition (t1_keyinfo=0x7f8c8401ea48, t1_recinfo=0x7f8c8401e928, t1_keys=3, t1_recs=3, t2_keyinfo=0x7f8c84029e40, t2_recinfo=0x7f8c8402a050, t2_keys=3, t2_recs=3, strict=true, table_arg=0x7f8c84027878) at ../src/storage/myisam/ha_myisam.cc:570
#1  0x0000000001621670 in ha_myisam::open (this=0x7f8c84028040, name=0x7f8c8402f928 "./test/t1", mode=2, test_if_locked=50) at ../src/storage/myisam/ha_myisam.cc:829
#2  0x0000000000cb75b4 in handler::ha_open (this=0x7f8c84028040, table_arg=0x7f8c84027878, name=0x7f8c8402f928 "./test/t1", mode=2, test_if_locked=50, mem_root=0x0, partitions_to_open=0x0) at ../src/sql/handler.cc:2997
#3  0x0000000000a539e9 in open_table_from_share (thd=0x7f8c84000d48, share=0x7f8c8402f390, alias=0x7f8c84015e88, db_stat=33, prgflag=8, ha_open_flags=50, outparam=0x7f8c84027878, is_create_table=false, partitions_to_open=0x0) at ../src/sql/table.cc:4234
#4  0x000000000081d4fc in open_table (thd=0x7f8c84000d48, table_list=0x7f8c84015e40, ot_ctx=0x7f8c9aa10e68) at ../src/sql/sql_base.cc:2001
#5  0x0000000000822547 in open_and_process_table (thd=0x7f8c84000d48, tables=0x7f8c84015e40, counter=0x7f8c9aa10f6c, flags=0, prelocking_strategy=0x7f8c9aa10fe0, has_prelocking_list=false, ot_ctx=0x7f8c9aa10e68) at ../src/sql/sql_base.cc:3786
#6  0x0000000000820ff0 in open_tables (thd=0x7f8c84000d48, options=..., start=0x7f8c9aa10f80, counter=0x7f8c9aa10f6c, flags=0, prelocking_strategy=0x7f8c9aa10fe0) at ../src/sql/sql_base.cc:4269
#7  0x0000000000825755 in open_and_lock_tables (thd=0x7f8c84000d48, options=..., tables=0x7f8c84015e40, derived=true, flags=0, prelocking_strategy=0x7f8c9aa10fe0) at ../src/sql/sql_base.cc:5215
#8  0x00000000007cc73c in open_and_lock_tables (thd=0x7f8c84000d48, tables=0x7f8c84015e40, derived=true, flags=0) at ../src/sql/sql_base.h:507
#9  0x0000000000ad891b in open_only_one_table (thd=0x7f8c84000d48, table=0x7f8c84015e40, repair_table_use_frm=false, is_view_operator_func=true) at ../src/sql/sql_admin.cc:408
#10 0x0000000000ad3d4d in mysql_admin_table (thd=0x7f8c84000d48, tables=0x7f8c84015e40, check_opt=0x7f8c84006188, operator_name=0x17af157 "check", lock_type=TL_READ_NO_INSERT, org_open_for_modify=false, repair_table_use_frm=false, extra_open_options=32, prepare_func=0x0, operator_func=(int (handler::*)(handler * const, THD *, HA_CHECK_OPT *)) 0xcbfb90 <handler::ha_check(THD*, st_ha_check_opt*)>, view_operator_func=0xa425a0 <view_check(THD*, TABLE_LIST*, st_ha_check_opt*)>, is_cmd_replicated=false) at ../src/sql/sql_admin.cc:614
#11 0x0000000000ad7682 in Sql_cmd_check_table::execute (this=0x7f8c84016520, thd=0x7f8c84000d48) at ../src/sql/sql_admin.cc:1477
#12 0x00000000008f049d in mysql_execute_command (thd=0x7f8c84000d48) at ../src/sql/sql_parse.cc:6056
#13 0x00000000008e108c in mysql_parse (thd=0x7f8c84000d48, rawbuf=0x7f8c84015d90 "check table t1", length=14, parser_state=0x7f8c9aa144f0, is_com_multi=false, is_next_command=false) at ../src/sql/sql_parse.cc:8
94       for (i= 0; i < t1_keys; i++)
495       {
496         HA_KEYSEG *t1_keysegs= t1_keyinfo[i].seg;
497         HA_KEYSEG *t2_keysegs= t2_keyinfo[i].seg;
...
531         for (j=  t1_keyinfo[i].keysegs; j--;)
532         {
...
552           if ((!mysql_40_compat &&
553               t1_keysegs[j].language != t2_keysegs[j].language) ||
554               t1_keysegs_j__type != t2_keysegs[j].type ||
555               t1_keysegs[j].null_bit != t2_keysegs[j].null_bit ||
556               t1_keysegs[j].length != t2_keysegs[j].length ||
557               t1_keysegs[j].start != t2_keysegs[j].start)
558           {
559             DBUG_PRINT("error", ("Key segment %d (key %d) has different "
560                                  "definition", j, i));
561             DBUG_PRINT("error", ("t1_type=%d, t1_language=%d, t1_null_bit=%d, "
562                                  "t1_length=%d",
563                                  t1_keysegs[j].type, t1_keysegs[j].language,
564                                  t1_keysegs[j].null_bit, t1_keysegs[j].length));
565             DBUG_PRINT("error", ("t2_type=%d, t2_language=%d, t2_null_bit=%d, "
566                                  "t2_length=%d",
567                                  t2_keysegs[j].type, t2_keysegs[j].language,
568                                  t2_keysegs[j].null_bit, t2_keysegs[j].length));
569
570             DBUG_RETURN(1);
571           }
(rr) p i
$37 = 1
(rr) p j
$38 = 0
(rr) p t1_keysegs[j].null_bit
$50 = 0 '\000'
(rr) p t2_keysegs[j].null_bit
$51 = 2 '\002'

Good (no ALTER)

(rr) p t1_keysegs[j].null_bit
$28 = 2 '\002'
(rr) p t2_keysegs[j].null_bit
$29 = 2 '\002'

Good (algorithm=copy)

(rr) p t1_keysegs[j].null_bit
$10 = 0 '\000'
(rr) p t2_keysegs[j].null_bit
$11 = 0 '\000'

Cause

Order of keys changed. Stale t2_keyinfo[i].seg.

Warning

Aria does not reproduce this error message because maria_check_definition() is never used! But the problem may still exist.

midenok commented 3 years ago

Info

t1_keyinfo came from TABLE instance, t2_keyinfo came from MYISAM_SHARE.

#0  check_definition (t1_keyinfo=0x7f8c8401ea48, t1_recinfo=0x7f8c8401e928, t1_keys=3, t1_recs=3, t2_keyinfo=0x7f8c84029e40, t2_recinfo=0x7f8c8402a050, t2_keys=3, t2_recs=3, strict=true, table_arg=0x7f8c84027878) at ../src/storage/myisam/ha_myisam.cc:570
#1  0x0000000001621670 in ha_myisam::open (this=0x7f8c84028040, name=0x7f8c8402f928 "./test/t1", mode=2, test_if_locked=50) at ../src/storage/myisam/ha_myisam.cc:829

It is OK that some members are uninitialized.

(rr) p t1_keyinfo[i]
$45 = {
  share = 0xa5a5a5a5a5a5a5a5,
  keysegs = 1,
  flag = 1,
  key_alg = 1 '\001',
  block_length = 1024,
  underflow_block_length = 42405,
  keylength = 42405,
  minlength = 42405,
  maxlength = 42405,
  block_size_index = 42405,
  version = 2779096485,
  ftkey_nr = 2779096485,
  seg = 0x7f8c8401ebb8,
  end = 0xa5a5a5a5a5a5a5a5,
  parser = 0xa5a5a5a5a5a5a5a5,
  bin_search = 0xa5a5a5a5a5a5a5a5,
  get_key = 0xa5a5a5a5a5a5a5a5,
  pack_key = 0xa5a5a5a5a5a5a5a5,
  store_key = 0xa5a5a5a5a5a5a5a5,
  ck_insert = 0xa5a5a5a5a5a5a5a5,
  ck_delete = 0xa5a5a5a5a5a5a5a5
}
(rr) p t2_keyinfo[i]
$46 = {
  share = 0x7f8c84029838,
  keysegs = 1,
  flag = 73,
  key_alg = 1 '\001',
  block_length = 1024,
  underflow_block_length = 341,
  keylength = 11,
  minlength = 11,
  maxlength = 11,
  block_size_index = 0,
  version = 0,
  ftkey_nr = 0,
  seg = 0x7f8c84029fd0,
  end = 0x7f8c84029ff0,
  parser = 0x1ff70c0 <ft_default_parser>,
  bin_search = 0x1666c70 <_mi_seq_search>,
  get_key = 0x16688b0 <_mi_get_pack_key>,
  pack_key = 0x166aa20 <_mi_calc_var_key_length>,
  store_key = 0x166bb50 <_mi_store_static_key>,
  ck_insert = 0x166fce0 <_mi_ck_write>,
  ck_delete = 0x1640040 <_mi_ck_delete>
}
(rr) p *t1_keyinfo[i].seg
$52 = {
  charset = 0xa5a5a5a5a5a5a5a5,
  start = 1,
  null_pos = 0,
  bit_pos = 0,
  flag = 0,
  length = 4,
  language = 63,
  type = 4 '\004',
  null_bit = 0 '\000',
  bit_start = 0 '\000',
  bit_length = 0 '\000'
}
(rr) p *t2_keyinfo[i].seg
$53 = {
  charset = 0x0,
  start = 5,
  null_pos = 0,
  bit_pos = 0,
  flag = 80,
  length = 4,
  language = 63,
  type = 4 '\004',
  null_bit = 2 '\002',
  bit_start = 0 '\000',
  bit_length = 0 '\000'
}
midenok commented 3 years ago

Cause

mysql_prepare_create_table() changes key order:

4439      /* Sort keys in optimized order */
4440      my_qsort((uchar*) *key_info_buffer, *key_count, sizeof(KEY),
4441               (qsort_cmp) sort_keys);

Fix candidate

Disable this my_qsort() for inplace alter (or when inplace is possible).

This is impossible because init_from_binary_frm_image() requires PK to be always first, so sorting is a must!

midenok commented 3 years ago

Reproduce 2: ASAN failure

Reproduces with Aria and MyISAM (MyISAM is reproduced with --repeat=30).

create table lineitem (
  l_partkey int(11) default null,
  l_suppkey int(11) default null,
  l_receiptdate date default null,
  l_shipmode char(10) default null,
  key i_l_suppkey_partkey (l_partkey,l_suppkey),
  unique a (l_shipmode),
  unique p (l_receiptdate),
  primary key (l_receiptdate,l_partkey)
) engine=aria default charset utf8mb4;

insert into lineitem values
  (156,4,'1996-03-22','truck'),
  (68,9,'1996-04-20','mail');

alter table lineitem rename index if exists x to xx;

--connect (con1,localhost,root,,test)
--send
  select * from lineitem;

--connection default
--error 0,1030
select * from lineitem where l_receiptdate > '1998-07-01';

# cleanup
--connection con1
--error 0,1030
--reap
drop table lineitem;

Fix candidate

--- a/sql/handler.cc
+++ b/sql/handler.cc
@@ -4902,8 +4902,7 @@ handler::check_if_supported_inplace_alter(TABLE *altered_table,
     ALTER_DROP_CHECK_CONSTRAINT |
     ALTER_PARTITIONED |
     ALTER_VIRTUAL_GCOL_EXPR |
-    ALTER_RENAME |
-    ALTER_RENAME_INDEX;
+    ALTER_RENAME;

   /* Is there at least one operation that requires copy algorithm? */
   if (ha_alter_info->handler_flags & ~inplace_offline_operations)

But this doesn't fix all the commands, because every of this may reproduce:

  alter_table_operations inplace_offline_operations=
    ALTER_COLUMN_TYPE_CHANGE_BY_ENGINE |
    ALTER_COLUMN_NAME |
    ALTER_RENAME_COLUMN |
    ALTER_CHANGE_COLUMN_DEFAULT |
    ALTER_COLUMN_DEFAULT |
    ALTER_COLUMN_OPTION |
    ALTER_CHANGE_CREATE_OPTION |
    ALTER_DROP_CHECK_CONSTRAINT |
    ALTER_PARTITIONED |
    ALTER_VIRTUAL_GCOL_EXPR |
    ALTER_RENAME;
midenok commented 3 years ago

Reproduce 2: short form, 10.3 reproducible

Aria reproduced always. MyISAM is reproduced with --repeat=30.

create table t1 (
  x int,
  y char(10),
  unique (y),
  unique (x),
  primary key (x)
) engine=aria;

insert into t1 values
  (1, 'truck'),
  (2, 'mail');

alter table t1 change y z char(10), algorithm=inplace;

--connect (con1,localhost,root,,test)
--send
  select * from t1;

--connection default
--error 0,1030
select * from t1 where x > 3;

# cleanup
--connection con1
--error 0,1030
--reap
drop table t1;

Cause

my_qsort() is indeterministic: it can change order of already sorted array.

Info: keysegs written to MyISAM metadata

int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs,
          uint columns, MI_COLUMNDEF *recinfo,
          uint uniques, MI_UNIQUEDEF *uniquedefs,
          MI_CREATE_INFO *ci,uint flags)
{
...
  DBUG_PRINT("info", ("write state info and base info"));
  if (mi_state_info_write(file, &share.state, 2) ||
      mi_base_info_write(file, &share.base))
    goto err;
#ifndef DBUG_OFF
  if ((uint) mysql_file_tell(file, MYF(0)) != base_pos + MI_BASE_INFO_SIZE)
  {
    uint pos=(uint) mysql_file_tell(file, MYF(0));
    DBUG_PRINT("warning",("base_length: %d  != used_length: %d",
              base_pos+ MI_BASE_INFO_SIZE, pos));
  }
#endif

  /* Write key and keyseg definitions */
  DBUG_PRINT("info", ("write key and keyseg definitions"));
  for (i=0 ; i < share.base.keys - uniques; i++)
  {
    uint sp_segs=(keydefs[i].flag & HA_SPATIAL) ? 2*SPDIMS : 0;

    if (mi_keydef_write(file, &keydefs[i]))
      goto err;
    for (j=0 ; j < keydefs[i].keysegs-sp_segs ; j++)
      if (mi_keyseg_write(file, &keydefs[i].seg[j]))
       goto err;
midenok commented 3 years ago

There is a bug in inpace alter for MyISAM/Aria tables (MDEV-25803). mysql_prepare_create_table() does my_qsort(sort_keys) on key info. This sorting is indeterministic: a table is created with one order and inplace alter may overwrite frm with another order. Since inplace alter does nothing about key info for MyISAM/Aria storage engines this results in discrepancy between frm and storage engine key definitions. What would you recommend for the fix now?

Notes: mi_keydef_write()/mi_keyseg_write() are used only in mi_create(). They should be used in ha_inplace_alter_table() as well.

Aria corruption detection is unimplemented: maria_check_definition() is never used!

midenok commented 3 years ago

Might be useful in the code, not committed now:

--- a/sql/sql_table.cc
+++ b/sql/sql_table.cc
@@ -9054,6 +9054,11 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
        my_error(ER_WRONG_NAME_FOR_INDEX, MYF(0), key->name.str);
         goto err;
       }
+      if (key->type == Key::PRIMARY)
+      {
+        DBUG_ASSERT(alter_info->flags & ALTER_ADD_INDEX);
+        alter_info->flags|= ALTER_ADD_PK_INDEX;
+      }
     }
   }