Fix Bug#53046 dict_update_statistics_low can still be run concurrently on same table

Replace the array of mutexes that used to protect
dict_index_t::stat_n_diff_key_vals[] with an array of rw locks that protects
all the stats related members in dict_table_t and all of its indexes.

Approved by:	Jimmy (rb://503)

mysql-test/suite/innodb_plugin/r/innodb_bug53046.result

+CREATE TABLE bug53046_1 (c1 INT PRIMARY KEY) ENGINE=INNODB;
+CREATE TABLE bug53046_2 (c2 INT PRIMARY KEY,
+FOREIGN KEY (c2) REFERENCES bug53046_1(c1)
+ON UPDATE CASCADE ON DELETE CASCADE) ENGINE=INNODB;
+INSERT INTO bug53046_1 VALUES (1);
+INSERT INTO bug53046_1 SELECT c1+(SELECT MAX(c1) FROM bug53046_1)
+FROM bug53046_1;
+INSERT INTO bug53046_1 SELECT c1+(SELECT MAX(c1) FROM bug53046_1)
+FROM bug53046_1;
+INSERT INTO bug53046_1 SELECT c1+(SELECT MAX(c1) FROM bug53046_1)
+FROM bug53046_1;
+INSERT INTO bug53046_1 SELECT c1+(SELECT MAX(c1) FROM bug53046_1)
+FROM bug53046_1;
+INSERT INTO bug53046_1 SELECT c1+(SELECT MAX(c1) FROM bug53046_1)
+FROM bug53046_1;
+INSERT INTO bug53046_2 VALUES (1), (2);
+ANALYZE TABLE bug53046_1;
+Table	Op	Msg_type	Msg_text
+test.bug53046_1	analyze	status	OK
+SHOW TABLE STATUS LIKE 'bug53046_1';
+UPDATE bug53046_1 SET c1 = c1 - 1;
+DELETE FROM bug53046_1;
+INSERT INTO bug53046_1 VALUES (1);
+INSERT INTO bug53046_2 VALUES (1);
+TRUNCATE TABLE bug53046_2;
+DROP TABLE bug53046_2;
+DROP TABLE bug53046_1;

mysql-test/suite/innodb_plugin/t/innodb_bug53046.test

+#
+# http://bugs.mysql.com/53046
+# dict_update_statistics_low can still be run concurrently on same table
+#
+# This is a symbolic test, it would not fail if the bug is present.
+# Rather those SQL commands have been used during manual testing under
+# UNIV_DEBUG & UNIV_SYNC_DEBUG to test all changed codepaths for locking
+# correctness.
+#
+
+-- source include/have_innodb_plugin.inc
+
+CREATE TABLE bug53046_1 (c1 INT PRIMARY KEY) ENGINE=INNODB;
+CREATE TABLE bug53046_2 (c2 INT PRIMARY KEY,
+		 FOREIGN KEY (c2) REFERENCES bug53046_1(c1)
+		 ON UPDATE CASCADE ON DELETE CASCADE) ENGINE=INNODB;
+
+INSERT INTO bug53046_1 VALUES (1);
+let $i = 5;
+while ($i) {
+	eval INSERT INTO bug53046_1 SELECT c1+(SELECT MAX(c1) FROM bug53046_1)
+		FROM bug53046_1;
+	dec $i;
+}
+
+INSERT INTO bug53046_2 VALUES (1), (2);
+
+# CREATE TABLE innodb_table_monitor (a int) ENGINE=INNODB;
+# wait more than 1 minute and observe the mysqld output
+# DROP TABLE innodb_table_monitor;
+
+ANALYZE TABLE bug53046_1;
+
+# this prints create time and other nondeterministic data
+-- disable_result_log
+SHOW TABLE STATUS LIKE 'bug53046_1';
+-- enable_result_log
+
+UPDATE bug53046_1 SET c1 = c1 - 1;
+
+DELETE FROM bug53046_1;
+
+INSERT INTO bug53046_1 VALUES (1);
+INSERT INTO bug53046_2 VALUES (1);
+TRUNCATE TABLE bug53046_2;
+
+DROP TABLE bug53046_2;
+DROP TABLE bug53046_1;

storage/innodb_plugin/btr/btr0cur.c

@@ -3355,8 +3355,6 @@ btr_estimate_number_of_different_key_vals(
 	also the pages used for external storage of fields (those pages are
 	included in index->stat_n_leaf_pages) */
 
-	dict_index_stat_mutex_enter(index);
-
 	for (j = 0; j <= n_cols; j++) {
 		index->stat_n_diff_key_vals[j]
 			= ((n_diff[j]
@@ -3386,8 +3384,6 @@ btr_estimate_number_of_different_key_vals(
 		index->stat_n_diff_key_vals[j] += add_on;
 	}
 
-	dict_index_stat_mutex_exit(index);
-
 	mem_free(n_diff);
 	if (UNIV_LIKELY_NULL(heap)) {
 		mem_heap_free(heap);

storage/innodb_plugin/dict/dict0dict.c

@@ -80,9 +80,18 @@ UNIV_INTERN rw_lock_t	dict_operation_lock;
 /** Identifies generated InnoDB foreign key names */
 static char	dict_ibfk[] = "_ibfk_";
 
-/** array of mutexes protecting dict_index_t::stat_n_diff_key_vals[] */
-#define DICT_INDEX_STAT_MUTEX_SIZE	32
-static mutex_t	dict_index_stat_mutex[DICT_INDEX_STAT_MUTEX_SIZE];
+/** array of rw locks protecting
+dict_table_t::stat_initialized
+dict_table_t::stat_n_rows (*)
+dict_table_t::stat_clustered_index_size
+dict_table_t::stat_sum_of_other_index_sizes
+dict_table_t::stat_modified_counter (*)
+dict_table_t::indexes*::stat_n_diff_key_vals[]
+dict_table_t::indexes*::stat_index_size
+dict_table_t::indexes*::stat_n_leaf_pages
+(*) those are not always protected for performance reasons */
+#define DICT_TABLE_STATS_LATCHES_SIZE	64
+static rw_lock_t	dict_table_stats_latches[DICT_TABLE_STATS_LATCHES_SIZE];
 
 /*******************************************************************//**
 Tries to find column names for the index and sets the col field of the
@@ -243,43 +252,65 @@ dict_mutex_exit_for_mysql(void)
 	mutex_exit(&(dict_sys->mutex));
 }
 
-/** Get the mutex that protects index->stat_n_diff_key_vals[] */
-#define GET_INDEX_STAT_MUTEX(index) \
-	(&dict_index_stat_mutex[ut_fold_dulint(index->id) \
-	 			% DICT_INDEX_STAT_MUTEX_SIZE])
+/** Get the latch that protects the stats of a given table */
+#define GET_TABLE_STATS_LATCH(table) \
+	(&dict_table_stats_latches[ut_fold_dulint(table->id) \
+				   % DICT_TABLE_STATS_LATCHES_SIZE])
 
 /**********************************************************************//**
-Lock the appropriate mutex to protect index->stat_n_diff_key_vals[].
-index->id is used to pick the right mutex and it should not change
-before dict_index_stat_mutex_exit() is called on this index. */
+Lock the appropriate latch to protect a given table's statistics.
+table->id is used to pick the corresponding latch from a global array of
+latches. */
 UNIV_INTERN
 void
-dict_index_stat_mutex_enter(
-/*========================*/
-	const dict_index_t*	index)	/*!< in: index */
+dict_table_stats_lock(
+/*==================*/
+	const dict_table_t*	table,		/*!< in: table */
+	ulint			latch_mode)	/*!< in: RW_S_LATCH or
+						RW_X_LATCH */
 {
-	ut_ad(index != NULL);
-	ut_ad(index->magic_n == DICT_INDEX_MAGIC_N);
-	ut_ad(index->cached);
-	ut_ad(!index->to_be_dropped);
+	ut_ad(table != NULL);
+	ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
 
-	mutex_enter(GET_INDEX_STAT_MUTEX(index));
+	switch (latch_mode) {
+	case RW_S_LATCH:
+		rw_lock_s_lock(GET_TABLE_STATS_LATCH(table));
+		break;
+	case RW_X_LATCH:
+		rw_lock_x_lock(GET_TABLE_STATS_LATCH(table));
+		break;
+	case RW_NO_LATCH:
+		/* fall through */
+	default:
+		ut_error;
+	}
 }
 
 /**********************************************************************//**
-Unlock the appropriate mutex that protects index->stat_n_diff_key_vals[]. */
+Unlock the latch that has been locked by dict_table_stats_lock() */
 UNIV_INTERN
 void
-dict_index_stat_mutex_exit(
-/*=======================*/
-	const dict_index_t*	index)	/*!< in: index */
+dict_table_stats_unlock(
+/*====================*/
+	const dict_table_t*	table,		/*!< in: table */
+	ulint			latch_mode)	/*!< in: RW_S_LATCH or
+						RW_X_LATCH */
 {
-	ut_ad(index != NULL);
-	ut_ad(index->magic_n == DICT_INDEX_MAGIC_N);
-	ut_ad(index->cached);
-	ut_ad(!index->to_be_dropped);
+	ut_ad(table != NULL);
+	ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
 
-	mutex_exit(GET_INDEX_STAT_MUTEX(index));
+	switch (latch_mode) {
+	case RW_S_LATCH:
+		rw_lock_s_unlock(GET_TABLE_STATS_LATCH(table));
+		break;
+	case RW_X_LATCH:
+		rw_lock_x_unlock(GET_TABLE_STATS_LATCH(table));
+		break;
+	case RW_NO_LATCH:
+		/* fall through */
+	default:
+		ut_error;
+	}
 }
 
 /********************************************************************//**
@@ -668,8 +699,8 @@ dict_init(void)
 
 	mutex_create(&dict_foreign_err_mutex, SYNC_ANY_LATCH);
 
-	for (i = 0; i < DICT_INDEX_STAT_MUTEX_SIZE; i++) {
-		mutex_create(&dict_index_stat_mutex[i], SYNC_INDEX_TREE);
+	for (i = 0; i < DICT_TABLE_STATS_LATCHES_SIZE; i++) {
+		rw_lock_create(&dict_table_stats_latches[i], SYNC_INDEX_TREE);
 	}
 }
 
@@ -700,12 +731,11 @@ dict_table_get(
 	mutex_exit(&(dict_sys->mutex));
 
 	if (table != NULL) {
-		if (!table->stat_initialized) {
-			/* If table->ibd_file_missing == TRUE, this will
-			print an error message and return without doing
-			anything. */
-			dict_update_statistics(table);
-		}
+		/* If table->ibd_file_missing == TRUE, this will
+		print an error message and return without doing
+		anything. */
+		dict_update_statistics(table, TRUE /* only update stats
+				       if they have not been initialized */);
 	}
 
 	return(table);
@@ -4186,6 +4216,10 @@ void
 dict_update_statistics_low(
 /*=======================*/
 	dict_table_t*	table,		/*!< in/out: table */
+	ibool		only_calc_if_missing_stats,/*!< in: only
+					update/recalc the stats if they have
+					not been initialized yet, otherwise
+					do nothing */
 	ibool		has_dict_mutex __attribute__((unused)))
 					/*!< in: TRUE if the caller has the
 					dictionary mutex */
@@ -4216,6 +4250,12 @@ dict_update_statistics_low(
 		return;
 	}
 
+	dict_table_stats_lock(table, RW_X_LATCH);
+
+	if (only_calc_if_missing_stats && table->stat_initialized) {
+		dict_table_stats_unlock(table, RW_X_LATCH);
+		return;
+	}
 
 	do {
 		if (UNIV_LIKELY
@@ -4261,13 +4301,9 @@ dict_update_statistics_low(
 
 	index = dict_table_get_first_index(table);
 
-	dict_index_stat_mutex_enter(index);
-
 	table->stat_n_rows = index->stat_n_diff_key_vals[
 		dict_index_get_n_unique(index)];
 
-	dict_index_stat_mutex_exit(index);
-
 	table->stat_clustered_index_size = index->stat_index_size;
 
 	table->stat_sum_of_other_index_sizes = sum_of_index_sizes
@@ -4276,6 +4312,8 @@ dict_update_statistics_low(
 	table->stat_initialized = TRUE;
 
 	table->stat_modified_counter = 0;
+
+	dict_table_stats_unlock(table, RW_X_LATCH);
 }
 
 /*********************************************************************//**
@@ -4285,9 +4323,13 @@ UNIV_INTERN
 void
 dict_update_statistics(
 /*===================*/
-	dict_table_t*	table)	/*!< in/out: table */
+	dict_table_t*	table,	/*!< in/out: table */
+	ibool		only_calc_if_missing_stats)/*!< in: only
+					update/recalc the stats if they have
+					not been initialized yet, otherwise
+					do nothing */
 {
-	dict_update_statistics_low(table, FALSE);
+	dict_update_statistics_low(table, only_calc_if_missing_stats, FALSE);
 }
 
 /**********************************************************************//**
@@ -4367,7 +4409,11 @@ dict_table_print_low(
 
 	ut_ad(mutex_own(&(dict_sys->mutex)));
 
-	dict_update_statistics_low(table, TRUE);
+	dict_update_statistics_low(table,
+				   FALSE /* update even if initialized */,
+				   TRUE /* we have the dict mutex */);
+
+	dict_table_stats_lock(table, RW_S_LATCH);
 
 	fprintf(stderr,
 		"--------------------------------------\n"
@@ -4396,6 +4442,8 @@ dict_table_print_low(
 		index = UT_LIST_GET_NEXT(indexes, index);
 	}
 
+	dict_table_stats_unlock(table, RW_S_LATCH);
+
 	foreign = UT_LIST_GET_FIRST(table->foreign_list);
 
 	while (foreign != NULL) {
@@ -4444,8 +4492,6 @@ dict_index_print_low(
 
 	ut_ad(mutex_own(&(dict_sys->mutex)));
 
-	dict_index_stat_mutex_enter(index);
-
 	if (index->n_user_defined_cols > 0) {
 		n_vals = index->stat_n_diff_key_vals[
 			index->n_user_defined_cols];
@@ -4453,8 +4499,6 @@ dict_index_print_low(
 		n_vals = index->stat_n_diff_key_vals[1];
 	}
 
-	dict_index_stat_mutex_exit(index);
-
 	fprintf(stderr,
 		"  INDEX: name %s, id %lu %lu, fields %lu/%lu,"
 		" uniq %lu, type %lu\n"
@@ -4943,8 +4987,8 @@ dict_close(void)
 	mem_free(dict_sys);
 	dict_sys = NULL;
 
-	for (i = 0; i < DICT_INDEX_STAT_MUTEX_SIZE; i++) {
-		mutex_free(&dict_index_stat_mutex[i]);
+	for (i = 0; i < DICT_TABLE_STATS_LATCHES_SIZE; i++) {
+		rw_lock_free(&dict_table_stats_latches[i]);
 	}
 }
 #endif /* !UNIV_HOTBACKUP */

storage/innodb_plugin/dict/dict0load.c

@@ -222,7 +222,10 @@ dict_print(void)
 			is no index */
 
 			if (dict_table_get_first_index(table)) {
-				dict_update_statistics_low(table, TRUE);
+				dict_update_statistics_low(
+					table,
+					FALSE /* update even if initialized */,
+					TRUE /* we have the dict mutex */);
 			}
 
 			dict_table_print_low(table);

storage/innodb_plugin/handler/ha_innodb.cc

@@ -7343,6 +7343,7 @@ ha_innobase::estimate_rows_upper_bound(void)
 	dict_index_t*	index;
 	ulonglong	estimate;
 	ulonglong	local_data_file_length;
+	ulint		stat_n_leaf_pages;
 
 	DBUG_ENTER("estimate_rows_upper_bound");
 
@@ -7362,10 +7363,12 @@ ha_innobase::estimate_rows_upper_bound(void)
 
 	index = dict_table_get_first_index(prebuilt->table);
 
-	ut_a(index->stat_n_leaf_pages > 0);
+	stat_n_leaf_pages = index->stat_n_leaf_pages;
+
+	ut_a(stat_n_leaf_pages > 0);
 
 	local_data_file_length =
-		((ulonglong) index->stat_n_leaf_pages) * UNIV_PAGE_SIZE;
+		((ulonglong) stat_n_leaf_pages) * UNIV_PAGE_SIZE;
 
 
 	/* Calculate a minimum length for a clustered index record and from
@@ -7564,7 +7567,9 @@ ha_innobase::info_low(
 
 			prebuilt->trx->op_info = "updating table statistics";
 
-			dict_update_statistics(ib_table);
+			dict_update_statistics(ib_table,
+					       FALSE /* update even if stats
+						     are initialized */);
 
 			prebuilt->trx->op_info = "returning various info to MySQL";
 		}
@@ -7583,6 +7588,9 @@ ha_innobase::info_low(
 	}
 
 	if (flag & HA_STATUS_VARIABLE) {
+
+		dict_table_stats_lock(ib_table, RW_S_LATCH);
+
 		n_rows = ib_table->stat_n_rows;
 
 		/* Because we do not protect stat_n_rows by any mutex in a
@@ -7632,6 +7640,8 @@ ha_innobase::info_low(
 				ib_table->stat_sum_of_other_index_sizes)
 					* UNIV_PAGE_SIZE;
 
+		dict_table_stats_unlock(ib_table, RW_S_LATCH);
+
 		/* Since fsp_get_available_space_in_free_extents() is
 		acquiring latches inside InnoDB, we do not call it if we
 		are asked by MySQL to avoid locking. Another reason to
@@ -7708,6 +7718,8 @@ ha_innobase::info_low(
 					table->s->keys);
 		}
 
+		dict_table_stats_lock(ib_table, RW_S_LATCH);
+
 		for (i = 0; i < table->s->keys; i++) {
 			ulong	j;
 			/* We could get index quickly through internal
@@ -7745,8 +7757,6 @@ ha_innobase::info_low(
 					break;
 				}
 
-				dict_index_stat_mutex_enter(index);
-
 				if (index->stat_n_diff_key_vals[j + 1] == 0) {
 
 					rec_per_key = stats.records;
@@ -7755,8 +7765,6 @@ ha_innobase::info_low(
 					 index->stat_n_diff_key_vals[j + 1]);
 				}
 
-				dict_index_stat_mutex_exit(index);
-
 				/* Since MySQL seems to favor table scans
 				too much over index searches, we pretend
 				index selectivity is 2 times better than
@@ -7773,6 +7781,8 @@ ha_innobase::info_low(
 				  (ulong) rec_per_key;
 			}
 		}
+
+		dict_table_stats_unlock(ib_table, RW_S_LATCH);
 	}
 
 	if (srv_force_recovery >= SRV_FORCE_NO_IBUF_MERGE) {

storage/innodb_plugin/include/dict0dict.h

@@ -1056,6 +1056,10 @@ void
 dict_update_statistics_low(
 /*=======================*/
 	dict_table_t*	table,		/*!< in/out: table */
+	ibool		only_calc_if_missing_stats,/*!< in: only
+					update/recalc the stats if they have
+					not been initialized yet, otherwise
+					do nothing */
 	ibool		has_dict_mutex);/*!< in: TRUE if the caller has the
 					dictionary mutex */
 /*********************************************************************//**
@@ -1065,7 +1069,11 @@ UNIV_INTERN
 void
 dict_update_statistics(
 /*===================*/
-	dict_table_t*	table);	/*!< in/out: table */
+	dict_table_t*	table,	/*!< in/out: table */
+	ibool		only_calc_if_missing_stats);/*!< in: only
+					update/recalc the stats if they have
+					not been initialized yet, otherwise
+					do nothing */
 /********************************************************************//**
 Reserves the dictionary system mutex for MySQL. */
 UNIV_INTERN
@@ -1079,21 +1087,25 @@ void
 dict_mutex_exit_for_mysql(void);
 /*===========================*/
 /**********************************************************************//**
-Lock the appropriate mutex to protect index->stat_n_diff_key_vals[].
-index->id is used to pick the right mutex and it should not change
-before dict_index_stat_mutex_exit() is called on this index. */
+Lock the appropriate latch to protect a given table's statistics.
+table->id is used to pick the corresponding latch from a global array of
+latches. */
 UNIV_INTERN
 void
-dict_index_stat_mutex_enter(
-/*========================*/
-	const dict_index_t*	index);	/*!< in: index */
+dict_table_stats_lock(
+/*==================*/
+	const dict_table_t*	table,		/*!< in: table */
+	ulint			latch_mode);	/*!< in: RW_S_LATCH or
+						RW_X_LATCH */
 /**********************************************************************//**
-Unlock the appropriate mutex that protects index->stat_n_diff_key_vals[]. */
+Unlock the latch that has been locked by dict_table_stats_lock() */
 UNIV_INTERN
 void
-dict_index_stat_mutex_exit(
-/*=======================*/
-	const dict_index_t*	index);	/*!< in: index */
+dict_table_stats_unlock(
+/*====================*/
+	const dict_table_t*	table,		/*!< in: table */
+	ulint			latch_mode);	/*!< in: RW_S_LATCH or
+						RW_X_LATCH */
 /********************************************************************//**
 Checks if the database name in two table names is the same.
 @return	TRUE if same db name */

storage/innodb_plugin/row/row0mysql.c

@@ -871,7 +871,8 @@ row_update_statistics_if_needed(
 	if (counter > 2000000000
 	    || ((ib_int64_t)counter > 16 + table->stat_n_rows / 16)) {
 
-		dict_update_statistics(table);
+		dict_update_statistics(table, FALSE /* update even if stats
+						    are initialized */);
 	}
 }
 
@@ -2953,7 +2954,8 @@ row_truncate_table_for_mysql(
 	dict_table_autoinc_lock(table);
 	dict_table_autoinc_initialize(table, 1);
 	dict_table_autoinc_unlock(table);
-	dict_update_statistics(table);
+	dict_update_statistics(table, FALSE /* update even if stats are
+					    initialized */);
 
 	trx_commit_for_mysql(trx);