diff options
| -rw-r--r-- | mysql-test/main/alter_table.result | 16 | ||||
| -rw-r--r-- | mysql-test/main/alter_table.test | 41 | ||||
| -rw-r--r-- | sql/handler.h | 1 | ||||
| -rw-r--r-- | sql/sql_alter.cc | 4 | ||||
| -rw-r--r-- | sql/sql_alter.h | 1 | ||||
| -rw-r--r-- | sql/sql_table.cc | 52 | ||||
| -rw-r--r-- | storage/maria/ha_maria.cc | 5 | ||||
| -rw-r--r-- | storage/myisam/mi_create.c | 6 |
8 files changed, 114 insertions, 12 deletions
diff --git a/mysql-test/main/alter_table.result b/mysql-test/main/alter_table.result index a43d9845947..4c64578b974 100644 --- a/mysql-test/main/alter_table.result +++ b/mysql-test/main/alter_table.result @@ -3373,5 +3373,21 @@ t1 CREATE TABLE `t1` ( ) ENGINE=MyISAM DEFAULT CHARSET=latin1 drop table t1; # +# MDEV-25803 Inplace ALTER breaks MyISAM/Aria tables when order of keys is changed +# +set @save_default_engine= @@default_storage_engine; +create or replace table t1 (x int, y int, unique (y), unique (x), primary key(x)) engine myisam; +alter table t1 change x xx int, algorithm=inplace; +check table t1; +Table Op Msg_type Msg_text +test.t1 check status OK +create or replace table t1 (x int, y int, unique (y), unique (x), primary key(x)); +alter table t1 change x xx int, algorithm=inplace; +check table t1; +Table Op Msg_type Msg_text +test.t1 check status OK +drop table t1; +set @@default_storage_engine= @save_default_engine; +# # End of 10.5 tests # diff --git a/mysql-test/main/alter_table.test b/mysql-test/main/alter_table.test index e65a4edf13e..85725820677 100644 --- a/mysql-test/main/alter_table.test +++ b/mysql-test/main/alter_table.test @@ -2568,5 +2568,46 @@ show create table t1; drop table t1; --echo # +--echo # MDEV-25803 Inplace ALTER breaks MyISAM/Aria tables when order of keys is changed +--echo # +set @save_default_engine= @@default_storage_engine; +--disable_query_log +if ($MTR_COMBINATION_INNODB) +{ + set default_storage_engine= innodb; +} +if ($MTR_COMBINATION_ARIA) +{ + set default_storage_engine= aria; +} +--enable_query_log + +if (!$MTR_COMBINATION_INNODB) +{ + --disable_query_log + --disable_result_log + # There is no inplace ADD INDEX for MyISAM/Aria: + create or replace table t1 (x int); + --error ER_ALTER_OPERATION_NOT_SUPPORTED + alter table t1 add unique (x), algorithm=inplace; + --error ER_ALTER_OPERATION_NOT_SUPPORTED + alter table t1 add primary key(x), algorithm=inplace; + --error ER_ALTER_OPERATION_NOT_SUPPORTED + alter table t1 add index(x), algorithm=inplace; + --enable_query_log + --enable_result_log +} + +create or replace table t1 (x int, y int, unique (y), unique (x), primary key(x)) engine myisam; +alter table t1 change x xx int, algorithm=inplace; +check table t1; +create or replace table t1 (x int, y int, unique (y), unique (x), primary key(x)); +alter table t1 change x xx int, algorithm=inplace; +check table t1; +# cleanup +drop table t1; +set @@default_storage_engine= @save_default_engine; + +--echo # --echo # End of 10.5 tests --echo # diff --git a/sql/handler.h b/sql/handler.h index 38c78e1961f..e58f56122d7 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -479,6 +479,7 @@ enum chf_create_flags { #define HA_CREATE_TMP_ALTER 8U #define HA_LEX_CREATE_SEQUENCE 16U #define HA_VERSIONED_TABLE 32U +#define HA_SKIP_KEY_SORT 64U #define HA_MAX_REC_LENGTH 65535 diff --git a/sql/sql_alter.cc b/sql/sql_alter.cc index 7901c9b5e32..8562643df97 100644 --- a/sql/sql_alter.cc +++ b/sql/sql_alter.cc @@ -259,7 +259,7 @@ Alter_table_ctx::Alter_table_ctx() db(null_clex_str), table_name(null_clex_str), alias(null_clex_str), new_db(null_clex_str), new_name(null_clex_str), new_alias(null_clex_str), fk_error_if_delete_row(false), fk_error_id(NULL), - fk_error_table(NULL) + fk_error_table(NULL), modified_primary_key(false) #ifdef DBUG_ASSERT_EXISTS , tmp_table(false) #endif @@ -279,7 +279,7 @@ Alter_table_ctx::Alter_table_ctx(THD *thd, TABLE_LIST *table_list, tables_opened(tables_opened_arg), new_db(*new_db_arg), new_name(*new_name_arg), fk_error_if_delete_row(false), fk_error_id(NULL), - fk_error_table(NULL) + fk_error_table(NULL), modified_primary_key(false) #ifdef DBUG_ASSERT_EXISTS , tmp_table(false) #endif diff --git a/sql/sql_alter.h b/sql/sql_alter.h index 89eb4ebb3e9..a499e978eef 100644 --- a/sql/sql_alter.h +++ b/sql/sql_alter.h @@ -324,6 +324,7 @@ public: const char *fk_error_id; /** Name of table for the above error. */ const char *fk_error_table; + bool modified_primary_key; private: char new_filename[FN_REFLEN + 1]; diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 92fd6dd3ea6..d68ea183dae 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4435,9 +4435,30 @@ without_overlaps_err: my_message(ER_WRONG_AUTO_KEY, ER_THD(thd, ER_WRONG_AUTO_KEY), MYF(0)); DBUG_RETURN(TRUE); } - /* Sort keys in optimized order */ - my_qsort((uchar*) *key_info_buffer, *key_count, sizeof(KEY), - (qsort_cmp) sort_keys); + /* + We cannot do qsort of key info if MyISAM/Aria does inplace. These engines + do not synchronise key info on inplace alter and that qsort is + indeterministic (MDEV-25803). + + Yet we do not know whether we do inplace or not. That detection is done + after this create_table_impl() and that cannot be changed because of chicken + and egg problem (inplace processing requires key info made by + create_table_impl()). + + MyISAM/Aria cannot add index inplace so we are safe to qsort key info in + that case. And if we don't add index then we do not need qsort at all. + */ + if (!(create_info->options & HA_SKIP_KEY_SORT)) + { + /* + Sort keys in optimized order. + + Note: PK must be always first key, otherwise init_from_binary_frm_image() + can not understand it. + */ + my_qsort((uchar*) *key_info_buffer, *key_count, sizeof(KEY), + (qsort_cmp) sort_keys); + } create_info->null_bits= null_fields; /* Check fields. */ @@ -8380,7 +8401,6 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, uint used_fields, dropped_sys_vers_fields= 0; KEY *key_info=table->key_info; bool rc= TRUE; - bool modified_primary_key= FALSE; bool vers_system_invisible= false; Create_field *def; Field **f_ptr,*field; @@ -8804,6 +8824,12 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, if (key_info->flags & HA_INVISIBLE_KEY) continue; const char *key_name= key_info->name.str; + const bool primary_key= table->s->primary_key == i; + const bool explicit_pk= primary_key && + !my_strcasecmp(system_charset_info, key_name, + primary_key_name); + const bool implicit_pk= primary_key && !explicit_pk; + Alter_drop *drop; drop_it.rewind(); while ((drop=drop_it++)) @@ -8817,7 +8843,7 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, if (table->s->tmp_table == NO_TMP_TABLE) { (void) delete_statistics_for_index(thd, table, key_info, FALSE); - if (i == table->s->primary_key) + if (primary_key) { KEY *tab_key_info= table->key_info; for (uint j=0; j < table->s->keys; j++, tab_key_info++) @@ -8904,13 +8930,19 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, } if (!cfield) { - if (table->s->primary_key == i) - modified_primary_key= TRUE; + if (primary_key) + alter_ctx->modified_primary_key= true; delete_index_stat= TRUE; if (!(kfield->flags & VERS_SYSTEM_FIELD)) dropped_key_part= key_part_name; continue; // Field is removed } + + DBUG_ASSERT(!primary_key || kfield->flags & NOT_NULL_FLAG); + if (implicit_pk && !alter_ctx->modified_primary_key && + !(cfield->flags & NOT_NULL_FLAG)) + alter_ctx->modified_primary_key= true; + key_part_length= key_part->length; if (cfield->field) // Not new field { @@ -8959,7 +8991,7 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, { if (delete_index_stat) (void) delete_statistics_for_index(thd, table, key_info, FALSE); - else if (modified_primary_key && + else if (alter_ctx->modified_primary_key && key_info->user_defined_key_parts != key_info->ext_key_parts) (void) delete_statistics_for_index(thd, table, key_info, TRUE); } @@ -9003,7 +9035,7 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, key_type= Key::SPATIAL; else if (key_info->flags & HA_NOSAME) { - if (! my_strcasecmp(system_charset_info, key_name, primary_key_name)) + if (explicit_pk) key_type= Key::PRIMARY; else key_type= Key::UNIQUE; @@ -10589,6 +10621,8 @@ do_continue:; tmp_disable_binlog(thd); create_info->options|=HA_CREATE_TMP_ALTER; + if (!(alter_info->flags & ALTER_ADD_INDEX) && !alter_ctx.modified_primary_key) + create_info->options|= HA_SKIP_KEY_SORT; create_info->alias= alter_ctx.table_name; error= create_table_impl(thd, alter_ctx.db, alter_ctx.table_name, alter_ctx.new_db, alter_ctx.tmp_name, diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc index 80d3d669703..fd28164cb59 100644 --- a/storage/maria/ha_maria.cc +++ b/storage/maria/ha_maria.cc @@ -735,6 +735,11 @@ static int table2maria(TABLE *table_arg, data_file_type row_type, - compare SPATIAL keys; - compare FIELD_SKIP_ZERO which is converted to FIELD_NORMAL correctly (should be correctly detected in table2maria). + + FIXME: + maria_check_definition() is never used! CHECK TABLE does not detect the + corruption! Do maria_check_definition() like check_definition() is done + by MyISAM (related to MDEV-25803). */ int maria_check_definition(MARIA_KEYDEF *t1_keyinfo, diff --git a/storage/myisam/mi_create.c b/storage/myisam/mi_create.c index 51354e0e8b5..30537cef3e7 100644 --- a/storage/myisam/mi_create.c +++ b/storage/myisam/mi_create.c @@ -713,7 +713,11 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs, } #endif - /* Write key and keyseg definitions */ + /* Write key and keyseg definitions + + TODO: update key and keyseg definitions for inplace alter (grep sql layer by + MDEV-25803). Do the same for Aria. + */ DBUG_PRINT("info", ("write key and keyseg definitions")); for (i=0 ; i < share.base.keys - uniques; i++) { |