MDEV-26554: Races between INSERT on child and DDL on parent table

dr-m · dr-m · commit c3c53926c467 · 2021-10-18T18:03:12.000+03:00
The SQL layer never acquires metadata locks (MDL) on the tables
that the tables that DML statement accesses is modifying.

However, the storage engine must access the parent table in order to
ensure that the child table will not refer to a non-existing record
in the parent table.

During certain DDL operations, the InnoDB table metadata (dict_table_t)
may be be freed and rebuilt. This would cause a race condition with
a concurrent INSERT that is attempting to report a FOREIGN KEY violation.

We work around the insufficient MDL during DML by acquiring exclusive
InnoDB table locks on all child tables during DDL. To avoid deadlocks,
we will follow the following order of acquisition:

1. tables whose REFERENCES clauses point to the current table
2. the current table that is being subjected to DDL
3. mysql.innodb_table_stats
4. mysql.innodb_index_stats
5. the InnoDB dictionary tables (SYS_TABLES and so on)
6. exclusive dict_sys.latch
diff --git a/mysql-test/suite/innodb/r/foreign_key.result b/mysql-test/suite/innodb/r/foreign_key.result
@@ -408,8 +408,8 @@ INSERT INTO t1 VALUES (1,2);
 CREATE TABLE x AS SELECT * FROM t1;
 ERROR XAE07: XAER_RMFAIL: The command cannot be executed when global transaction is in the  ACTIVE state
 connect  con1,localhost,root,,test;
-SET foreign_key_checks= OFF, innodb_lock_wait_timeout= 1;
-SET lock_wait_timeout=5;
+SET foreign_key_checks= OFF, innodb_lock_wait_timeout= 0;
+SET lock_wait_timeout=2;
 ALTER TABLE t1 ADD FOREIGN KEY f (a) REFERENCES t1 (pk), LOCK=EXCLUSIVE;
 ERROR HY000: Lock wait timeout exceeded; try restarting transaction
 disconnect con1;
@@ -491,7 +491,7 @@ BEGIN;
 UPDATE users SET name = 'qux' WHERE id = 1;
 connect  con1,localhost,root;
 connection con1;
-SET innodb_lock_wait_timeout= 1;
+SET innodb_lock_wait_timeout= 0;
 DELETE FROM matchmaking_groups WHERE id = 10;
 connection default;
 COMMIT;
@@ -531,9 +531,10 @@ connection con1;
 BEGIN;
 UPDATE t2 SET f = 11 WHERE id = 1;
 connection default;
-SET innodb_lock_wait_timeout= 1;
+SET innodb_lock_wait_timeout= 0;
 DELETE FROM t1 WHERE id = 1;
 ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+SET innodb_lock_wait_timeout= 1;
 connection con1;
 COMMIT;
 connection default;
@@ -897,3 +898,30 @@ create or replace table t2 (pk int primary key, a varchar(4096) unique, foreign
 ERROR HY000: Can't create table `test`.`t2` (errno: 150 "Foreign key constraint is incorrectly formed")
 drop table t1;
 # End of 10.5 tests
+#
+# MDEV-26554 Table-rebuilding DDL on parent table causes crash
+#            for INSERT into child table
+#
+CREATE TABLE parent(a INT PRIMARY KEY) ENGINE=InnoDB;
+CREATE TABLE child(a INT PRIMARY KEY REFERENCES parent(a)) ENGINE=InnoDB;
+connect  con1, localhost, root,,;
+BEGIN;
+INSERT INTO child SET a=1;
+ERROR 23000: Cannot add or update a child row: a foreign key constraint fails (`test`.`child`, CONSTRAINT `child_ibfk_1` FOREIGN KEY (`a`) REFERENCES `parent` (`a`))
+connection default;
+SET innodb_lock_wait_timeout=0, foreign_key_checks=0;
+TRUNCATE TABLE parent;
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+ALTER TABLE parent FORCE, ALGORITHM=COPY;
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+ALTER TABLE parent FORCE, ALGORITHM=INPLACE;
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+ALTER TABLE parent ADD COLUMN b INT, ALGORITHM=INSTANT;
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+disconnect con1;
+TRUNCATE TABLE parent;
+ALTER TABLE parent FORCE, ALGORITHM=COPY;
+ALTER TABLE parent FORCE, ALGORITHM=INPLACE;
+ALTER TABLE parent ADD COLUMN b INT, ALGORITHM=INSTANT;
+DROP TABLE child, parent;
+# End of 10.6 tests
diff --git a/mysql-test/suite/innodb/r/row_format_redundant.result b/mysql-test/suite/innodb/r/row_format_redundant.result
@@ -75,7 +75,7 @@ DROP TABLE t1;
 Warnings:
 Warning	1932	Table 'test.t1' doesn't exist in engine
 DROP TABLE t2,t3;
-FOUND 5 /\[ERROR\] InnoDB: Table test/t1 in InnoDB data dictionary contains invalid flags\. SYS_TABLES\.TYPE=1 SYS_TABLES\.MIX_LEN=511\b/ in mysqld.1.err
+FOUND 6 /\[ERROR\] InnoDB: Table test/t1 in InnoDB data dictionary contains invalid flags\. SYS_TABLES\.TYPE=1 SYS_TABLES\.MIX_LEN=511\b/ in mysqld.1.err
 # restart
 ib_buffer_pool
 ib_logfile0
diff --git a/mysql-test/suite/innodb/r/truncate_foreign.result b/mysql-test/suite/innodb/r/truncate_foreign.result
@@ -43,16 +43,19 @@ SET DEBUG_SYNC='foreign_constraint_check_for_ins SIGNAL fk WAIT_FOR go';
 INSERT INTO child SET a=5;
 connection default;
 SET DEBUG_SYNC='now WAIT_FOR fk';
-SET foreign_key_checks=0;
+SET foreign_key_checks=0, innodb_lock_wait_timeout=0;
 TRUNCATE TABLE parent;
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
 SET DEBUG_SYNC='now SIGNAL go';
 connection dml;
-ERROR 23000: Cannot add or update a child row: a foreign key constraint fails (`test`.`child`, CONSTRAINT `child_ibfk_1` FOREIGN KEY (`a`) REFERENCES `parent` (`a`) ON UPDATE CASCADE)
 SELECT * FROM parent;
 a
+3
+5
 SELECT * FROM child;
 a
 3
+5
 disconnect dml;
 connection default;
 SET DEBUG_SYNC = RESET;
diff --git a/mysql-test/suite/innodb/t/foreign_key.test b/mysql-test/suite/innodb/t/foreign_key.test
@@ -411,8 +411,8 @@ INSERT INTO t1 VALUES (1,2);
 --error ER_XAER_RMFAIL
 CREATE TABLE x AS SELECT * FROM t1;
 --connect (con1,localhost,root,,test)
-SET foreign_key_checks= OFF, innodb_lock_wait_timeout= 1;
-SET lock_wait_timeout=5;
+SET foreign_key_checks= OFF, innodb_lock_wait_timeout= 0;
+SET lock_wait_timeout=2;
 --error ER_LOCK_WAIT_TIMEOUT
 ALTER TABLE t1 ADD FOREIGN KEY f (a) REFERENCES t1 (pk), LOCK=EXCLUSIVE;# Cleanup
 --disconnect con1
@@ -506,7 +506,7 @@ UPDATE users SET name = 'qux' WHERE id = 1;
 
 connect (con1,localhost,root);
 --connection con1
-SET innodb_lock_wait_timeout= 1;
+SET innodb_lock_wait_timeout= 0;
 DELETE FROM matchmaking_groups WHERE id = 10;
 
 --connection default
@@ -544,9 +544,10 @@ BEGIN;
 UPDATE t2 SET f = 11 WHERE id = 1;
 
 --connection default
-SET innodb_lock_wait_timeout= 1;
+SET innodb_lock_wait_timeout= 0;
 --error ER_LOCK_WAIT_TIMEOUT
 DELETE FROM t1 WHERE id = 1;
+SET innodb_lock_wait_timeout= 1;
 
 --connection con1
 COMMIT;
@@ -902,4 +903,34 @@ drop table t1;
 
 --echo # End of 10.5 tests
 
+--echo #
+--echo # MDEV-26554 Table-rebuilding DDL on parent table causes crash
+--echo #            for INSERT into child table
+--echo #
+
+CREATE TABLE parent(a INT PRIMARY KEY) ENGINE=InnoDB;
+CREATE TABLE child(a INT PRIMARY KEY REFERENCES parent(a)) ENGINE=InnoDB;
+connect (con1, localhost, root,,);
+BEGIN;
+--error ER_NO_REFERENCED_ROW_2
+INSERT INTO child SET a=1;
+connection default;
+SET innodb_lock_wait_timeout=0, foreign_key_checks=0;
+--error ER_LOCK_WAIT_TIMEOUT
+TRUNCATE TABLE parent;
+--error ER_LOCK_WAIT_TIMEOUT
+ALTER TABLE parent FORCE, ALGORITHM=COPY;
+--error ER_LOCK_WAIT_TIMEOUT
+ALTER TABLE parent FORCE, ALGORITHM=INPLACE;
+--error ER_LOCK_WAIT_TIMEOUT
+ALTER TABLE parent ADD COLUMN b INT, ALGORITHM=INSTANT;
+disconnect con1;
+TRUNCATE TABLE parent;
+ALTER TABLE parent FORCE, ALGORITHM=COPY;
+ALTER TABLE parent FORCE, ALGORITHM=INPLACE;
+ALTER TABLE parent ADD COLUMN b INT, ALGORITHM=INSTANT;
+DROP TABLE child, parent;
+
+--echo # End of 10.6 tests
+
 --source include/wait_until_count_sessions.inc
diff --git a/mysql-test/suite/innodb/t/truncate_foreign.test b/mysql-test/suite/innodb/t/truncate_foreign.test
@@ -52,12 +52,12 @@ send INSERT INTO child SET a=5;
 
 connection default;
 SET DEBUG_SYNC='now WAIT_FOR fk';
-SET foreign_key_checks=0;
+SET foreign_key_checks=0, innodb_lock_wait_timeout=0;
+--error ER_LOCK_WAIT_TIMEOUT
 TRUNCATE TABLE parent;
 SET DEBUG_SYNC='now SIGNAL go';
 
 connection dml;
---error ER_NO_REFERENCED_ROW_2
 reap;
 SELECT * FROM parent;
 SELECT * FROM child;
diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc
@@ -13645,7 +13645,6 @@ static dberr_t innobase_rename_table(trx_t *trx, const char *from,
 
 	DEBUG_SYNC_C("innodb_rename_table_ready");
 
-	trx_start_if_not_started(trx, true);
 	ut_ad(trx->will_lock);
 
 	error = row_rename_table_for_mysql(norm_from, norm_to, trx, use_fk);
@@ -13782,7 +13781,23 @@ int ha_innobase::truncate()
 	dict_table_t *table_stats = nullptr, *index_stats = nullptr;
 	MDL_ticket *mdl_table = nullptr, *mdl_index = nullptr;
 
-	dberr_t error = lock_table_for_trx(ib_table, trx, LOCK_X);
+	dberr_t error = DB_SUCCESS;
+
+	dict_sys.freeze(SRW_LOCK_CALL);
+	for (const dict_foreign_t* f : ib_table->referenced_set) {
+		if (dict_table_t* child = f->foreign_table) {
+			error = lock_table_for_trx(child, trx, LOCK_X);
+			if (error != DB_SUCCESS) {
+				break;
+			}
+		}
+	}
+	dict_sys.unfreeze();
+
+	if (error == DB_SUCCESS) {
+		error = lock_table_for_trx(ib_table, trx, LOCK_X);
+	}
+
 	const bool fts = error == DB_SUCCESS
 		&& ib_table->flags2 & (DICT_TF2_FTS_HAS_DOC_ID | DICT_TF2_FTS);
 
@@ -13945,6 +13960,26 @@ ha_innobase::rename_table(
 
 	dberr_t error = DB_SUCCESS;
 
+	if (dict_table_t::is_temporary_name(norm_from)) {
+		/* There is no need to lock any FOREIGN KEY child tables. */
+	} else if (dict_table_t *table = dict_table_open_on_name(
+		    norm_from, false, DICT_ERR_IGNORE_FK_NOKEY)) {
+		dict_sys.freeze(SRW_LOCK_CALL);
+		for (const dict_foreign_t* f : table->referenced_set) {
+			if (dict_table_t* child = f->foreign_table) {
+				error = lock_table_for_trx(child, trx, LOCK_X);
+				if (error != DB_SUCCESS) {
+					break;
+				}
+			}
+		}
+		dict_sys.unfreeze();
+		if (error == DB_SUCCESS) {
+			error = lock_table_for_trx(table, trx, LOCK_X);
+		}
+		table->release();
+	}
+
 	if (strcmp(norm_from, TABLE_STATS_NAME)
 	    && strcmp(norm_from, INDEX_STATS_NAME)
 	    && strcmp(norm_to, TABLE_STATS_NAME)
@@ -13966,7 +14001,7 @@ ha_innobase::rename_table(
 			dict_sys.unfreeze();
 		}
 
-		if (table_stats && index_stats
+		if (error == DB_SUCCESS && table_stats && index_stats
 		    && !strcmp(table_stats->name.m_name, TABLE_STATS_NAME)
 		    && !strcmp(index_stats->name.m_name, INDEX_STATS_NAME) &&
 		    !(error = lock_table_for_trx(table_stats, trx, LOCK_X))) {
diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc
@@ -10847,12 +10847,24 @@ ha_innobase::commit_inplace_alter_table(
 
 	for (inplace_alter_handler_ctx** pctx = ctx_array; *pctx; pctx++) {
 		auto ctx = static_cast<ha_innobase_inplace_ctx*>(*pctx);
+		dberr_t error = DB_SUCCESS;
 
 		if (new_clustered && ctx->old_table->fts) {
 			ut_ad(!ctx->old_table->fts->add_wq);
 			fts_optimize_remove_table(ctx->old_table);
 		}
 
+		dict_sys.freeze(SRW_LOCK_CALL);
+		for (auto f : ctx->old_table->referenced_set) {
+			if (dict_table_t* child = f->foreign_table) {
+				error = lock_table_for_trx(child, trx, LOCK_X);
+				if (error != DB_SUCCESS) {
+					break;
+				}
+			}
+		}
+		dict_sys.unfreeze();
+
 		if (ctx->new_table->fts) {
 			ut_ad(!ctx->new_table->fts->add_wq);
 			fts_optimize_remove_table(ctx->new_table);
@@ -10863,9 +10875,12 @@ ha_innobase::commit_inplace_alter_table(
 		transaction is holding locks on the table while we
 		change the table definition. Any recovered incomplete
 		transactions would be holding InnoDB locks only, not MDL. */
+		if (error == DB_SUCCESS) {
+			error = lock_table_for_trx(ctx->new_table, trx,
+						   LOCK_X);
+		}
 
-		if (dberr_t error = lock_table_for_trx(ctx->new_table, trx,
-						       LOCK_X)) {
+		if (error != DB_SUCCESS) {
 lock_fail:
 			my_error_innodb(
 				error, table_share->table_name.str, 0);
