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

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.

The fix avoids the sorting of keys when no new keys added by ALTER
(and this is ok for MyISAM/Aria since it cannot add new keys inplace).

There is a case when implicit primary key may be changed when removing
NOT NULL from the part of unique key. In that case we update
modified_primary_key which is then used to not skip key sorting.

According to is_candidate_key() there is no other cases when primary
key may be changed implicitly.

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!

MySQL 8.0 has this bug as well as of 8.0.26.

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
 #

mysql-test/main/alter_table.test

@@ -2567,6 +2567,47 @@ alter table t1 rename column abc to ABC;
 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 #

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
 

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

sql/sql_alter.h

@@ -324,6 +324,7 @@ class Alter_table_ctx
   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];

sql/sql_table.cc

@@ -4435,9 +4435,30 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
     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,

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,

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++)
   {