MDEV-29440 InnoDB instant ALTER TABLE recovery must use READ UNCOMMITTED

In commit 8f8ba758 (MDEV-27234)
the data dictionary recovery was changed to use READ COMMITTED
so that table-rebuild operations (OPTIMIZE TABLE, TRUNCATE TABLE,
some forms of ALTER TABLE) would be recovered correctly.

However, for operations that avoid a table rebuild thanks to
being able to instantly ADD, DROP or reorder columns, recovery
must use the READ UNCOMMITTED isolation level so that changes to
the hidden metadata record can be rolled back.

We will detect instant operations by detecting uncommitted changes
to SYS_COLUMNS in case there is no uncommitted change of SYS_TABLES.ID
for the table. In any table-rebuilding DDL operation, the SYS_TABLES.ID
(and likely also the table name) will be updated.

As part of rolling back the instant ALTER TABLE operation, after the
operation on the hidden metadata record has been rolled back, a rollback
of an INSERT into SYS_COLUMNS in row_undo_ins_remove_clust_rec() will
invoke trx_t::evict_table() to discard the READ UNCOMMITTED definition
of the table. After that, subsequent recovery steps will load and use
the correct table definition.

Reviewed by: Thirunarayanan Balathandayuthapani
Tested by: Matthias Leich

mysql-test/suite/innodb/r/instant_alter_crash.result

@@ -176,5 +176,25 @@ t3	CREATE TABLE `t3` (
   PRIMARY KEY (`id`),
   UNIQUE KEY `v2` (`v2`)
 ) ENGINE=InnoDB DEFAULT CHARSET=latin1
-DROP TABLE t1,t2,t3;
+DROP TABLE t2,t3;
+#
+# MDEV-29440 InnoDB instant ALTER TABLE recovery wrongly uses
+#            READ COMMITTED isolation level instead of READ UNCOMMITTED
+#
+CREATE TABLE t2(a INT UNSIGNED PRIMARY KEY) ENGINE=InnoDB;
+INSERT INTO t2 VALUES (1),(2),(3),(4),(5),(6);
+connect ddl, localhost, root;
+SET DEBUG_SYNC='innodb_alter_inplace_before_commit SIGNAL ddl WAIT_FOR ever';
+ALTER TABLE t2 ADD COLUMN b TINYINT UNSIGNED NOT NULL DEFAULT 42 FIRST;
+connection default;
+SET DEBUG_SYNC='now WAIT_FOR ddl';
+SET GLOBAL innodb_flush_log_at_trx_commit=1;
+DELETE FROM t1;
+# Kill the server
+disconnect ddl;
+# restart
+CHECK TABLE t2;
+Table	Op	Msg_type	Msg_text
+test.t2	check	status	OK
+DROP TABLE t1,t2;
 db.opt

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

@@ -198,6 +198,30 @@ disconnect ddl;
 SHOW CREATE TABLE t1;
 SHOW CREATE TABLE t2;
 SHOW CREATE TABLE t3;
-DROP TABLE t1,t2,t3;
+DROP TABLE t2,t3;
+
+--echo #
+--echo # MDEV-29440 InnoDB instant ALTER TABLE recovery wrongly uses
+--echo #            READ COMMITTED isolation level instead of READ UNCOMMITTED
+--echo #
+CREATE TABLE t2(a INT UNSIGNED PRIMARY KEY) ENGINE=InnoDB;
+INSERT INTO t2 VALUES (1),(2),(3),(4),(5),(6);
+
+connect ddl, localhost, root;
+SET DEBUG_SYNC='innodb_alter_inplace_before_commit SIGNAL ddl WAIT_FOR ever';
+--send
+ALTER TABLE t2 ADD COLUMN b TINYINT UNSIGNED NOT NULL DEFAULT 42 FIRST;
+
+connection default;
+SET DEBUG_SYNC='now WAIT_FOR ddl';
+SET GLOBAL innodb_flush_log_at_trx_commit=1;
+DELETE FROM t1;
+
+--source include/kill_mysqld.inc
+disconnect ddl;
+--source include/start_mysqld.inc
+
+CHECK TABLE t2;
+DROP TABLE t1,t2;
 
 --list_files $MYSQLD_DATADIR/test

storage/innobase/handler/ha_innodb.cc

@@ -12839,7 +12839,7 @@ int create_table_info_t::create_table(bool create_fk)
 	if (err == DB_SUCCESS) {
 		/* Check that also referencing constraints are ok */
 		dict_names_t	fk_tables;
-		err = dict_load_foreigns(m_table_name, nullptr,
+		err = dict_load_foreigns(m_table_name, nullptr, false,
 					 m_trx->id, true,
 					 DICT_ERR_IGNORE_NONE, fk_tables);
 		while (err == DB_SUCCESS && !fk_tables.empty()) {

storage/innobase/handler/handler0alter.cc

@@ -9881,7 +9881,7 @@ innobase_update_foreign_cache(
 	dict_names_t	fk_tables;
 
 	err = dict_load_foreigns(user_table->name.m_name,
-				 ctx->col_names, 1, true,
+				 ctx->col_names, false, 1, true,
 				 DICT_ERR_IGNORE_NONE,
 				 fk_tables);
 
@@ -9892,7 +9892,7 @@ innobase_update_foreign_cache(
 		loaded with "foreign_key checks" off,
 		so let's retry the loading with charset_check is off */
 		err = dict_load_foreigns(user_table->name.m_name,
-					 ctx->col_names, 1, false,
+					 ctx->col_names, false, 1, false,
 					 DICT_ERR_IGNORE_NONE,
 					 fk_tables);
 

storage/innobase/handler/i_s.cc

@@ -4772,7 +4772,7 @@ static const char *i_s_sys_tables_rec(const btr_pcur_t &pcur, mtr_t *mtr,
   }
 
   if (rec)
-    return dict_load_table_low(mtr, rec, table);
+    return dict_load_table_low(mtr, false, rec, table);
 
   *table= dict_sys.load_table
     (span<const char>{reinterpret_cast<const char*>(pcur.old_rec), len});

storage/innobase/include/dict0load.h

@@ -89,6 +89,8 @@ dict_load_foreigns(
 	const char*		table_name,	/*!< in: table name */
 	const char**		col_names,	/*!< in: column names, or NULL
 						to use table->col_names */
+	bool			uncommitted,	/*!< in: use READ UNCOMMITTED
+						transaction isolation level */
 	trx_id_t		trx_id,		/*!< in: DDL transaction id,
 						or 0 to check
 						recursive load of tables
@@ -125,11 +127,12 @@ dict_getnext_system(
 /** Load a table definition from a SYS_TABLES record to dict_table_t.
 Do not load any columns or indexes.
 @param[in,out]	mtr		mini-transaction
+@param[in]	uncommitted	whether to use READ UNCOMMITTED isolation level
 @param[in]	rec		SYS_TABLES record
 @param[out,own]	table		table, or nullptr
 @return	error message
 @retval	nullptr on success */
-const char *dict_load_table_low(mtr_t *mtr,
+const char *dict_load_table_low(mtr_t *mtr, bool uncommitted,
                                 const rec_t *rec, dict_table_t **table)
   MY_ATTRIBUTE((nonnull, warn_unused_result));
 

storage/innobase/row/row0mysql.cc

@@ -2869,7 +2869,7 @@ row_rename_table_for_mysql(
 		dict_names_t	fk_tables;
 
 		err = dict_load_foreigns(
-			new_name, nullptr, trx->id,
+			new_name, nullptr, false, trx->id,
 			!old_is_tmp || trx->check_foreigns,
 			use_fk
 			? DICT_ERR_IGNORE_NONE