FIXMEs, fixes, tests

- alter fix
- Test definite order fix

mysql-test/include/foreign_fail.inc

@@ -54,7 +54,7 @@ if ($crash_statement)
   --echo # State after crash (before recovery)
   --list_files_write_file $DATADIR.files.txt $DATADIR/test
   --replace_result #p# #P# #sp# #SP# #tmp# #TMP#
-  --replace_regex /sql-exchange.*\./sql-exchange./ /sql-shadow-[0-9a-f]*-/sql-shadow-/ /sql-alter-[0-9a-f]*-/sql-alter-/ /#sql-ib[1-9][0-9]*\.ibd\n//
+  --replace_regex /sql-exchange.*\./sql-exchange./ /sql(f?)-shadow-[0-9a-f]*-/sql\1-shadow-/ /sql-alter-[0-9a-f]*-/sql-alter-/ /#sql-ib[1-9][0-9]*\.ibd\n//
   --cat_file $DATADIR.files.txt
   --remove_file $DATADIR.files.txt
 
@@ -68,6 +68,9 @@ if ($crash_statement)
   --replace_regex /sql-alter-[0-9a-f]*-/sql-alter-/ /#sql-ib[1-9][0-9]*\.ibd\n//
   --cat_file $DATADIR.files.txt
   --remove_file $DATADIR.files.txt
+  # TODO: when ALTER crashes it leaves tmp garbage. Should be fixed via DDL log
+  # (though engine files can left too).
+  --remove_files_wildcard $DATADIR/test #sql-alter*.*
 }
 
 if (!$crash_statement)

mysql-test/suite/innodb/t/f.test

 --source include/have_innodb.inc
 --source include/have_debug.inc
+# Don't test this under valgrind, memory leaks will occur
+--source include/not_valgrind.inc
+# Crash tests don't work with embedded
+--source include/not_embedded.inc
 
 # In fail_fk_backup_frm the log is already written but rename is not done
 call mtr.add_suppression("Failed to execute action for entry");
@@ -60,8 +64,6 @@ let $debug_dbug="+d,crash_fk_install_shadow_frm";
 --source include/foreign_fail.inc
 call drop_tables;
 
-# FIXME: crash statements
-
 --echo # CREATE TABLE
 create table t1 (x int primary key);
 create table t2 (y int primary key);
@@ -100,11 +102,13 @@ let $debug_dbug="+d,fail_fk_backup_frm";
 let $debug_dbug="+d,fail_fk_install_shadow_frm";
 --source include/foreign_fail.inc
 let $crash_statement= $fail_statement;
-# TODO: rollback RENAME TABLE (and other DDL?) via DDL log
+# TODO: RENAME TABLE is not crash-safe itself, so rollback it via DDL log
 # NB: if we rename back in different order we'll lose some referenced hints
 # (Foreign_key_io::parse() skips them if it can't find table by name)
 # MDEV-23433 solves the problem of missing referenced hints
 let $fixup_statement= rename table xt4 to t4, xt3 to t3, xt2 to t2, xt1 to t1;
+let $debug_dbug="+d,crash_fk_write_shadow_frm";
+--source include/foreign_fail.inc
 let $debug_dbug="+d,crash_fk_backup_frm";
 --source include/foreign_fail.inc
 let $debug_dbug="+d,crash_fk_install_shadow_frm";
@@ -118,11 +122,13 @@ let $debug_dbug="+d,fail_fk_backup_frm";
 --source include/foreign_fail.inc
 let $debug_dbug="+d,fail_fk_install_shadow_frm";
 --source include/foreign_fail.inc
-#let $crash_statement= $fail_statement;
-#let $debug_dbug="+d,crash_fk_backup_frm";
-#--source include/foreign_fail.inc
-#let $debug_dbug="+d,crash_fk_install_shadow_frm";
-#--source include/foreign_fail.inc
+let $crash_statement= $fail_statement;
+let $debug_dbug="+d,crash_fk_write_shadow_frm";
+--source include/foreign_fail.inc
+let $debug_dbug="+d,crash_fk_backup_frm";
+--source include/foreign_fail.inc
+let $debug_dbug="+d,crash_fk_install_shadow_frm";
+--source include/foreign_fail.inc
 call drop_tables;
 
 --echo # RENAME COLUMN
@@ -136,16 +142,34 @@ let $debug_dbug="+d,fail_fk_backup_frm";
 --source include/foreign_fail.inc
 let $debug_dbug="+d,fail_fk_install_shadow_frm";
 --source include/foreign_fail.inc
+let $crash_statement= $fail_statement;
+let $debug_dbug="+d,crash_fk_write_shadow_frm";
+--source include/foreign_fail.inc
+let $debug_dbug="+d,crash_fk_backup_frm";
+--source include/foreign_fail.inc
+let $debug_dbug="+d,crash_fk_install_shadow_frm";
+--source include/foreign_fail.inc
 let $fail_statement= alter table t3 rename column z to z2, rename column y to y2, rename column x to x2, rename column c to c2;
+--let $crash_statement=
+let $debug_dbug="+d,fail_fk_write_shadow_frm";
+--source include/foreign_fail.inc
 let $debug_dbug="+d,fail_fk_backup_frm";
 --source include/foreign_fail.inc
 let $debug_dbug="+d,fail_fk_install_shadow_frm";
 --source include/foreign_fail.inc
+let $crash_statement= $fail_statement;
+let $debug_dbug="+d,crash_fk_write_shadow_frm";
+--source include/foreign_fail.inc
+let $debug_dbug="+d,crash_fk_backup_frm";
+--source include/foreign_fail.inc
+let $debug_dbug="+d,crash_fk_install_shadow_frm";
+--source include/foreign_fail.inc
 call drop_tables;
 
 --echo # ADD/DROP FOREIGN KEY, DROP COLUMN
 call make_tables;
 --let $create_statement=
+--let $crash_statement=
 show create table t3;
 let $fail_statement= alter table t3
   drop foreign key fk_t3,
@@ -160,18 +184,16 @@ let $debug_dbug="+d,fail_fk_backup_frm";
 --source include/foreign_fail.inc
 let $debug_dbug="+d,fail_fk_install_shadow_frm";
 --source include/foreign_fail.inc
+let $crash_statement= $fail_statement;
+let $debug_dbug="+d,crash_fk_write_shadow_frm";
+--source include/foreign_fail.inc
+# TODO: ALTER TABLE is not crash-safe itself, so rollback it via DDL log
+#let $debug_dbug="+d,crash_fk_backup_frm";
+#--source include/foreign_fail.inc
+#let $debug_dbug="+d,crash_fk_install_shadow_frm";
+#--source include/foreign_fail.inc
 call drop_tables;
 
-
-#set @save_dbug=@@debug_dbug;
-#let $create_statement= create or replace table t1 (id int primary key);
-#let $create_statement2= create or replace table t2 (id int);
-#let $insert_statement= insert into t1 values(1);
-#let $insert_statement2= insert into t2 values(1);
-#set session debug_dbug="+d,crash_fk_alter_1";
-#let $crash_statement= alter table t2 add foreign key (id) references t1(id), algorithm=copy;
-#--source suite/parts/inc/partition_crash_t2.inc
-
 drop procedure make_tables;
 drop procedure drop_tables;
 set session debug_dbug=@save_dbug;

mysql-test/suite/parts/r/partition_debug_innodb.result

@@ -388,13 +388,7 @@ a	b
 ALTER TABLE t1 EXCHANGE PARTITION p0 WITH TABLE t2;
 ERROR HY000: Lost connection to MySQL server during query
 # State after crash (before recovery)
-#sqlx-nnnn_nnnn.ibd
-db.opt
-t1#P#p0.ibd
-t1#P#p1.ibd
-t1.frm
-t1.par
-t2.frm
+#sql-exchange.frm
 # State after crash recovery
 db.opt
 t1#P#p0.ibd
@@ -496,13 +490,7 @@ a	b
 ALTER TABLE t1 EXCHANGE PARTITION p0 WITH TABLE t2;
 ERROR HY000: Lost connection to MySQL server during query
 # State after crash (before recovery)
-#sqlx-nnnn_nnnn.ibd
-db.opt
-t1#P#p0.ibd
-t1#P#p1.ibd
-t1.frm
-t1.par
-t2.frm
+#sql-exchange.frm
 # State after crash recovery
 db.opt
 t1#P#p0.ibd
@@ -604,13 +592,7 @@ a	b
 ALTER TABLE t1 EXCHANGE PARTITION p0 WITH TABLE t2;
 ERROR HY000: Lost connection to MySQL server during query
 # State after crash (before recovery)
-#sqlx-nnnn_nnnn.ibd
-db.opt
-t1#P#p1.ibd
-t1.frm
-t1.par
-t2.frm
-t2.ibd
+#sql-exchange.ibd
 # State after crash recovery
 db.opt
 t1#P#p0.ibd
@@ -712,13 +694,7 @@ a	b
 ALTER TABLE t1 EXCHANGE PARTITION p0 WITH TABLE t2;
 ERROR HY000: Lost connection to MySQL server during query
 # State after crash (before recovery)
-#sqlx-nnnn_nnnn.ibd
-db.opt
-t1#P#p1.ibd
-t1.frm
-t1.par
-t2.frm
-t2.ibd
+#sql-exchange.ibd
 # State after crash recovery
 db.opt
 t1#P#p0.ibd

sql/datadict.cc

@@ -767,7 +767,7 @@ void FK_backup_storage::drop_backup_frms(THD *thd)
       continue;
     if (deactivate_ddl_log_entry(bak.second.restore_backup_entry->entry_pos))
     {
-      // FIXME: test getting into here (and other deactivate_ddl_log_entry() failures)
+      // TODO: test getting into here (and other deactivate_ddl_log_entry() failures)
       my_printf_error(ER_DDL_LOG_ERROR, "Deactivating restore backup entry %u failed",
                       MYF(0), bak.second.restore_backup_entry->entry_pos);
       // TODO: must be atomic

sql/sql_class.h

@@ -1023,7 +1023,7 @@ class FK_share_backup : public FK_backup
 };
 
 
-// NB: FK_ddl_backup responds for share release unlike FK_table_backup
+// NB: FK_ddl_backup responds for share release unlike FK_share_backup
 class FK_ddl_backup : public FK_share_backup
 {
   /* NB: if sa.share is not empty, share is auto-released on destructor */
@@ -1032,7 +1032,7 @@ class FK_ddl_backup : public FK_share_backup
      NB: if sa.share is not empty, share == sa.share. ALTER algorithms are more
      complex and shares are held and released in separate container alter_ctx.fk_shares.
      To make DDL logging common for all commands we handle it via FK_backup_storage interface, but
-     without templating and virtual interfaces (these are overcomplexity for only 2 variations)
+     without templating and virtual interfaces (these are overcomplexity for only 2 variants)
      we have to converge backup operations into single FK_ddl_backup.
   */
 
@@ -1057,6 +1057,17 @@ class FK_ddl_backup : public FK_share_backup
   bool backup_frm(ddl_log_info &log_info, Table_name table);
 };
 
+#ifndef DBUG_OFF
+// NB: we do want definite order of shares in test cases
+struct TABLE_SHARE_by_name
+{
+  bool operator() (const TABLE_SHARE *s1, const TABLE_SHARE *s2) const
+  {
+    return s1->cmp_db_table(s2->db, s2->table_name) < 0;
+  }
+};
+#endif
+
 
 /*
    NB: again, ALTER does require duplicate check hence mbd::map is used, while other commands
@@ -1064,7 +1075,12 @@ class FK_ddl_backup : public FK_share_backup
    ifaces we just use mbd::map for everything. We are not going to hit bottleneck here:
    it is DDL (rare operation), it is less than hundred of foreign keys normally.
 */
-class FK_backup_storage: public mbd::map<TABLE_SHARE *, FK_ddl_backup>, public ddl_log_info
+class FK_backup_storage: public mbd::map<TABLE_SHARE *, FK_ddl_backup
+#ifndef DBUG_OFF
+                                         , TABLE_SHARE_by_name
+#endif
+                         >,
+                         public ddl_log_info
 {
 public:
   int write_shadow_frms();

sql/sql_rename.cc

@@ -348,8 +348,7 @@ do_rename(THD *thd, TABLE_LIST *ren_table, const LEX_CSTRING *new_db,
       Shared table. Just drop the old .frm as it's not correct anymore
       Discovery will find the old table when it's accessed
      */
-    // FIXME: tdc_remove_table() is now done later. Does that work?
-    // tdc_remove_table(thd, ren_table->db.str, ren_table->table_name.str);
+    tdc_remove_table(thd, ren_table->db.str, ren_table->table_name.str);
     quick_rm_table(thd, 0, &ren_table->db, &old_alias, FRM_ONLY, 0);
     DBUG_RETURN(0);
   }

sql/sql_table.cc

@@ -8191,7 +8191,8 @@ static bool mysql_inplace_alter_table(THD *thd,
   if (wait_while_table_is_used(thd, table, HA_EXTRA_PREPARE_FOR_RENAME))
     goto rollback;
 
-  if (alter_ctx->fk_handle_alter(thd))
+  if (alter_ctx->fk_handle_alter(thd) ||
+      alter_ctx->fk_ref_backup.install_shadow_frms())
     goto rollback;
 
   /* Set MDL_BACKUP_DDL */
@@ -8262,13 +8263,15 @@ static bool mysql_inplace_alter_table(THD *thd,
     {
       my_error(HA_ERR_INCOMPATIBLE_DEFINITION, MYF(0));
       alter_ctx->fk_ref_backup.rollback(thd);
-      // FIXME: why not goto rollback?
       goto cleanup;
     }
   }
 
   table->s->frm_image= NULL;
 
+  alter_ctx->fk_ref_backup.drop_backup_frms(thd);
+  alter_ctx->fk_ref_backup.clear();
+
   close_all_tables_for_name(thd, table->s,
                             alter_ctx->is_table_renamed() ?
                             HA_EXTRA_PREPARE_FOR_RENAME :
@@ -8276,15 +8279,6 @@ static bool mysql_inplace_alter_table(THD *thd,
                             NULL);
   table_list->table= table= NULL;
 
-  // FIXME: do right after fk_handle_alter?
-  if (alter_ctx->fk_ref_backup.install_shadow_frms())
-  {
-    alter_ctx->fk_ref_backup.rollback(thd);
-    DBUG_RETURN(true);
-  }
-
-  alter_ctx->fk_ref_backup.drop_backup_frms(thd);
-
   /*
     Replace the old .FRM with the new .FRM, but keep the old name for now.
     Rename to the new name (if needed) will be handled separately below.
@@ -8310,7 +8304,6 @@ static bool mysql_inplace_alter_table(THD *thd,
     if (mysql_rename_table(db_type, &alter_ctx->db, &alter_ctx->table_name,
                            &alter_ctx->new_db, &alter_ctx->new_alias, 0))
     {
-      // FIXME: rollback only table name
       /*
         If the rename fails we will still have a working table
         with the old name, but with other changes applied.
@@ -8331,7 +8324,6 @@ static bool mysql_inplace_alter_table(THD *thd,
       (void) mysql_rename_table(db_type,
                                 &alter_ctx->new_db, &alter_ctx->new_alias,
                                 &alter_ctx->db, &alter_ctx->alias, NO_FK_CHECKS);
-      // FIXME: if rename succeeds rollback only table name
       DBUG_RETURN(true);
     }
     rename_table_in_stat_tables(thd, &alter_ctx->db, &alter_ctx->alias,
@@ -11252,6 +11244,7 @@ do_continue:;
   if (wait_while_table_is_used(thd, table, HA_EXTRA_PREPARE_FOR_RENAME))
     goto err_new_table_cleanup;
 
+  alter_ctx.fk_ref_backup.erase(table->s);
   if (alter_ctx.fk_handle_alter(thd))
     // NB: now after lock upgrade it jumps to "err_with_mdl" as well
     goto err_new_table_cleanup;
@@ -13191,7 +13184,7 @@ bool fk_handle_rename(THD *thd, TABLE_LIST *old_table, const LEX_CSTRING *new_db
   if (share->foreign_keys.is_empty() && share->referenced_keys.is_empty())
     return false;
   mbd::set<Table_name> tables;
-  mbd::set<Table_name> already; // FIXME: do we need it for mbd::map?
+  mbd::set<Table_name> already;
   MDL_request_list mdl_list;
   for (auto &bak: fk_rename_backup)
   {