Remove dict_table_t::rollback_instant(unsigned n)

On rollback of changes to SYS_COLUMNS, we can no longer assume that
the change was the instant addition of a column at the end of the
column list. The function dict_table_t::rollback_instant(unsigned n)
was inherently incompatible with instantly dropping or reordering
columns.

When a change to SYS_COLUMNS is rolled back, we must simply evict
the affected table definition, at the end of the rollback. We cannot
free the table object immediately, because the current transaction
that is being rolled back may be holding locks on the table.

dict_table_remove_from_cache_low(): Replaced
by dict_table_remove_from_cache().

dict_table_remove_from_cache(): Add a third parameter keep=false,
so that the table can be freed by the caller.

dict_table_t::rollback_instant(): Add the parameter old_n_fields,
and simplify some logic.

trx_lock_t::evicted_tables: List of tables on which trx_t::evict_table()
was invoked.

trx_t::evict_table(): Evict a table definition during rollback.

trx_commit_in_memory(): Empty the trx->lock.evicted_tables list
after the locks were released, by freeing the table objects.

row_undo_ins_remove_clust_rec: Invoke trx_t::evict_table() on the
affected table if a change to SYS_COLUMNS is being rolled back.

row_undo_mod_clust_low(): FIXME: do the same

storage/innobase/dict/dict0dict.cc

@@ -1435,7 +1435,7 @@ dict_make_room_in_cache(
 					ut_ad(0);
 				  }
 			};);
-			dict_table_remove_from_cache_low(table, TRUE);
+			dict_table_remove_from_cache(table, true);
 
 			++n_evicted;
 		}
@@ -1969,14 +1969,11 @@ dict_table_change_id_in_cache(
 		    ut_fold_ull(table->id), table);
 }
 
-/**********************************************************************//**
-Removes a table object from the dictionary cache. */
-void
-dict_table_remove_from_cache_low(
-/*=============================*/
-	dict_table_t*	table,		/*!< in, own: table */
-	ibool		lru_evict)	/*!< in: TRUE if table being evicted
-					to make room in the table LRU list */
+/** Evict a table definition from the InnoDB data dictionary cache.
+@param[in,out]	table	cached table definition to be evicted
+@param[in]	lru	whether this is part of least-recently-used eviction
+@param[in]	keep	whether to keep (not free) the object */
+void dict_table_remove_from_cache(dict_table_t* table, bool lru, bool keep)
 {
 	dict_foreign_t*	foreign;
 	dict_index_t*	index;
@@ -2009,7 +2006,7 @@ dict_table_remove_from_cache_low(
 	     index != NULL;
 	     index = UT_LIST_GET_LAST(table->indexes)) {
 
-		dict_index_remove_from_cache_low(table, index, lru_evict);
+		dict_index_remove_from_cache_low(table, index, lru);
 	}
 
 	/* Remove table from the hash tables of tables */
@@ -2031,7 +2028,7 @@ dict_table_remove_from_cache_low(
 
 	ut_ad(dict_lru_validate());
 
-	if (lru_evict && table->drop_aborted) {
+	if (lru && table->drop_aborted) {
 		/* When evicting the table definition,
 		drop the orphan indexes from the data dictionary
 		and free the index pages. */
@@ -2056,17 +2053,9 @@ dict_table_remove_from_cache_low(
 		UT_DELETE(table->vc_templ);
 	}
 
+	if (!keep) {
 		dict_mem_table_free(table);
-}
-
-/**********************************************************************//**
-Removes a table object from the dictionary cache. */
-void
-dict_table_remove_from_cache(
-/*=========================*/
-	dict_table_t*	table)	/*!< in, own: table */
-{
-	dict_table_remove_from_cache_low(table, FALSE);
+	}
 }
 
 /****************************************************************//**

storage/innobase/dict/dict0mem.cc

@@ -1601,7 +1601,8 @@ ulint find_old_col_no(
 @param[in]	old_cols		original cols
 @param[in]	old_col_names		original col_names
 @param[in]	old_instant		original instant structure
-@param[in]	old_clust_fields	original clustered fields
+@param[in]	old_fields		original fields
+@param[in]	old_n_fields		original number of fields
 @param[in]	col_map			column map */
 void
 dict_table_t::rollback_instant(
@@ -1609,7 +1610,8 @@ dict_table_t::rollback_instant(
 	dict_col_t*	old_cols,
 	const char*	old_col_names,
 	dict_instant_t*	old_instant,
-	dict_field_t*	old_clust_fields,
+	dict_field_t*	old_fields,
+	unsigned	old_n_fields,
 	const ulint*	col_map)
 {
 	ut_ad(mutex_own(&dict_sys->mutex));
@@ -1619,6 +1621,9 @@ dict_table_t::rollback_instant(
 	DBUG_ASSERT(old_n_cols >= DATA_N_SYS_COLS);
 	DBUG_ASSERT(n_cols == n_def);
 	DBUG_ASSERT(index->n_def == index->n_fields);
+	DBUG_ASSERT(index->n_core_fields <= old_n_fields);
+	DBUG_ASSERT(index->n_core_fields <= index->n_fields);
+	DBUG_ASSERT(instant || !old_instant);
 
 	unsigned n_remove = 0;
 	unsigned n_add = 0;
@@ -1633,25 +1638,13 @@ dict_table_t::rollback_instant(
 
 	index->n_nullable = 0;
 
-	ulint	old_n_clust_fields = old_n_cols;
-
-	if (old_instant) {
-		old_n_clust_fields += old_instant->n_dropped;
-	}
-
-	for (unsigned i = 0; i < old_n_clust_fields; i++) {
-		if (old_clust_fields[i].col->is_nullable()) {
+	for (unsigned i = 0; i < old_n_fields; i++) {
+		if (old_fields[i].col->is_nullable()) {
 			index->n_nullable++;
 		}
 	}
 
-	index->n_fields = old_n_clust_fields;
-	index->n_def = index->n_fields;
-	if (index->n_core_fields > index->n_fields) {
-		index->n_core_fields = index->n_fields;
-		index->n_core_null_bytes
-			= UT_BITS_IN_BYTES(unsigned(index->n_nullable));
-	}
+	index->n_def = index->n_fields = old_n_fields;
 
 	const dict_col_t* const new_cols = cols;
 	const dict_col_t* const new_cols_end = cols + n_cols;
@@ -1683,7 +1676,7 @@ dict_table_t::rollback_instant(
 		}
 	}
 
-	index->fields = old_clust_fields;
+	index->fields = old_fields;
 
 	while ((index = dict_table_get_next_index(index)) != NULL) {
 
@@ -1712,81 +1705,6 @@ dict_table_t::rollback_instant(
 	}
 }
 
-/** Trim the instantly added columns when an insert into SYS_COLUMNS
-is rolled back during ALTER TABLE or recovery.
-@param[in]	n	number of surviving non-system columns */
-void dict_table_t::rollback_instant(unsigned n)
-{
-	ut_ad(mutex_own(&dict_sys->mutex));
-	dict_index_t* index = indexes.start;
-	DBUG_ASSERT(index->is_instant());
-	DBUG_ASSERT(index->n_def == index->n_fields);
-	DBUG_ASSERT(n_cols == n_def);
-	DBUG_ASSERT(n >= index->n_uniq);
-	DBUG_ASSERT(n_cols > n + DATA_N_SYS_COLS);
-	const unsigned n_remove = n_cols - n - DATA_N_SYS_COLS;
-
-	char* names = const_cast<char*>(dict_table_get_col_name(this, n));
-	const char* sys = names;
-	for (unsigned i = n_remove; i--; ) {
-		sys += strlen(sys) + 1;
-	}
-	static const char system[] = "DB_ROW_ID\0DB_TRX_ID\0DB_ROLL_PTR";
-	DBUG_ASSERT(!memcmp(sys, system, sizeof system));
-	for (unsigned i = index->n_fields - n_remove; i < index->n_fields;
-	     i++) {
-		if (index->fields[i].col->is_nullable()) {
-			index->n_nullable--;
-		}
-	}
-	index->n_fields -= n_remove;
-	index->n_def = index->n_fields;
-	memmove(names, sys, sizeof system);
-	memmove(cols + n, cols + n_cols - DATA_N_SYS_COLS,
-		DATA_N_SYS_COLS * sizeof *cols);
-	n_cols -= n_remove;
-	n_def = n_cols;
-	n_t_cols -= n_remove;
-	n_t_def -= n_remove;
-
-	for (unsigned i = DATA_N_SYS_COLS; i--; ) {
-		cols[n_cols - i].ind--;
-	}
-
-	if (dict_index_is_auto_gen_clust(index)) {
-		DBUG_ASSERT(index->n_uniq == 1);
-		dict_field_t* field = index->fields;
-		field->name = sys;
-		field->col = dict_table_get_sys_col(this, DATA_ROW_ID);
-		field++;
-		field->name = sys + sizeof "DB_ROW_ID";
-		field->col = dict_table_get_sys_col(this, DATA_TRX_ID);
-		field++;
-		field->name = sys + sizeof "DB_ROW_ID\0DB_TRX_ID";
-		field->col = dict_table_get_sys_col(this, DATA_ROLL_PTR);
-
-		/* Replace the DB_ROW_ID column in secondary indexes. */
-		while ((index = dict_table_get_next_index(index)) != NULL) {
-			field = &index->fields[index->n_fields - 1];
-			DBUG_ASSERT(field->col->mtype == DATA_SYS);
-			DBUG_ASSERT(field->col->prtype
-				    == DATA_NOT_NULL + DATA_TRX_ID);
-			field->col--;
-			field->name = sys;
-		}
-
-		return;
-	}
-
-	dict_field_t* field = &index->fields[index->n_uniq];
-	field->name = sys + sizeof "DB_ROW_ID";
-	field->col = dict_table_get_sys_col(this, DATA_TRX_ID);
-	field++;
-	field->name = sys + sizeof "DB_ROW_ID\0DB_TRX_ID";
-	field->col = dict_table_get_sys_col(this, DATA_ROLL_PTR);
-}
-
-
 /** Check if record in clustered index is historical row.
 @param[in]	rec	clustered row
 @param[in]	offsets	offsets

storage/innobase/handler/ha_innodb.cc

@@ -17213,7 +17213,7 @@ innodb_internal_table_validate(
 
 		DBUG_EXECUTE_IF("innodb_evict_autoinc_table",
 			mutex_enter(&dict_sys->mutex);
-			dict_table_remove_from_cache_low(user_table, TRUE);
+			dict_table_remove_from_cache(user_table, true);
 			mutex_exit(&dict_sys->mutex);
 		);
 	}

storage/innobase/handler/handler0alter.cc

@@ -207,8 +207,10 @@ struct ha_innobase_inplace_ctx : public inplace_alter_handler_ctx
 	const char* const old_col_names;
 	/** original instantly dropped or reordered columns */
 	dict_instant_t*	const	old_instant;
-	/** original clustered index fields */
-	dict_field_t* const	old_clust_fields;
+	/** original index fields */
+	dict_field_t* const	old_fields;
+	/** size of old_fields */
+	const unsigned		old_n_fields;
 	/** 0, or 1 + first column whose position changes in instant ALTER */
 	unsigned	first_alter_pos;
 	/** Allow non-null conversion.
@@ -268,7 +270,8 @@ struct ha_innobase_inplace_ctx : public inplace_alter_handler_ctx
 		old_cols(prebuilt_arg->table->cols),
 		old_col_names(prebuilt_arg->table->col_names),
 		old_instant(prebuilt_arg->table->instant),
-		old_clust_fields(prebuilt_arg->table->indexes.start->fields),
+		old_fields(prebuilt_arg->table->indexes.start->fields),
+		old_n_fields(prebuilt_arg->table->indexes.start->n_fields),
 		first_alter_pos(0),
 		allow_not_null(allow_not_null_flag),
 		page_compression_level(page_compressed
@@ -531,7 +534,8 @@ struct ha_innobase_inplace_ctx : public inplace_alter_handler_ctx
 		if (!is_instant()) return;
 		old_table->rollback_instant(old_n_cols,
 					    old_cols, old_col_names,
-					    old_instant, old_clust_fields,
+					    old_instant,
+					    old_fields, old_n_fields,
 					    col_map);
 	}
 

storage/innobase/include/dict0dict.h

@@ -374,21 +374,12 @@ dict_table_add_system_columns(
 	dict_table_t*	table,	/*!< in/out: table */
 	mem_heap_t*	heap)	/*!< in: temporary heap */
 	MY_ATTRIBUTE((nonnull));
-/**********************************************************************//**
-Removes a table object from the dictionary cache. */
-void
-dict_table_remove_from_cache(
-/*=========================*/
-	dict_table_t*	table)	/*!< in, own: table */
-	MY_ATTRIBUTE((nonnull));
-/**********************************************************************//**
-Removes a table object from the dictionary cache. */
-void
-dict_table_remove_from_cache_low(
-/*=============================*/
-	dict_table_t*	table,		/*!< in, own: table */
-	ibool		lru_evict)	/*!< in: TRUE if table being evicted
-					to make room in the table LRU list */
+/** Evict a table definition from the InnoDB data dictionary cache.
+@param[in,out]	table	cached table definition to be evicted
+@param[in]	lru	whether this is part of least-recently-used evictiono
+@param[in]	keep	whether to keep (not free) the object */
+void dict_table_remove_from_cache(dict_table_t* table, bool lru = false,
+				  bool keep = false)
 	MY_ATTRIBUTE((nonnull));
 /**********************************************************************//**
 Renames a table object.

storage/innobase/include/dict0mem.h

@@ -1622,21 +1622,18 @@ struct dict_table_t {
 	@param[in]	old_cols		original cols
 	@param[in]	old_col_names		original col_names
 	@param[in]	old_instant		original instant structure
-	@param[in]	old_clust_fields	original clustered fields
+	@param[in]	old_fields		original fields
+	@param[in]	old_n_fields		original number of fields
 	@param[in]	col_map			column map */
 	void rollback_instant(
 		unsigned	old_n_cols,
 		dict_col_t*	old_cols,
 		const char*	old_col_names,
 		dict_instant_t*	old_instant,
-		dict_field_t*	old_clust_fields,
+		dict_field_t*	old_fields,
+		unsigned	old_n_fields,
 		const ulint*	col_map);
 
-	/** Trim the instantly added columns when an insert into SYS_COLUMNS
-	is rolled back during ALTER TABLE or recovery.
-	@param[in]	n	number of surviving non-system columns */
-	void rollback_instant(unsigned n);
-
 	/** Add the table definition to the data dictionary cache */
 	void add_to_cache();
 

storage/innobase/include/trx0trx.h

@@ -482,6 +482,7 @@ Check transaction state */
 	ut_ad(!(t)->read_view.is_open());				\
 	ut_ad((t)->lock.wait_thr == NULL);				\
 	ut_ad(UT_LIST_GET_LEN((t)->lock.trx_locks) == 0);		\
+	ut_ad(UT_LIST_GET_LEN((t)->lock.evicted_tables) == 0);		\
 	ut_ad((t)->dict_operation == TRX_DICT_OP_NONE);			\
 } while(0)
 
@@ -606,6 +607,9 @@ struct trx_lock_t {
 	lock_list	table_locks;	/*!< All table locks requested by this
 					transaction, including AUTOINC locks */
 
+	/** List of pending trx_t::evict_table() */
+	UT_LIST_BASE_NODE_T(dict_table_t) evicted_tables;
+
 	bool		cancel;		/*!< true if the transaction is being
 					rolled back either via deadlock
 					detection or due to lock timeout. The
@@ -1112,6 +1116,10 @@ struct trx_t {
 		return(assign_temp_rseg());
 	}
 
+	/** Evict a table definition due to the rollback of ALTER TABLE.
+	@param[in]	table_id	table identifier */
+	void evict_table(table_id_t table_id);
+
 
   bool is_referenced()
   {

storage/innobase/row/row0uins.cc

@@ -140,10 +140,11 @@ row_undo_ins_remove_clust_rec(
 		break;
 	case DICT_COLUMNS_ID:
 		/* This is rolling back an INSERT into SYS_COLUMNS.
-		If it was part of an instant ADD COLUMN operation, we
-		must modify the table definition. At this point, any
-		corresponding operation to the metadata record will have
-		been rolled back. */
+		If it was part of an instant ALTER TABLE operation, we
+		must evict the table definition, so that it can be
+		reloaded after the dictionary operation has been
+		completed. At this point, any corresponding operation
+		to the metadata record will have been rolled back. */
 		ut_ad(!online);
 		ut_ad(node->trx->dict_operation_lock_mode == RW_X_LATCH);
 		ut_ad(node->rec_type == TRX_UNDO_INSERT_REC);
@@ -158,33 +159,7 @@ row_undo_ins_remove_clust_rec(
 		if (len != 8) {
 			break;
 		}
-		const table_id_t table_id = mach_read_from_8(data);
-		data = rec_get_nth_field_old(rec, DICT_FLD__SYS_COLUMNS__POS,
-					     &len);
-		if (len != 4) {
-			break;
-		}
-		const unsigned pos = mach_read_from_4(data);
-		if (pos == 0 || pos >= (1U << 16)) {
-			break;
-		}
-		dict_table_t* table = dict_table_open_on_id(
-			table_id, true, DICT_TABLE_OP_OPEN_ONLY_IF_CACHED);
-		if (!table) {
-			break;
-		}
-
-		dict_index_t* index = dict_table_get_first_index(table);
-
-		if (index && index->is_instant()
-		    && DATA_N_SYS_COLS + 1 + pos == table->n_cols) {
-			/* This is the rollback of an instant ADD COLUMN.
-			Remove the column from the dictionary cache,
-			but keep the system columns. */
-			table->rollback_instant(pos);
-		}
-
-		dict_table_close(table, true, false);
+		node->trx->evict_table(mach_read_from_8(data));
 	}
 
 	if (btr_cur_optimistic_delete(btr_cur, 0, &mtr)) {

storage/innobase/trx/trx0trx.cc

@@ -214,6 +214,9 @@ struct TrxFactory {
 
 		lock_trx_lock_list_init(&trx->lock.trx_locks);
 
+		UT_LIST_INIT(trx->lock.evicted_tables,
+			     &dict_table_t::table_LRU);
+
 		UT_LIST_INIT(
 			trx->trx_savepoints,
 			&trx_named_savept_t::trx_savepoints);
@@ -238,6 +241,7 @@ struct TrxFactory {
 		}
 
 		ut_a(UT_LIST_GET_LEN(trx->lock.trx_locks) == 0);
+		ut_ad(UT_LIST_GET_LEN(trx->lock.evicted_tables) == 0);
 
 		UT_DELETE(trx->xid);
 		ut_free(trx->detailed_error);
@@ -390,6 +394,7 @@ trx_t *trx_create()
 	ut_ad(trx->lock.n_rec_locks == 0);
 	ut_ad(trx->lock.table_cached == 0);
 	ut_ad(trx->lock.rec_cached == 0);
+	ut_ad(UT_LIST_GET_LEN(trx->lock.evicted_tables) == 0);
 
 #ifdef WITH_WSREP
 	trx->wsrep_event = NULL;
@@ -1250,6 +1255,37 @@ trx_update_mod_tables_timestamp(
 	trx->mod_tables.clear();
 }
 
+/** Evict a table definition due to the rollback of ALTER TABLE.
+@param[in]	table_id	table identifier */
+void trx_t::evict_table(table_id_t table_id)
+{
+	ut_ad(in_rollback);
+
+	dict_table_t* table = dict_table_open_on_id(
+		table_id, true, DICT_TABLE_OP_OPEN_ONLY_IF_CACHED);
+	if (!table) {
+		return;
+	}
+
+	if (!table->release()) {
+		/* This must be a DDL operation that is being rolled
+		back in an active connection. */
+		ut_a(table->get_ref_count() == 1);
+		ut_ad(!is_recovered);
+		ut_ad(mysql_thd);
+		return;
+	}
+
+	/* This table should only be locked by this transaction, if at all. */
+	ut_ad(UT_LIST_GET_LEN(table->locks) <= 1);
+	const bool locked = UT_LIST_GET_LEN(table->locks);
+	ut_ad(!locked || UT_LIST_GET_FIRST(table->locks)->trx == this);
+	dict_table_remove_from_cache(table, true, locked);
+	if (locked) {
+		UT_LIST_ADD_FIRST(lock.evicted_tables, table);
+	}
+}
+
 /****************************************************************//**
 Commits a transaction in memory. */
 static
@@ -1315,9 +1351,16 @@ trx_commit_in_memory(
 			trx_update_mod_tables_timestamp(trx);
 			MONITOR_INC(MONITOR_TRX_RW_COMMIT);
 		}
+
+		while (dict_table_t* table = UT_LIST_GET_FIRST(
+			       trx->lock.evicted_tables)) {
+			UT_LIST_REMOVE(trx->lock.evicted_tables, table);
+			dict_mem_table_free(table);
+		}
 	}
 
 	ut_ad(!trx->rsegs.m_redo.undo);
+	ut_ad(UT_LIST_GET_LEN(trx->lock.evicted_tables) == 0);
 
 	if (trx_rseg_t*	rseg = trx->rsegs.m_redo.rseg) {
 		mutex_enter(&rseg->mutex);