MDEV-25919: Replace dict_table_t::stats_bg_flag with MDL

The purpose of dict_table_t::stats_bg_flag was to prevent
race conditions between DDL operations and a background thread
that updates persistent statistics for InnoDB tables.

Now that with the parent commit, we started to acquire a
shared meta-data lock (MDL) on the InnoDB persistent statistics tables
in background tasks that access them, we may easily acquire MDL
on the table for which the statistics are being updated. This will by
design prevent race conditions with any DDL operations on that table,
and the stats_bg_flag may be removed.

dict_stats_process_entry_from_recalc_pool(): Complete rewrite.
During the processing, retain the entry in recalc_pool, so
that dict_stats_recalc_pool_del() will be able to request
deletion of the entry, or delete the entry if its caller is
holding MDL_EXCLUSIVE while we are waiting for MDL.

recalc_pool: In addition to the table ID, store a state for
inter-thread communication, so that dict_stats_recalc_pool_del()
can wait until all processing is finished.

Reviewed by: Thirunarayanan Balathandayuthapani

storage/innobase/dict/dict0stats.cc

@@ -839,9 +839,6 @@ dict_stats_assert_initialized(
 	MEM_CHECK_DEFINED(&table->stat_modified_counter,
 			  sizeof table->stat_modified_counter);
 
-	MEM_CHECK_DEFINED(&table->stats_bg_flag,
-			  sizeof table->stats_bg_flag);
-
 	for (dict_index_t* index = dict_table_get_first_index(table);
 	     index != NULL;
 	     index = dict_table_get_next_index(index)) {
@@ -1009,7 +1006,6 @@ dict_stats_snapshot_create(
 	t->stat_persistent = table->stat_persistent;
 	t->stats_auto_recalc = table->stats_auto_recalc;
 	t->stats_sample_pages = table->stats_sample_pages;
-	t->stats_bg_flag = table->stats_bg_flag;
 
 	dict_sys.unlock();
 
@@ -2496,21 +2492,20 @@ dict_stats_update_persistent(
 			continue;
 		}
 
-		if (!(table->stats_bg_flag & BG_STAT_SHOULD_QUIT)) {
-			table->stats_mutex_unlock();
-			stats = dict_stats_analyze_index(index);
-			table->stats_mutex_lock();
+		table->stats_mutex_unlock();
+		stats = dict_stats_analyze_index(index);
+		table->stats_mutex_lock();
 
-			index->stat_index_size = stats.index_size;
-			index->stat_n_leaf_pages = stats.n_leaf_pages;
-			for (size_t i = 0; i < stats.stats.size(); ++i) {
-				index->stat_n_diff_key_vals[i]
-					= stats.stats[i].n_diff_key_vals;
-				index->stat_n_sample_sizes[i]
-					= stats.stats[i].n_sample_sizes;
-				index->stat_n_non_null_key_vals[i]
-					= stats.stats[i].n_non_null_key_vals;
-			}
+		index->stat_index_size = stats.index_size;
+		index->stat_n_leaf_pages = stats.n_leaf_pages;
+
+		for (size_t i = 0; i < stats.stats.size(); ++i) {
+			index->stat_n_diff_key_vals[i]
+				= stats.stats[i].n_diff_key_vals;
+			index->stat_n_sample_sizes[i]
+				= stats.stats[i].n_sample_sizes;
+			index->stat_n_non_null_key_vals[i]
+				= stats.stats[i].n_non_null_key_vals;
 		}
 
 		table->stat_sum_of_other_index_sizes

storage/innobase/dict/drop.cc

@@ -59,7 +59,6 @@ that would cause the InnoDB to hang or to intentionally crash.
 (d) The only changes to the data dictionary cache that are performed
 before transaction commit and must be rolled back explicitly are as follows:
 (d1) fts_optimize_add_table() to undo fts_optimize_remove_table()
-(d2) stats_bg_flag= BG_STAT_NONE to undo dict_stats_stop_bg()
 */
 
 #include "trx0purge.h"
@@ -148,7 +147,6 @@ dberr_t trx_t::drop_table(const dict_table_t &table)
   ut_ad(dict_operation);
   ut_ad(dict_operation_lock_mode);
   ut_ad(!table.is_temporary());
-  ut_ad(!(table.stats_bg_flag & BG_STAT_IN_PROGRESS));
   /* The table must be exclusively locked by this transaction. */
   ut_ad(table.get_ref_count() <= 1);
   ut_ad(table.n_lock_x_or_s == 1);
@@ -241,7 +239,7 @@ void trx_t::commit(std::vector<pfs_os_file_t> &deleted)
       if (p.second.is_dropped())
       {
         dict_table_t *table= p.first;
-        dict_stats_recalc_pool_del(table);
+        dict_stats_recalc_pool_del(table->id, true);
         dict_stats_defrag_pool_del(table, nullptr);
         if (btr_defragment_active)
           btr_defragment_remove_table(table);

storage/innobase/handler/ha_innodb.cc

@@ -1339,27 +1339,10 @@ static void innodb_drop_database(handlerton*, char *path)
 
   THD * const thd= current_thd;
   trx_t *trx= innobase_trx_allocate(thd);
-retry:
-  dict_sys.lock(SRW_LOCK_CALL);
-
-  for (auto i= dict_sys.table_id_hash.n_cells; i--; )
-  {
-    for (dict_table_t *table= static_cast<dict_table_t*>
-         (dict_sys.table_id_hash.array[i].node); table; table= table->id_hash)
-    {
-      ut_ad(table->cached);
-      if (!strncmp(table->name.m_name, namebuf, len) &&
-          !dict_stats_stop_bg(table))
-      {
-        dict_sys.unlock();
-        std::this_thread::sleep_for(std::chrono::milliseconds(250));
-        goto retry;
-      }
-    }
-  }
-
   dberr_t err= DB_SUCCESS;
 
+  dict_sys.lock(SRW_LOCK_CALL);
+
   for (auto i= dict_sys.table_id_hash.n_cells; i--; )
   {
     for (dict_table_t *next, *table= static_cast<dict_table_t*>
@@ -2069,7 +2052,6 @@ static void drop_garbage_tables_after_restore()
         ({reinterpret_cast<const char*>(pcur.old_rec), len},
          DICT_ERR_IGNORE_DROP))
     {
-      ut_ad(table->stats_bg_flag == BG_STAT_NONE);
       table->acquire();
       row_mysql_unlock_data_dictionary(trx);
       err= lock_table_for_trx(table, trx, LOCK_X);
@@ -13399,7 +13381,6 @@ int ha_innobase::delete_table(const char *name)
   trx_t *parent_trx= check_trx_exists(thd);
   dict_table_t *table;
 
-  for (;;)
   {
     char norm_name[FN_REFLEN];
     normalize_table_name(norm_name, name);
@@ -13420,10 +13401,6 @@ int ha_innobase::delete_table(const char *name)
       dict_sys.unlock();
       DBUG_RETURN(HA_ERR_NO_SUCH_TABLE);
     }
-
-    if (dict_stats_stop_bg(table))
-      break;
-    dict_sys.unlock();
   }
 
   if (table->is_temporary())
@@ -13568,7 +13545,6 @@ int ha_innobase::delete_table(const char *name)
     default:
       ib::error() << "DROP TABLE " << table->name << ": " << err;
     }
-    table->stats_bg_flag= BG_STAT_NONE;
     if (fts)
     {
       fts_optimize_add_table(table);
@@ -13862,7 +13838,6 @@ int ha_innobase::truncate()
 	}
 
 	row_mysql_lock_data_dictionary(trx);
-	dict_stats_wait_bg_to_stop_using_table(ib_table);
 
 	if (error == DB_SUCCESS) {
 		error = innobase_rename_table(trx, ib_table->name.m_name,
@@ -14569,13 +14544,11 @@ ha_innobase::info_low(
 			m_prebuilt->trx->op_info = "updating table statistics";
 
 			if (dict_stats_is_persistent_enabled(ib_table)) {
-
 				if (is_analyze) {
-					dict_sys.lock(SRW_LOCK_CALL);
-					dict_stats_recalc_pool_del(ib_table);
-					dict_stats_wait_bg_to_stop_using_table(
-						ib_table);
-					dict_sys.unlock();
+					if (!srv_read_only_mode) {
+						dict_stats_recalc_pool_del(
+							ib_table->id, false);
+					}
 					opt = DICT_STATS_RECALC_PERSISTENT;
 				} else {
 					/* This is e.g. 'SHOW INDEXES', fetch
@@ -14588,15 +14561,6 @@ ha_innobase::info_low(
 
 			ret = dict_stats_update(ib_table, opt);
 
-			if (opt == DICT_STATS_RECALC_PERSISTENT) {
-				dict_sys.freeze(SRW_LOCK_CALL);
-				ib_table->stats_mutex_lock();
-				ib_table->stats_bg_flag
-					&= byte(~BG_STAT_SHOULD_QUIT);
-				ib_table->stats_mutex_unlock();
-				dict_sys.unfreeze();
-			}
-
 			if (ret != DB_SUCCESS) {
 				m_prebuilt->trx->op_info = "";
 				DBUG_RETURN(HA_ERR_GENERIC);

storage/innobase/handler/handler0alter.cc

@@ -6212,7 +6212,6 @@ prepare_inplace_alter_table_dict(
 	create_table_info_t info(ctx->prebuilt->trx->mysql_thd, altered_table,
 				 ha_alter_info->create_info, NULL, NULL,
 				 srv_file_per_table);
-	ut_d(bool stats_wait = false);
 
 	/* The primary index would be rebuilt if a FTS Doc ID
 	column is to be added, and the primary index definition
@@ -6280,16 +6279,6 @@ prepare_inplace_alter_table_dict(
 
 	row_mysql_lock_data_dictionary(ctx->trx);
 	dict_locked = true;
-
-	/* Wait for background stats processing to stop using the table that
-	we are going to alter. We know bg stats will not start using it again
-	until we are holding the data dict locked and we are holding it here
-	at least until checking ut_ad(user_table->n_ref_count == 1) below.
-	XXX what may happen if bg stats opens the table after we
-	have unlocked data dictionary below? */
-	dict_stats_wait_bg_to_stop_using_table(user_table);
-	ut_d(stats_wait = true);
-
 	online_retry_drop_indexes_low(ctx->new_table, ctx->trx);
 
 	ut_d(dict_table_check_for_dup_indexes(
@@ -7175,10 +7164,10 @@ prepare_inplace_alter_table_dict(
 		}
 	}
 
-	/* n_ref_count must be 1, because purge cannot
+	/* n_ref_count must be 1, because background threads cannot
 	be executing on this very table as we are
 	holding MDL_EXCLUSIVE. */
-	ut_ad(!stats_wait || ctx->online || user_table->get_ref_count() == 1);
+	ut_ad(ctx->online || user_table->get_ref_count() == 1);
 
 	if (new_clustered) {
 		online_retry_drop_indexes_low(user_table, ctx->trx);
@@ -10973,35 +10962,6 @@ ha_innobase::commit_inplace_alter_table(
 
 	row_mysql_lock_data_dictionary(trx);
 
-	/* Prevent the background statistics collection from accessing
-	the tables. */
-	for (;;) {
-		bool	retry = false;
-
-		for (inplace_alter_handler_ctx** pctx = ctx_array;
-		     *pctx; pctx++) {
-			ha_innobase_inplace_ctx*	ctx
-				= static_cast<ha_innobase_inplace_ctx*>(*pctx);
-
-			DBUG_ASSERT(new_clustered == ctx->need_rebuild());
-
-			if (new_clustered
-			    && !dict_stats_stop_bg(ctx->old_table)) {
-				retry = true;
-			}
-
-			if (!dict_stats_stop_bg(ctx->new_table)) {
-				retry = true;
-			}
-		}
-
-		if (!retry) {
-			break;
-		}
-
-		DICT_BG_YIELD;
-	}
-
 	/* Apply the changes to the data dictionary tables, for all
 	partitions. */
 	for (inplace_alter_handler_ctx** pctx = ctx_array; *pctx; pctx++) {

storage/innobase/include/dict0mem.h

@@ -2244,24 +2244,6 @@ struct dict_table_t {
 	any latch, because this is only used for heuristics. */
 	ib_uint64_t				stat_modified_counter;
 
-	/** Background stats thread is not working on this table. */
-	#define BG_STAT_NONE			0
-
-	/** Set in 'stats_bg_flag' when the background stats code is working
-	on this table. The DROP TABLE code waits for this to be cleared before
-	proceeding. */
-	#define BG_STAT_IN_PROGRESS		(1 << 0)
-
-	/** Set in 'stats_bg_flag' when DROP TABLE starts waiting on
-	BG_STAT_IN_PROGRESS to be cleared. The background stats thread will
-	detect this and will eventually quit sooner. */
-	#define BG_STAT_SHOULD_QUIT		(1 << 1)
-
-	/** The state of the background stats thread wrt this table.
-	See BG_STAT_NONE, BG_STAT_IN_PROGRESS and BG_STAT_SHOULD_QUIT.
-	Writes are covered by dict_sys.latch and stats_mutex_lock(). */
-	byte					stats_bg_flag;
-
 	bool		stats_error_printed;
 				/*!< Has persistent stats error beein
 				already printed for this table ? */

storage/innobase/include/dict0stats_bg.h

@@ -39,54 +39,9 @@ extern mysql_pfs_key_t	recalc_pool_mutex_key;
 extern my_bool		innodb_dict_stats_disabled_debug;
 #endif /* UNIV_DEBUG */
 
-/*****************************************************************//**
-Delete a given table from the auto recalc pool.
-dict_stats_recalc_pool_del() */
-void
-dict_stats_recalc_pool_del(
-/*=======================*/
-	const dict_table_t*	table);	/*!< in: table to remove */
-
-/** Yield the data dictionary latch when waiting
-for the background thread to stop accessing a table.
-@param trx	transaction holding the data dictionary locks */
-#define DICT_BG_YIELD	do {	\
-	dict_sys.unlock();	\
-	std::this_thread::sleep_for(std::chrono::milliseconds(250));	\
-	dict_sys.lock(SRW_LOCK_CALL); \
-} while (0)
-
-/*****************************************************************//**
-Request the background collection of statistics to stop for a table.
-@retval true when no background process is active
-@retval false when it is not safe to modify the table definition */
-UNIV_INLINE
-bool
-dict_stats_stop_bg(
-/*===============*/
-	dict_table_t*	table)	/*!< in/out: table */
-{
-	ut_ad(!srv_read_only_mode);
-	ut_ad(dict_sys.locked());
-
-	if (!(table->stats_bg_flag & BG_STAT_IN_PROGRESS)) {
-		return(true);
-	}
-
-	/* In dict_stats_update_persistent() this flag is being read
-	while holding the mutex, not dict_sys.latch. */
-	table->stats_mutex_lock();
-	table->stats_bg_flag |= BG_STAT_SHOULD_QUIT;
-	table->stats_mutex_unlock();
-	return(false);
-}
-
-/*****************************************************************//**
-Wait until background stats thread has stopped using the specified table.
-The background stats thread is guaranteed not to start using the specified
-table after this function returns and before the caller releases
-dict_sys.latch. */
-void dict_stats_wait_bg_to_stop_using_table(dict_table_t *table);
+/** Delete a table from the auto recalc pool, and ensure that
+no statistics are being updated on it. */
+void dict_stats_recalc_pool_del(table_id_t id, bool have_mdl_exclusive);
 
 /*****************************************************************//**
 Initialize global variables needed for the operation of dict_stats_thread().

storage/innobase/row/row0mysql.cc

@@ -2517,8 +2517,6 @@ dberr_t row_discard_tablespace_for_mysql(dict_table_t *table, trx_t *trx)
   }
 
   row_mysql_lock_data_dictionary(trx);
-  dict_stats_wait_bg_to_stop_using_table(table);
-
   trx->op_info = "discarding tablespace";
   trx->dict_operation= true;
 
@@ -2528,7 +2526,6 @@ dberr_t row_discard_tablespace_for_mysql(dict_table_t *table, trx_t *trx)
   err= row_discard_tablespace_foreign_key_checks(trx, table);
   if (err != DB_SUCCESS)
   {
-    table->stats_bg_flag= BG_STAT_NONE;
     row_mysql_unlock_data_dictionary(trx);
     goto rollback;
   }