MDEV-20938 Double free of dict_foreign_t during instant ALTER TABLE

innobase_drop_foreign_try(): Don't evict and reload the dict_foreign_t
during instant ALTER TABLE if the FOREIGN KEY constraint is being
dropped.

The MDEV-19630 fix (commit 07b1a26)
was incomplete, because it did not cover a case where the
FOREIGN KEY constraint is being dropped.

Author: Thirunarayanan

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

@@ -128,6 +128,10 @@ HANDLER h READ `PRIMARY` PREV WHERE 0;
 pk	f1	f2	f3	f4	f5	f6	f7	f8	filler
 HANDLER h CLOSE;
 DROP TABLE t1;
+#
+# MDEV-19630 ALTER TABLE ... ADD COLUMN damages foreign keys
+# which are pointed to the table being altered
+#
 CREATE TABLE t1(f1 int not null, primary key(f1))engine=innodb;
 CREATE TABLE t2(f1 INT AUTO_INCREMENT NOT NULL, f2 INT NOT NULL,
 status ENUM ('a', 'b', 'c'), INDEX idx1(f2),
@@ -154,3 +158,14 @@ t2	CREATE TABLE `t2` (
 ) ENGINE=InnoDB DEFAULT CHARSET=latin1
 ALTER TABLE t2 CHANGE status status VARCHAR(20) DEFAULT NULL;
 DROP TABLE t2, t1;
+#
+# MDEV-20938 Double free of dict_foreign_t during instant ALTER TABLE
+#
+CREATE TABLE t1 (id INT UNSIGNED PRIMARY KEY) ENGINE=InnoDB;
+CREATE TABLE t2 (a INT UNSIGNED PRIMARY KEY, b INT UNSIGNED UNIQUE,
+FOREIGN KEY fk1 (b) REFERENCES t1 (id)) ENGINE=InnoDB;
+ALTER TABLE t2
+DROP FOREIGN KEY fk1,
+CHANGE b d INT UNSIGNED,
+ADD c INT;
+DROP TABLE t2, t1;

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

@@ -135,8 +135,10 @@ HANDLER h READ `PRIMARY` PREV WHERE 0;
 HANDLER h CLOSE;
 DROP TABLE t1;
 
-# MDEV-19630 ALTER TABLE ... ADD COLUMN damages foreign keys which are pointed
-#		to the table being altered
+--echo #
+--echo # MDEV-19630 ALTER TABLE ... ADD COLUMN damages foreign keys
+--echo # which are pointed to the table being altered
+--echo #
 CREATE TABLE t1(f1 int not null, primary key(f1))engine=innodb;
 CREATE TABLE t2(f1 INT AUTO_INCREMENT NOT NULL, f2 INT NOT NULL,
 		status ENUM ('a', 'b', 'c'), INDEX idx1(f2),
@@ -154,3 +156,16 @@ DROP TABLE t2, t1;
 
 --let $datadir= `select @@datadir`
 --remove_file $datadir/test/load.data
+
+--echo #
+--echo # MDEV-20938 Double free of dict_foreign_t during instant ALTER TABLE
+--echo #
+
+CREATE TABLE t1 (id INT UNSIGNED PRIMARY KEY) ENGINE=InnoDB;
+CREATE TABLE t2 (a INT UNSIGNED PRIMARY KEY, b INT UNSIGNED UNIQUE,
+                 FOREIGN KEY fk1 (b) REFERENCES t1 (id)) ENGINE=InnoDB;
+ALTER TABLE t2
+    DROP FOREIGN KEY fk1,
+    CHANGE b d INT UNSIGNED,
+    ADD c INT;
+DROP TABLE t2, t1;

storage/innobase/handler/handler0alter.cc

@@ -7428,27 +7428,23 @@ innobase_drop_foreign_try(
 }
 
 /** Rename a column in the data dictionary tables.
-@param[in] user_table		InnoDB table that was being altered
-@param[in] trx			Data dictionary transaction
+@param[in] ctx			ALTER TABLE context
+@param[in,out] trx		Data dictionary transaction
 @param[in] table_name		Table name in MySQL
 @param[in] nth_col		0-based index of the column
 @param[in] from			old column name
 @param[in] to			new column name
-@param[in] new_clustered	whether the table has been rebuilt
-@param[in] evict_fk_cache	Evict the fk info from cache
 @retval true Failure
 @retval false Success */
 static MY_ATTRIBUTE((nonnull, warn_unused_result))
 bool
 innobase_rename_column_try(
-	const dict_table_t*	user_table,
-	trx_t*			trx,
-	const char*		table_name,
-	ulint			nth_col,
-	const char*		from,
-	const char*		to,
-	bool			new_clustered,
-	bool			evict_fk_cache)
+	const ha_innobase_inplace_ctx&	ctx,
+	trx_t*				trx,
+	const char*			table_name,
+	ulint				nth_col,
+	const char*			from,
+	const char*			to)
 {
 	pars_info_t*	info;
 	dberr_t		error;
@@ -7460,13 +7456,13 @@ innobase_rename_column_try(
 	ut_ad(mutex_own(&dict_sys->mutex));
 	ut_ad(rw_lock_own(&dict_operation_lock, RW_LOCK_X));
 
-	if (new_clustered) {
+	if (ctx.need_rebuild()) {
 		goto rename_foreign;
 	}
 
 	info = pars_info_create();
 
-	pars_info_add_ull_literal(info, "tableid", user_table->id);
+	pars_info_add_ull_literal(info, "tableid", ctx.old_table->id);
 	pars_info_add_int4_literal(info, "nth", nth_col);
 	pars_info_add_str_literal(info, "new", to);
 
@@ -7496,7 +7492,7 @@ innobase_rename_column_try(
 	trx->op_info = "renaming column in SYS_FIELDS";
 
 	for (const dict_index_t* index = dict_table_get_first_index(
-		     user_table);
+		     ctx.old_table);
 	     index != NULL;
 	     index = dict_table_get_next_index(index)) {
 
@@ -7549,8 +7545,8 @@ innobase_rename_column_try(
 	std::set<dict_foreign_t*> fk_evict;
 	bool		foreign_modified;
 
-	for (dict_foreign_set::const_iterator it = user_table->foreign_set.begin();
-	     it != user_table->foreign_set.end();
+	for (dict_foreign_set::const_iterator it = ctx.old_table->foreign_set.begin();
+	     it != ctx.old_table->foreign_set.end();
 	     ++it) {
 
 		dict_foreign_t*	foreign = *it;
@@ -7563,6 +7559,14 @@ innobase_rename_column_try(
 				continue;
 			}
 
+			/* Ignore the foreign key rename if fk info
+			is being dropped. */
+			if (innobase_dropping_foreign(
+				    foreign, ctx.drop_fk,
+				    ctx.num_to_drop_fk)) {
+				continue;
+			}
+
 			info = pars_info_create();
 
 			pars_info_add_str_literal(info, "id", foreign->id);
@@ -7591,8 +7595,8 @@ innobase_rename_column_try(
 	}
 
 	for (dict_foreign_set::const_iterator it
-		= user_table->referenced_set.begin();
-	     it != user_table->referenced_set.end();
+		= ctx.old_table->referenced_set.begin();
+	     it != ctx.old_table->referenced_set.end();
 	     ++it) {
 
 		foreign_modified = false;
@@ -7633,7 +7637,7 @@ innobase_rename_column_try(
 	}
 
 	/* Reload the foreign key info for instant table too. */
-	if (new_clustered || evict_fk_cache) {
+	if (ctx.need_rebuild() || ctx.is_instant()) {
 		std::for_each(fk_evict.begin(), fk_evict.end(),
 			      dict_foreign_remove_from_cache);
 	}
@@ -7684,12 +7688,10 @@ innobase_rename_columns_try(
 						: i - num_v;
 
 				if (innobase_rename_column_try(
-					    ctx->old_table, trx, table_name,
+					    *ctx, trx, table_name,
 					    col_n,
 					    cf->field->field_name.str,
-					    cf->field_name.str,
-					    ctx->need_rebuild(),
-					    ctx->is_instant())) {
+					    cf->field_name.str)) {
 					return(true);
 				}
 				goto processed_field;