MDEV-21175 follow-up: Remove redundant locking; rely on MDL

Before entering DML or DDL execution in the storage engine, the SQL layer
will have acquired metadata lock (MDL) on the current table name as well
as the names of FOREIGN KEY (grand)child tables (that is,
tables whose REFERENCES clauses point to the current table).
The MDL prevents any metadata changes to these tables, such as
RENAME, TRUNCATE, DROP, ALTER.

While the MDL on the current table prevents dict_table_t::foreign_set
from being modified, it does not prevent the table metadata that the
stored pointers are pointing to from being modified.

The MDL on the child tables will prevent both dict_table_t::referenced_set
as well as the pointed child table metadata from being modified.

wsrep_row_upd_index_is_foreign(): Do not unnecessarily acquire the
data dictionary latch if Galera replication is not enabled.

ha_innobase::can_switch_engines(): Rely on MDL. We are not dereferencing
any pointers stored in the sets.

row_mysql_freeze_data_dictionary(), row_mysql_unfreeze_data_dictionary():
Remove.

row_update_for_mysql(): Call init_fts_doc_id_for_ref() only once.

In ALTER TABLE...IMPORT TABLESPACE and FLUSH TABLES...FOR EXPORT
the SQL layer is protecting the current table with MDL. We do not
need InnoDB latches.

storage/innobase/handler/ha_innodb.cc

@@ -15289,32 +15289,13 @@ struct table_list_item {
 	const char*		name;
 };
 
-/*****************************************************************//**
-Checks if ALTER TABLE may change the storage engine of the table.
-Changing storage engines is not allowed for tables for which there
-are foreign key constraints (parent or child tables).
-@return TRUE if can switch engines */
-
-bool
-ha_innobase::can_switch_engines(void)
-/*=================================*/
+/** @return whether ALTER TABLE may change the storage engine of the table */
+bool ha_innobase::can_switch_engines()
 {
-	DBUG_ENTER("ha_innobase::can_switch_engines");
-
-	update_thd();
-
-	m_prebuilt->trx->op_info =
-			"determining if there are foreign key constraints";
-
-	row_mysql_freeze_data_dictionary(m_prebuilt->trx);
-
-	bool	can_switch = m_prebuilt->table->referenced_set.empty()
-		&& m_prebuilt->table->foreign_set.empty();
-
-	row_mysql_unfreeze_data_dictionary(m_prebuilt->trx);
-	m_prebuilt->trx->op_info = "";
-
-	DBUG_RETURN(can_switch);
+  DBUG_ENTER("ha_innobase::can_switch_engines");
+  update_thd();
+  return m_prebuilt->table->foreign_set.empty() &&
+    m_prebuilt->table->referenced_set.empty();
 }
 
 /*******************************************************************//**

storage/innobase/include/dict0mem.h

@@ -1524,24 +1524,6 @@ struct dict_foreign_with_index {
 	const dict_index_t*	m_index;
 };
 
-#ifdef WITH_WSREP
-/** A function object to find a foreign key with the given index as the
-foreign index. Return the foreign key with matching criteria or NULL */
-struct dict_foreign_with_foreign_index {
-
-	dict_foreign_with_foreign_index(const dict_index_t*	index)
-	: m_index(index)
-	{}
-
-	bool operator()(const dict_foreign_t*	foreign) const
-	{
-		return(foreign->foreign_index == m_index);
-	}
-
-	const dict_index_t*	m_index;
-};
-#endif
-
 /* A function object to check if the foreign constraint is between different
 tables.  Returns true if foreign key constraint is between different tables,
 false otherwise. */

storage/innobase/include/row0mysql.h

@@ -330,17 +330,6 @@ row_mysql_unlock_data_dictionary(
 /*=============================*/
 	trx_t*	trx);	/*!< in/out: transaction */
 /*********************************************************************//**
-Locks the data dictionary in shared mode from modifications, for performing
-foreign key check, rollback, or other operation invisible to MySQL. */
-void row_mysql_freeze_data_dictionary(trx_t *trx);
-
-/*********************************************************************//**
-Unlocks the data dictionary shared lock. */
-void
-row_mysql_unfreeze_data_dictionary(
-/*===============================*/
-	trx_t*	trx);	/*!< in/out: transaction */
-/*********************************************************************//**
 Creates a table for MySQL. On failure the transaction will be rolled back
 and the 'table' object will be freed.
 @return error code or DB_SUCCESS */

storage/innobase/lock/lock0lock.cc

@@ -1703,8 +1703,7 @@ dberr_t lock_wait(que_thr_t *thr)
   const ulong innodb_lock_wait_timeout= trx_lock_wait_timeout_get(trx);
   const bool no_timeout= innodb_lock_wait_timeout >= 100000000;
   const my_hrtime_t suspend_time= my_hrtime_coarse();
-  ut_ad(!trx->dict_operation_lock_mode ||
-        trx->dict_operation_lock_mode == RW_S_LATCH);
+  ut_ad(!trx->dict_operation_lock_mode);
 
   /* The wait_lock can be cleared by another thread in lock_grant(),
   lock_rec_cancel(), or lock_cancel_waiting_and_release(). But, a wait
@@ -1739,9 +1738,7 @@ dberr_t lock_wait(que_thr_t *thr)
 
   trx->lock.suspend_time= suspend_time;
 
-  const auto had_dict_lock= trx->dict_operation_lock_mode;
-  if (had_dict_lock) /* Release foreign key check latch */
-    row_mysql_unfreeze_data_dictionary(trx);
+  ut_ad(!trx->dict_operation_lock_mode);
 
   IF_WSREP(if (trx->is_wsrep()) lock_wait_wsrep(trx),);
 
@@ -1837,9 +1834,6 @@ dberr_t lock_wait(que_thr_t *thr)
   mysql_mutex_unlock(&lock_sys.wait_mutex);
   thd_wait_end(trx->mysql_thd);
 
-  if (had_dict_lock)
-    row_mysql_freeze_data_dictionary(trx);
-
   trx->error_state= error_state;
   return error_state;
 }

storage/innobase/row/row0import.cc

@@ -3960,10 +3960,6 @@ row_import_for_mysql(
 
 	prebuilt->trx->op_info = "read meta-data file";
 
-	/* Prevent DDL operations while we are checking. */
-
-	dict_sys.freeze();
-
 	row_import	cfg;
 
 	err = row_import_read_cfg(table, trx->mysql_thd, cfg);
@@ -3987,15 +3983,10 @@ row_import_for_mysql(
 			autoinc = cfg.m_autoinc;
 		}
 
-		dict_sys.unfreeze();
-
 		DBUG_EXECUTE_IF("ib_import_set_index_root_failure",
 				err = DB_TOO_MANY_CONCURRENT_TRXS;);
 
 	} else if (cfg.m_missing) {
-
-		dict_sys.unfreeze();
-
 		/* We don't have a schema file, we will have to discover
 		the index root pages from the .ibd file and skip the schema
 		matching step. */
@@ -4022,8 +4013,6 @@ row_import_for_mysql(
 				err = cfg.set_root_by_heuristic();
 			}
 		}
-	} else {
-		dict_sys.unfreeze();
 	}
 
 	if (err != DB_SUCCESS) {

storage/innobase/row/row0ins.cc

@@ -1342,16 +1342,6 @@ row_ins_foreign_check_on_constraint(
 	err = row_update_cascade_for_mysql(thr, cascade,
 					   foreign->foreign_table);
 
-	/* Release the data dictionary latch for a while, so that we do not
-	starve other threads from doing CREATE TABLE etc. if we have a huge
-	cascaded operation running. */
-
-	row_mysql_unfreeze_data_dictionary(thr_get_trx(thr));
-
-	DEBUG_SYNC_C("innodb_dml_cascade_dict_unfreeze");
-
-	row_mysql_freeze_data_dictionary(thr_get_trx(thr));
-
 	mtr_start(mtr);
 
 	/* Restore pcur position */

storage/innobase/row/row0mysql.cc

@@ -1626,33 +1626,24 @@ init_fts_doc_id_for_ref(
 	dict_table_t*	table,		/*!< in: table */
 	ulint*		depth)		/*!< in: recusive call depth */
 {
-	dict_foreign_t* foreign;
-
 	table->fk_max_recusive_level = 0;
 
-	(*depth)++;
-
 	/* Limit on tables involved in cascading delete/update */
-	if (*depth > FK_MAX_CASCADE_DEL) {
+	if (++*depth > FK_MAX_CASCADE_DEL) {
 		return;
 	}
 
 	/* Loop through this table's referenced list and also
 	recursively traverse each table's foreign table list */
-	for (dict_foreign_set::iterator it = table->referenced_set.begin();
-	     it != table->referenced_set.end();
-	     ++it) {
-
-		foreign = *it;
+	for (dict_foreign_t* foreign : table->referenced_set) {
+		ut_ad(foreign->foreign_table);
 
-		ut_ad(foreign->foreign_table != NULL);
-
-		if (foreign->foreign_table->fts != NULL) {
+		if (foreign->foreign_table->fts) {
 			fts_init_doc_id(foreign->foreign_table);
 		}
 
-		if (!foreign->foreign_table->referenced_set.empty()
-		    && foreign->foreign_table != table) {
+		if (foreign->foreign_table != table
+		    && !foreign->foreign_table->referenced_set.empty()) {
 			init_fts_doc_id_for_ref(
 				foreign->foreign_table, depth);
 		}
@@ -1673,7 +1664,6 @@ row_update_for_mysql(row_prebuilt_t* prebuilt)
 	dict_table_t*	table		= prebuilt->table;
 	trx_t*		trx		= prebuilt->trx;
 	ulint		fk_depth	= 0;
-	bool		got_s_lock	= false;
 
 	DBUG_ENTER("row_update_for_mysql");
 
@@ -1703,18 +1693,6 @@ row_update_for_mysql(row_prebuilt_t* prebuilt)
 		trx_start_if_not_started_xa(trx, true);
 	}
 
-	if (dict_table_is_referenced_by_foreign_key(table)) {
-		/* Share lock the data dictionary to prevent any
-		table dictionary (for foreign constraint) change.
-		This is similar to row_ins_check_foreign_constraint
-		check protect by the dictionary lock as well.
-		In the future, this can be removed once the Foreign
-		key MDL is implemented */
-		row_mysql_freeze_data_dictionary(trx);
-		init_fts_doc_id_for_ref(table, &fk_depth);
-		row_mysql_unfreeze_data_dictionary(trx);
-	}
-
 	node = prebuilt->upd_node;
 	const bool is_delete = node->is_delete == PLAIN_DELETE;
 	ut_ad(node->table == table);
@@ -1795,10 +1773,6 @@ row_update_for_mysql(row_prebuilt_t* prebuilt)
 	}
 
 	/* Completed cascading operations (if any) */
-	if (got_s_lock) {
-		row_mysql_unfreeze_data_dictionary(trx);
-	}
-
 	bool	update_statistics;
 	ut_ad(is_delete == (node->is_delete == PLAIN_DELETE));
 
@@ -1834,16 +1808,8 @@ row_update_for_mysql(row_prebuilt_t* prebuilt)
 		prebuilt->table->stat_modified_counter++;
 	}
 
-	trx->op_info = "";
-
-	DBUG_RETURN(err);
-
 error:
 	trx->op_info = "";
-	if (got_s_lock) {
-		row_mysql_unfreeze_data_dictionary(trx);
-	}
-
 	DBUG_RETURN(err);
 }
 
@@ -1970,29 +1936,6 @@ row_unlock_for_mysql(
 	trx->op_info = "";
 }
 
-/*********************************************************************//**
-Locks the data dictionary in shared mode from modifications, for performing
-foreign key check, rollback, or other operation invisible to MySQL. */
-void row_mysql_freeze_data_dictionary(trx_t *trx)
-{
-  ut_a(trx->dict_operation_lock_mode == 0);
-  trx->dict_operation_lock_mode = RW_S_LATCH;
-  dict_sys.freeze();
-}
-
-/*********************************************************************//**
-Unlocks the data dictionary shared lock. */
-void
-row_mysql_unfreeze_data_dictionary(
-/*===============================*/
-	trx_t*	trx)	/*!< in/out: transaction */
-{
-  ut_ad(!lock_trx_has_sys_table_locks(trx));
-  ut_ad(trx->dict_operation_lock_mode == RW_S_LATCH);
-  dict_sys.unfreeze();
-  trx->dict_operation_lock_mode = 0;
-}
-
 /** Write query start time as SQL field data to a buffer. Needed by InnoDB.
 @param	thd	Thread object
 @param	buf	Buffer to hold start time data */

storage/innobase/row/row0upd.cc

@@ -122,10 +122,6 @@ row_upd_changes_first_fields_binary(
 Checks if index currently is mentioned as a referenced index in a foreign
 key constraint.
 
-NOTE that since we do not hold dict_sys.latch when leaving the
-function, it may be that the referencing table has been dropped when
-we leave this function: this function is only for heuristic use!
-
 @return true if referenced */
 static
 bool
@@ -134,64 +130,44 @@ row_upd_index_is_referenced(
 	dict_index_t*	index,	/*!< in: index */
 	trx_t*		trx)	/*!< in: transaction */
 {
-	dict_table_t*	table		= index->table;
-
-	if (table->referenced_set.empty()) {
-		return false;
-	}
-
-	const bool froze_data_dict = !trx->dict_operation_lock_mode;
-	if (froze_data_dict) {
-		row_mysql_freeze_data_dictionary(trx);
-	}
-
-	dict_foreign_set::iterator	it
-		= std::find_if(table->referenced_set.begin(),
-			       table->referenced_set.end(),
-			       dict_foreign_with_index(index));
-
-	const bool is_referenced = (it != table->referenced_set.end());
-
-	if (froze_data_dict) {
-		row_mysql_unfreeze_data_dictionary(trx);
-	}
-
-	return is_referenced;
+  dict_table_t *table= index->table;
+  /* The pointers in table->referenced_set are safe to dereference
+  thanks to the SQL layer having acquired MDL on all (grand)parent tables. */
+  dict_foreign_set::iterator end= table->referenced_set.end();
+  return end != std::find_if(table->referenced_set.begin(), end,
+                             dict_foreign_with_index(index));
 }
 
 #ifdef WITH_WSREP
 static
-ibool
+bool
 wsrep_row_upd_index_is_foreign(
 /*========================*/
 	dict_index_t*	index,	/*!< in: index */
 	trx_t*		trx)	/*!< in: transaction */
 {
-	dict_table_t*	table		= index->table;
-	ibool		froze_data_dict	= FALSE;
-	ibool		is_referenced	= FALSE;
+  if (!trx->is_wsrep())
+    return false;
 
-	if (table->foreign_set.empty()) {
-		return(FALSE);
-	}
-
-	if (trx->dict_operation_lock_mode == 0) {
-		row_mysql_freeze_data_dictionary(trx);
-		froze_data_dict = TRUE;
-	}
+  dict_table_t *table= index->table;
 
-	dict_foreign_set::iterator	it
-		= std::find_if(table->foreign_set.begin(),
-			       table->foreign_set.end(),
-			       dict_foreign_with_foreign_index(index));
+  if (table->foreign_set.empty())
+    return false;
 
-	is_referenced = (it != table->foreign_set.end());
+  /* No MDL protects dereferencing the members of table->foreign_set. */
+  const bool no_lock= !trx->dict_operation_lock_mode;
+  if (no_lock)
+    dict_sys.freeze();
 
-	if (froze_data_dict) {
-		row_mysql_unfreeze_data_dictionary(trx);
-	}
+  auto end= table->foreign_set.end();
+  const bool is_referenced= end !=
+    std::find_if(table->foreign_set.begin(), end,
+                 [index](const dict_foreign_t* f)
+                 {return f->foreign_index == index;});
+  if (no_lock)
+    dict_sys.unfreeze();
 
-	return(is_referenced);
+  return is_referenced;
 }
 #endif /* WITH_WSREP */
 
@@ -219,10 +195,8 @@ row_upd_check_references_constraints(
 	dict_foreign_t*	foreign;
 	mem_heap_t*	heap;
 	dtuple_t*	entry;
-	trx_t*		trx;
 	const rec_t*	rec;
 	dberr_t		err;
-	ibool		got_s_lock	= FALSE;
 
 	DBUG_ENTER("row_upd_check_references_constraints");
 
@@ -230,8 +204,6 @@ row_upd_check_references_constraints(
 		DBUG_RETURN(DB_SUCCESS);
 	}
 
-	trx = thr_get_trx(thr);
-
 	rec = btr_pcur_get_rec(pcur);
 	ut_ad(rec_offs_validate(rec, index, offsets));
 
@@ -245,12 +217,6 @@ row_upd_check_references_constraints(
 
 	mtr->start();
 
-	if (trx->dict_operation_lock_mode == 0) {
-		got_s_lock = TRUE;
-
-		row_mysql_freeze_data_dictionary(trx);
-	}
-
 	DEBUG_SYNC_C_IF_THD(thr_get_trx(thr)->mysql_thd,
 			    "foreign_constraint_check_for_insert");
 
@@ -297,10 +263,6 @@ row_upd_check_references_constraints(
 	err = DB_SUCCESS;
 
 func_exit:
-	if (got_s_lock) {
-		row_mysql_unfreeze_data_dictionary(trx);
-	}
-
 	mem_heap_free(heap);
 
 	DEBUG_SYNC_C("foreign_constraint_check_for_update_done");
@@ -324,18 +286,14 @@ wsrep_row_upd_check_foreign_constraints(
 	dict_foreign_t*	foreign;
 	mem_heap_t*	heap;
 	dtuple_t*	entry;
-	trx_t*		trx;
 	const rec_t*	rec;
 	dberr_t		err;
-	ibool		got_s_lock	= FALSE;
 	ibool		opened     	= FALSE;
 
 	if (table->foreign_set.empty()) {
 		return(DB_SUCCESS);
 	}
 
-	trx = thr_get_trx(thr);
-
 	/* TODO: make native slave thread bail out here */
 
 	rec = btr_pcur_get_rec(pcur);
@@ -349,12 +307,6 @@ wsrep_row_upd_check_foreign_constraints(
 
 	mtr_start(mtr);
 
-	if (trx->dict_operation_lock_mode == 0) {
-		got_s_lock = TRUE;
-
-		row_mysql_freeze_data_dictionary(trx);
-	}
-
 	for (dict_foreign_set::iterator it = table->foreign_set.begin();
 	     it != table->foreign_set.end();
 	     ++it) {
@@ -402,10 +354,6 @@ wsrep_row_upd_check_foreign_constraints(
 
 	err = DB_SUCCESS;
 func_exit:
-	if (got_s_lock) {
-		row_mysql_unfreeze_data_dictionary(trx);
-	}
-
 	mem_heap_free(heap);
 
 	return(err);
@@ -1957,7 +1905,7 @@ row_upd_sec_index_entry(
 
 	const bool referenced = row_upd_index_is_referenced(index, trx);
 #ifdef WITH_WSREP
-	bool foreign = wsrep_row_upd_index_is_foreign(index, trx);
+	const bool foreign = wsrep_row_upd_index_is_foreign(index, trx);
 #endif /* WITH_WSREP */
 
 	heap = mem_heap_create(1024);