MDEV-23991 dict_table_stats_lock() has unnecessarily long scope

Patch removes dict_index_t::stats_latch. Table/index statistics now protected with dict_sys->mutex. That way statistics computation can happen in parallel in several threads and dict_sys->mutex will be locked only for a short period of time. This patch is a joint work with Marko Mäkelä dict_index_t::lock: make mutable which allows to pass const pointer when only lock is touched in an object btr_height_get() btr_get_size(): make index argument const for better type safety btr_estimate_number_of_different_key_vals(): now returns computed values instead of setting fields in dict_index_t directly remove everything related to dict_index_t::stats_latch dict_stats_index_set_n_diff(): now returns computed values instead of setting fields in dict_index_t directly dict_stats_analyze_index(): now returns computed values instead of setting fields in dict_index_t directly Reviewed by: Marko Mäkelä

Author: kevgs

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

@@ -0,0 +1,18 @@
+#
+# MDEV-23991 dict_table_stats_lock() has unnecessarily long scope
+#
+CREATE TABLE t1(a INT) ENGINE=INNODB STATS_PERSISTENT=1;
+SET DEBUG_SYNC='dict_stats_update_persistent SIGNAL stop WAIT_FOR go';
+ANALYZE TABLE t1;
+connect con1, localhost, root;
+SET DEBUG_SYNC='now WAIT_FOR stop';
+SELECT ENGINE,SUM(DATA_LENGTH+INDEX_LENGTH),COUNT(ENGINE),SUM(DATA_LENGTH),SUM(INDEX_LENGTH) FROM information_schema.TABLES WHERE ENGINE='InnoDB';
+ENGINE	SUM(DATA_LENGTH+INDEX_LENGTH)	COUNT(ENGINE)	SUM(DATA_LENGTH)	SUM(INDEX_LENGTH)
+InnoDB	49152	3	49152	0
+SET DEBUG_SYNC='now SIGNAL go';
+disconnect con1;
+connection default;
+Table	Op	Msg_type	Msg_text
+test.t1	analyze	status	OK
+SET DEBUG_SYNC= 'RESET';
+DROP TABLE t1;

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

@@ -0,0 +1,27 @@
+--source include/have_innodb.inc
+--source include/have_debug.inc
+--source include/have_debug_sync.inc
+--source include/count_sessions.inc
+
+--echo #
+--echo # MDEV-23991 dict_table_stats_lock() has unnecessarily long scope
+--echo #
+CREATE TABLE t1(a INT) ENGINE=INNODB STATS_PERSISTENT=1;
+
+SET DEBUG_SYNC='dict_stats_update_persistent SIGNAL stop WAIT_FOR go';
+--send ANALYZE TABLE t1
+
+--connect(con1, localhost, root)
+SET DEBUG_SYNC='now WAIT_FOR stop';
+
+SELECT ENGINE,SUM(DATA_LENGTH+INDEX_LENGTH),COUNT(ENGINE),SUM(DATA_LENGTH),SUM(INDEX_LENGTH) FROM information_schema.TABLES WHERE ENGINE='InnoDB';
+
+SET DEBUG_SYNC='now SIGNAL go';
+--disconnect con1
+
+--connection default
+--reap
+SET DEBUG_SYNC= 'RESET';
+DROP TABLE t1;
+
+--source include/wait_until_count_sessions.inc

storage/innobase/btr/btr0btr.cc

@@ -283,7 +283,7 @@ the index.
 ulint
 btr_height_get(
 /*===========*/
-dict_index_t*	index,/*!< in: index tree */
+const dict_index_t*	index,/*!< in: index tree */
 mtr_t*	mtr)/*!< in/out: mini-transaction */
 {
 ulint	height=0;
@@ -592,7 +592,7 @@ Gets the number of pages in a B-tree.
 ulint
 btr_get_size(
 /*=========*/
-dict_index_t*	index,/*!< in: index */
+const dict_index_t*	index,/*!< in: index */
 ulint	flag,/*!< in: BTR_N_LEAF_PAGES or BTR_TOTAL_SIZE */
 mtr_t*	mtr)/*!< in/out: mini-transaction where index
 is s-latched */

storage/innobase/btr/btr0cur.cc

@@ -6133,21 +6133,19 @@ btr_record_not_null_field_in_rec(
 }
 }
 
-/*******************************************************************//**
-Estimates the number of different key values in a given index, for
+/** Estimates the number of different key values in a given index, for
 each n-column prefix of the index where 1 <= n <= dict_index_get_n_unique(index).
 The estimates are stored in the array index->stat_n_diff_key_vals[] (indexed
 0..n_uniq-1) and the number of pages that were sampled is saved in
-index->stat_n_sample_sizes[].
+result.n_sample_sizes[].
 If innodb_stats_method is nulls_ignored, we also record the number of
 non-null values for each prefix and stored the estimates in
-array index->stat_n_non_null_key_vals.
-@return true if the index is available and we get the estimated numbers,
-false if the index is unavailable. */
-bool
-btr_estimate_number_of_different_key_vals(
-/*======================================*/
-dict_index_t*	index)/*!< in: index */
+array result.n_non_null_key_vals.
+@param[in]	index	index
+@return vector with statistics information
+empty vector if the index is unavailable. */
+std::vector<index_field_stats_t>
+btr_estimate_number_of_different_key_vals(dict_index_t* index)
 {
 btr_cur_tcursor;
 page_t*	page;
@@ -6167,11 +6165,11 @@ btr_estimate_number_of_different_key_vals(
 rec_offs*	offsets_rec	= NULL;
 rec_offs*	offsets_next_rec = NULL;
 
+std::vector<index_field_stats_t> result;
+
 /* For spatial index, there is no such stats can be
 fetched. */
-if (dict_index_is_spatial(index)) {
-return(false);
-}
+ut_ad(!dict_index_is_spatial(index));
 
 n_cols = dict_index_get_n_unique(index);
 
@@ -6280,7 +6278,7 @@ btr_estimate_number_of_different_key_vals(
 mtr_commit(&mtr);
 mem_heap_free(heap);
 
-return(false);
+return result;
 }
 
 /* Count the number of different key values for each prefix of
@@ -6386,8 +6384,12 @@ 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) */
 
+result.reserve(n_cols);
+
 for (j = 0; j < n_cols; j++) {
-index->stat_n_diff_key_vals[j]
+index_field_stats_t stat;
+
+stat.n_diff_key_vals
 = BTR_TABLE_STATS_FROM_SAMPLE(
 n_diff[j], index, n_sample_pages,
 total_external_size, not_empty_flag);
@@ -6408,25 +6410,23 @@ btr_estimate_number_of_different_key_vals(
 add_on = n_sample_pages;
 }
 
-index->stat_n_diff_key_vals[j] += add_on;
+stat.n_diff_key_vals += add_on;
 
-index->stat_n_sample_sizes[j] = n_sample_pages;
+stat.n_sample_sizes = n_sample_pages;
 
-/* Update the stat_n_non_null_key_vals[] with our
-sampled result. stat_n_non_null_key_vals[] is created
-and initialized to zero in dict_index_add_to_cache(),
-along with stat_n_diff_key_vals[] array */
 if (n_not_null != NULL) {
-index->stat_n_non_null_key_vals[j] =
+stat.n_non_null_key_vals =
  BTR_TABLE_STATS_FROM_SAMPLE(
 n_not_null[j], index, n_sample_pages,
 total_external_size, not_empty_flag);
 }
+
+result.push_back(stat);
 }
 
 mem_heap_free(heap);
 
-return(true);
+return result;
 }
 
 /*================== EXTERNAL STORAGE OF BIG FIELDS ===================*/

storage/innobase/dict/dict0dict.cc

@@ -267,56 +267,6 @@ dict_mutex_exit_for_mysql(void)
 mutex_exit(&dict_sys->mutex);
 }
 
-/** Lock the appropriate latch to protect a given table's statistics.
-@param[in]	table	table whose stats to lock
-@param[in]	latch_mode	RW_S_LATCH or RW_X_LATCH */
-void
-dict_table_stats_lock(
-dict_table_t*	table,
-ulint	latch_mode)
-{
-ut_ad(table != NULL);
-ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
-
-switch (latch_mode) {
-case RW_S_LATCH:
-rw_lock_s_lock(&table->stats_latch);
-break;
-case RW_X_LATCH:
-rw_lock_x_lock(&table->stats_latch);
-break;
-case RW_NO_LATCH:
-/* fall through */
-default:
-ut_error;
-}
-}
-
-/** Unlock the latch that has been locked by dict_table_stats_lock().
-@param[in]	table	table whose stats to unlock
-@param[in]	latch_mode	RW_S_LATCH or RW_X_LATCH */
-void
-dict_table_stats_unlock(
-dict_table_t*	table,
-ulint	latch_mode)
-{
-ut_ad(table != NULL);
-ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
-
-switch (latch_mode) {
-case RW_S_LATCH:
-rw_lock_s_unlock(&table->stats_latch);
-break;
-case RW_X_LATCH:
-rw_lock_x_unlock(&table->stats_latch);
-break;
-case RW_NO_LATCH:
-/* fall through */
-default:
-ut_error;
-}
-}
-
 /**********************************************************************//**
 Try to drop any indexes after an aborted index creation.
 This can also be after a server kill during DROP INDEX. */

storage/innobase/dict/dict0mem.cc

@@ -125,8 +125,7 @@ dict_mem_table_create(
 ulint	n_cols,
 ulint	n_v_cols,
 ulint	flags,
-ulint	flags2,
-boolinit_stats_latch)
+ulint	flags2)
 {
 dict_table_t*	table;
 mem_heap_t*	heap;
@@ -182,12 +181,6 @@ dict_mem_table_create(
 new(&table->foreign_set) dict_foreign_set();
 new(&table->referenced_set) dict_foreign_set();
 
-if (init_stats_latch) {
-rw_lock_create(dict_table_stats_key, &table->stats_latch,
- SYNC_INDEX_TREE);
-table->stats_latch_inited = true;
-}
-
 return(table);
 }
 
@@ -237,10 +230,6 @@ dict_mem_table_free(
 UT_DELETE(table->s_cols);
 }
 
-if (table->stats_latch_inited) {
-rw_lock_free(&table->stats_latch);
-}
-
 mem_heap_free(table->heap);
 }