MDEV-23514 Race conditions between ROLLBACK and ALTER TABLE

Since commit 1509363970e9cb574005e3af560299c055dda983 (MDEV-23484)
the rollback of InnoDB transactions is no longer protected by
dict_operation_lock. Removing that protection revealed a race
condition between transaction rollback and the rollback of an
online table-rebuilding operation (OPTIMIZE TABLE, or any online
ALTER TABLE that is rebuilding the table).

row_undo_mod_clust(): Re-check dict_index_is_online_ddl() after
acquiring index->lock, similar to how row_undo_ins_remove_clust_rec()
is doing it. Because innobase_online_rebuild_log_free() is holding
exclusive index->lock while invoking row_log_free(), this re-check
will ensure that row_log_table_low() will not be invoked when
index->online_log=NULL.

A different race condition is possible between the rollback of a
recovered transaction and the start of online secondary index creation.
Because prepare_inplace_alter_table_dict() is not acquiring an InnoDB
table lock in this case, and because recovered transactions are not
covered by metadata locks (MDL), the dict_table_t::indexes could be
modified by prepare_inplace_alter_table_dict() while the rollback of
a recovered transaction is being executed. Normal transactions would
be covered by MDL, and during prepare_inplace_alter_table_dict() we
do hold MDL_EXCLUSIVE, that is, an online ALTER TABLE operation may
not execute concurrently with other transactions that have accessed
the table.

row_undo(): To prevent a race condition with
prepare_inplace_alter_table_dict(), acquire dict_operation_lock
for all recovered transactions. Before MDEV-23484 we used to acquire
it for all transactions, not only recovered ones.

Note: row_merge_drop_indexes() would not invoke
dict_index_remove_from_cache() while transactional locks
exist on the table, or while any thread is holding an open table handle.
OK, it does that for FULLTEXT INDEX, but ADD FULLTEXT INDEX is not
supported as an online operation, and therefore
prepare_inplace_alter_table_dict() would acquire a table S lock,
which cannot succeed as long as recovered transactions on the table
exist, because they would hold a conflicting IX lock on the table.

3 files changed, 20 insertions, 13 deletions

diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc
index f0aae491eae..09bfc5f5503 100644
--- a/storage/innobase/handler/handler0alter.cc
+++ b/storage/innobase/handler/handler0alter.cc
@@ -8267,11 +8267,11 @@ ha_innobase::commit_inplace_alter_table(
 
 		/* Exclusively lock the table, to ensure that no other
 		transaction is holding locks on the table while we
-		change the table definition. The MySQL meta-data lock
+		change the table definition. The meta-data lock (MDL)
 		should normally guarantee that no conflicting locks
 		exist. However, FOREIGN KEY constraints checks and any
 		transactions collected during crash recovery could be
-		holding InnoDB locks only, not MySQL locks. */
+		holding InnoDB locks only, not MDL. */
 
 		dberr_t error = row_merge_lock_table(
 			m_prebuilt->trx, ctx->old_table, LOCK_X);
diff --git a/storage/innobase/row/row0umod.cc b/storage/innobase/row/row0umod.cc
index b1cc994cdd8..0de0760834e 100644
--- a/storage/innobase/row/row0umod.cc
+++ b/storage/innobase/row/row0umod.cc
@@ -319,17 +319,7 @@ row_undo_mod_clust(
 		ut_ad(err == DB_SUCCESS || err == DB_OUT_OF_FILE_SPACE);
 	}
 
-	/* Online rebuild cannot be initiated while we are holding
-	dict_operation_lock and index->lock. (It can be aborted.) */
-	ut_ad(online || !dict_index_is_online_ddl(index));
-
-	if (err == DB_SUCCESS && online) {
-
-		ut_ad(rw_lock_own_flagged(
-				&index->lock,
-				RW_LOCK_FLAG_S | RW_LOCK_FLAG_X
-				| RW_LOCK_FLAG_SX));
-
+	if (err == DB_SUCCESS && online && dict_index_is_online_ddl(index)) {
 		switch (node->rec_type) {
 		case TRX_UNDO_DEL_MARK_REC:
 			row_log_table_insert(
diff --git a/storage/innobase/row/row0undo.cc b/storage/innobase/row/row0undo.cc
index 34b55f25527..185a71e670b 100644
--- a/storage/innobase/row/row0undo.cc
+++ b/storage/innobase/row/row0undo.cc
@@ -279,6 +279,19 @@ row_undo(
 			? UNDO_NODE_INSERT : UNDO_NODE_MODIFY;
 	}
 
+	/* Prevent prepare_inplace_alter_table_dict() from adding
+	dict_table_t::indexes while we are processing the record.
+	Recovered transactions are not protected by MDL, and the
+	secondary index creation is not protected by table locks
+	for online operation. (A table lock would only be acquired
+	when committing the ALTER TABLE operation.) */
+	const bool locked_data_dict = UNIV_UNLIKELY(trx->is_recovered)
+		&& !trx->dict_operation_lock_mode;
+
+	if (UNIV_UNLIKELY(locked_data_dict)) {
+		row_mysql_freeze_data_dictionary(trx);
+	}
+
 	dberr_t err;
 
 	if (node->state == UNDO_NODE_INSERT) {
@@ -291,6 +304,10 @@ row_undo(
 		err = row_undo_mod(node, thr);
 	}
 
+	if (UNIV_UNLIKELY(locked_data_dict)) {
+		row_mysql_unfreeze_data_dictionary(trx);
+	}
+
 	/* Do some cleanup */
 	btr_pcur_close(&(node->pcur));