MDEV-15641 InnoDB crash while committing table-rebuilding ALTER TABLE

Problem:
========
 There is a possibility that there can be more concurrent DMLs While the
alter table thread is waiting for upgrading to MDL_EXCLUSIVE before commit phase.
In commit phase, InnoDB acquires dict_operation_lock and it already holds MDL_EXCLUSIVE
on the table. After that, InnoDB applies the concurrent DML logs in commit phase.
This could lead to blocking of the following things:

  1) DML on the particular table (due to MDL_EXCLUSIVE on the table)
  2) InnoDB DDLs (due to dict_operation_lock)
  3) Purge thread, stats thread, the master thread (due to dict_operation_lock)

Fix:
====
Apply the concurrent DML logs in commit phase but before acquiring
dict_operation_lock in commit phase. It makes sure that (2), (3) can't be
blocked for longer time.

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

@@ -0,0 +1,49 @@
+CREATE TABLE t1(f1 char(200), f2 char(200), f3 char(200),
+f4 char(200), f5 char(200), f6 char(200),
+f7 char(200), f8 char(200))ENGINE=InnoDB;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+SET DEBUG_SYNC = 'inplace_after_index_build SIGNAL rebuilt WAIT_FOR dml_pause';
+SET DEBUG_SYNC = 'alter_table_inplace_before_lock_upgrade SIGNAL dml_restart WAIT_FOR  dml_done';
+SET DEBUG_SYNC = 'row_log_table_apply2_before SIGNAL ddl_start';
+ALTER TABLE t1 FORCE, ALGORITHM=INPLACE;
+connect con1,localhost,root,,test;
+SET DEBUG_SYNC = 'now WAIT_FOR rebuilt';
+BEGIN;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+SET DEBUG_SYNC = 'now SIGNAL dml_pause';
+SET DEBUG_SYNC = 'now WAIT_FOR dml_restart';
+ROLLBACK;
+BEGIN;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+ROLLBACK;
+BEGIN;
+INSERT INTO t1 SELECT * FROM t1;
+INSERT INTO t1 SELECT * FROM t1;
+INSERT INTO t1 SELECT * FROM t1;
+ROLLBACK;
+SET DEBUG_SYNC = 'now SIGNAL dml_done';
+connect con2, localhost,root,,test;
+SET DEBUG_SYNC = 'now WAIT_FOR ddl_start';
+CREATE TABLE t2(f1 INT NOT NULL)ENGINE=InnoDB;
+connection default;
+SHOW CREATE TABLE t1;
+Table	Create Table
+t1	CREATE TABLE `t1` (
+  `f1` char(200) DEFAULT NULL,
+  `f2` char(200) DEFAULT NULL,
+  `f3` char(200) DEFAULT NULL,
+  `f4` char(200) DEFAULT NULL,
+  `f5` char(200) DEFAULT NULL,
+  `f6` char(200) DEFAULT NULL,
+  `f7` char(200) DEFAULT NULL,
+  `f8` char(200) DEFAULT NULL
+) ENGINE=InnoDB DEFAULT CHARSET=latin1
+SELECT COUNT(*) FROM t1;
+COUNT(*)
+16384
+SET DEBUG_SYNC = 'RESET';
+DROP TABLE t1, t2;

mysql-test/suite/innodb/t/alter_large_dml.opt

@@ -0,0 +1,2 @@
+--innodb_fatal_semaphore_wait_threshold=20
+--innodb_online_alter_log_max_size=536870912

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

@@ -0,0 +1,53 @@
+--source include/big_test.inc
+--source include/have_innodb.inc
+--source include/have_debug.inc
+--source include/have_debug_sync.inc
+--source include/have_sequence.inc
+
+CREATE TABLE t1(f1 char(200), f2 char(200), f3 char(200),
+		f4 char(200), f5 char(200), f6 char(200),
+		f7 char(200), f8 char(200))ENGINE=InnoDB;
+
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+
+SET DEBUG_SYNC = 'inplace_after_index_build SIGNAL rebuilt WAIT_FOR dml_pause';
+SET DEBUG_SYNC = 'alter_table_inplace_before_lock_upgrade SIGNAL dml_restart WAIT_FOR  dml_done';
+SET DEBUG_SYNC = 'row_log_table_apply2_before SIGNAL ddl_start';
+--send
+ALTER TABLE t1 FORCE, ALGORITHM=INPLACE;
+
+--connect(con1,localhost,root,,test)
+SET DEBUG_SYNC = 'now WAIT_FOR rebuilt';
+BEGIN;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+SET DEBUG_SYNC = 'now SIGNAL dml_pause';
+SET DEBUG_SYNC = 'now WAIT_FOR dml_restart';
+ROLLBACK;
+
+BEGIN;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+INSERT INTO t1 SELECT '','','','','','','','' FROM seq_1_to_16384;
+ROLLBACK;
+
+BEGIN;
+INSERT INTO t1 SELECT * FROM t1;
+INSERT INTO t1 SELECT * FROM t1;
+INSERT INTO t1 SELECT * FROM t1;
+ROLLBACK;
+
+SET DEBUG_SYNC = 'now SIGNAL dml_done';
+
+--connect(con2, localhost,root,,test)
+SET DEBUG_SYNC = 'now WAIT_FOR ddl_start';
+CREATE TABLE t2(f1 INT NOT NULL)ENGINE=InnoDB;
+
+connection default;
+reap;
+SHOW CREATE TABLE t1;
+
+SELECT COUNT(*) FROM t1;
+SET DEBUG_SYNC = 'RESET';
+DROP TABLE t1, t2;

sql/sql_table.cc

@@ -7369,6 +7369,7 @@ static bool mysql_inplace_alter_table(THD *thd,
   if (res)
     goto rollback;
 
+  DEBUG_SYNC(thd, "alter_table_inplace_before_lock_upgrade");
   // Upgrade to EXCLUSIVE before commit.
   if (wait_while_table_is_used(thd, table, HA_EXTRA_PREPARE_FOR_RENAME))
     goto rollback;

storage/innobase/handler/handler0alter.cc

@@ -7534,73 +7534,6 @@ commit_try_rebuild(
 		index->to_be_dropped = 0;
 	}
 
-	/* We copied the table. Any indexes that were requested to be
-	dropped were not created in the copy of the table. Apply any
-	last bit of the rebuild log and then rename the tables. */
-
-	if (ctx->online) {
-		DEBUG_SYNC_C("row_log_table_apply2_before");
-
-		dict_vcol_templ_t* s_templ  = NULL;
-
-		if (ctx->new_table->n_v_cols > 0) {
-			s_templ = UT_NEW_NOKEY(
-					dict_vcol_templ_t());
-			s_templ->vtempl = NULL;
-
-			innobase_build_v_templ(
-				altered_table, ctx->new_table, s_templ,
-				NULL, true);
-			ctx->new_table->vc_templ = s_templ;
-		}
-
-		error = row_log_table_apply(
-			ctx->thr, user_table, altered_table,
-			static_cast<ha_innobase_inplace_ctx*>(
-				ha_alter_info->handler_ctx)->m_stage);
-
-		if (s_templ) {
-			ut_ad(ctx->need_rebuild());
-			dict_free_vc_templ(s_templ);
-			UT_DELETE(s_templ);
-			ctx->new_table->vc_templ = NULL;
-		}
-
-		ulint	err_key = thr_get_trx(ctx->thr)->error_key_num;
-
-		switch (error) {
-			KEY*	dup_key;
-		case DB_SUCCESS:
-			break;
-		case DB_DUPLICATE_KEY:
-			if (err_key == ULINT_UNDEFINED) {
-				/* This should be the hidden index on
-				FTS_DOC_ID. */
-				dup_key = NULL;
-			} else {
-				DBUG_ASSERT(err_key <
-					    ha_alter_info->key_count);
-				dup_key = &ha_alter_info
-					->key_info_buffer[err_key];
-			}
-			print_keydup_error(altered_table, dup_key, MYF(0));
-			DBUG_RETURN(true);
-		case DB_ONLINE_LOG_TOO_BIG:
-			my_error(ER_INNODB_ONLINE_LOG_TOO_BIG, MYF(0),
-				 get_error_key_name(err_key, ha_alter_info,
-						    rebuilt_table));
-			DBUG_RETURN(true);
-		case DB_INDEX_CORRUPT:
-			my_error(ER_INDEX_CORRUPT, MYF(0),
-				 get_error_key_name(err_key, ha_alter_info,
-						    rebuilt_table));
-			DBUG_RETURN(true);
-		default:
-			my_error_innodb(error, table_name, user_table->flags);
-			DBUG_RETURN(true);
-		}
-	}
-
 	if ((ha_alter_info->handler_flags
 	     & Alter_inplace_info::ALTER_COLUMN_NAME)
 	    && innobase_rename_columns_try(ha_alter_info, ctx, old_table,
@@ -8127,6 +8060,90 @@ do {								\
 # define DBUG_INJECT_CRASH(prefix, count)
 #endif
 
+/** Apply the log for the table rebuild operation.
+@param[in]	ctx		Inplace Alter table context
+@param[in]	altered_table	MySQL table that is being altered
+@return true Failure, else false. */
+static bool alter_rebuild_apply_log(
+	ha_innobase_inplace_ctx*	ctx,
+	Alter_inplace_info*		ha_alter_info,
+	TABLE*				altered_table)
+{
+	DBUG_ENTER("alter_rebuild_apply_log");
+
+	if (!ctx->online) {
+		DBUG_RETURN(false);
+	}
+
+	/* We copied the table. Any indexes that were requested to be
+	dropped were not created in the copy of the table. Apply any
+	last bit of the rebuild log and then rename the tables. */
+	dict_table_t*	user_table = ctx->old_table;
+	dict_table_t*	rebuilt_table = ctx->new_table;
+
+	DEBUG_SYNC_C("row_log_table_apply2_before");
+
+	dict_vcol_templ_t* s_templ  = NULL;
+
+	if (ctx->new_table->n_v_cols > 0) {
+		s_templ = UT_NEW_NOKEY(
+				dict_vcol_templ_t());
+		s_templ->vtempl = NULL;
+
+		innobase_build_v_templ(altered_table, ctx->new_table, s_templ,
+				       NULL, true);
+		ctx->new_table->vc_templ = s_templ;
+	}
+
+	dberr_t error = row_log_table_apply(
+		ctx->thr, user_table, altered_table,
+		static_cast<ha_innobase_inplace_ctx*>(
+			ha_alter_info->handler_ctx)->m_stage);
+
+	if (s_templ) {
+		ut_ad(ctx->need_rebuild());
+		dict_free_vc_templ(s_templ);
+		UT_DELETE(s_templ);
+		ctx->new_table->vc_templ = NULL;
+	}
+
+	ulint	err_key = thr_get_trx(ctx->thr)->error_key_num;
+
+	switch (error) {
+		KEY*	dup_key;
+	case DB_SUCCESS:
+		break;
+	case DB_DUPLICATE_KEY:
+		if (err_key == ULINT_UNDEFINED) {
+			/* This should be the hidden index on
+			   FTS_DOC_ID. */
+			dup_key = NULL;
+		} else {
+			DBUG_ASSERT(err_key < ha_alter_info->key_count);
+			dup_key = &ha_alter_info->key_info_buffer[err_key];
+		}
+
+		print_keydup_error(altered_table, dup_key, MYF(0));
+		DBUG_RETURN(true);
+	case DB_ONLINE_LOG_TOO_BIG:
+		my_error(ER_INNODB_ONLINE_LOG_TOO_BIG, MYF(0),
+			 get_error_key_name(err_key, ha_alter_info,
+					    rebuilt_table));
+		DBUG_RETURN(true);
+	case DB_INDEX_CORRUPT:
+		my_error(ER_INDEX_CORRUPT, MYF(0),
+			 get_error_key_name(err_key, ha_alter_info,
+					    rebuilt_table));
+		DBUG_RETURN(true);
+	default:
+		my_error_innodb(error, ctx->old_table->name.m_name,
+				user_table->flags);
+		DBUG_RETURN(true);
+	}
+
+	DBUG_RETURN(false);
+}
+
 /** Commit or rollback the changes made during
 prepare_inplace_alter_table() and inplace_alter_table() inside
 the storage engine. Note that the allowed level of concurrency
@@ -8271,6 +8288,19 @@ ha_innobase::commit_inplace_alter_table(
 			ut_ad(!ctx->new_table->fts->add_wq);
 			fts_optimize_remove_table(ctx->new_table);
 		}
+
+		/* Apply the online log of the table before acquiring
+		data dictionary latches. Here alter thread already acquired
+		MDL_EXCLUSIVE on the table. So there can't be anymore DDLs, DMLs
+		for the altered table. By applying the log here, InnoDB
+		makes sure that concurrent DDLs, purge thread or any other
+		background thread doesn't wait for the dict_operation_lock
+		for longer time. */
+		if (new_clustered && commit
+		    && alter_rebuild_apply_log(
+				ctx, ha_alter_info, altered_table)) {
+			DBUG_RETURN(true);
+		}
 	}
 
 	if (!trx) {