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ä

3 files changed, 133 insertions, 140 deletions

diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc
index 37a8a4387d1..e3ce13b5b27 100644
--- a/storage/innobase/dict/dict0dict.cc
+++ b/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. */
diff --git a/storage/innobase/dict/dict0mem.cc b/storage/innobase/dict/dict0mem.cc
index 11d362d32c6..efefa40e69f 100644
--- a/storage/innobase/dict/dict0mem.cc
+++ b/storage/innobase/dict/dict0mem.cc
@@ -125,8 +125,7 @@ dict_mem_table_create(
 	ulint		n_cols,
 	ulint		n_v_cols,
 	ulint		flags,
-	ulint		flags2,
-	bool		init_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);
 }
 
diff --git a/storage/innobase/dict/dict0stats.cc b/storage/innobase/dict/dict0stats.cc
index 5083fda48ee..bf2645fd65c 100644
--- a/storage/innobase/dict/dict0stats.cc
+++ b/storage/innobase/dict/dict0stats.cc
@@ -486,8 +486,6 @@ dict_stats_table_clone_create(
 
 	ut_d(t->magic_n = DICT_TABLE_MAGIC_N);
 
-	rw_lock_create(dict_table_stats_key, &t->stats_latch, SYNC_INDEX_TREE);
-
 	return(t);
 }
 
@@ -500,15 +498,12 @@ dict_stats_table_clone_free(
 /*========================*/
 	dict_table_t*	t)	/*!< in: dummy table object to free */
 {
-	rw_lock_free(&t->stats_latch);
 	mem_heap_free(t->heap);
 }
 
 /*********************************************************************//**
 Write all zeros (or 1 where it makes sense) into an index
-statistics members. The resulting stats correspond to an empty index.
-The caller must own index's table stats latch in X mode
-(dict_table_stats_lock(table, RW_X_LATCH)) */
+statistics members. The resulting stats correspond to an empty index. */
 static
 void
 dict_stats_empty_index(
@@ -519,6 +514,7 @@ dict_stats_empty_index(
 {
 	ut_ad(!(index->type & DICT_FTS));
 	ut_ad(!dict_index_is_ibuf(index));
+	ut_ad(mutex_own(&dict_sys->mutex));
 
 	ulint	n_uniq = index->n_uniq;
 
@@ -548,10 +544,9 @@ dict_stats_empty_table(
 	bool		empty_defrag_stats)
 				/*!< in: whether to empty defrag stats */
 {
-	/* Zero the stats members */
-
-	dict_table_stats_lock(table, RW_X_LATCH);
+	mutex_enter(&dict_sys->mutex);
 
+	/* Zero the stats members */
 	table->stat_n_rows = 0;
 	table->stat_clustered_index_size = 1;
 	/* 1 page for each index, not counting the clustered */
@@ -575,8 +570,7 @@ dict_stats_empty_table(
 	}
 
 	table->stat_initialized = TRUE;
-
-	dict_table_stats_unlock(table, RW_X_LATCH);
+	mutex_exit(&dict_sys->mutex);
 }
 
 /*********************************************************************//**
@@ -675,6 +669,8 @@ dict_stats_copy(
                                              to have the same statistics as if
                                              the table was empty */
 {
+	ut_ad(mutex_own(&dict_sys->mutex));
+
 	dst->stats_last_recalc = src->stats_last_recalc;
 	dst->stat_n_rows = src->stat_n_rows;
 	dst->stat_clustered_index_size = src->stat_clustered_index_size;
@@ -792,8 +788,6 @@ dict_stats_snapshot_create(
 {
 	mutex_enter(&dict_sys->mutex);
 
-	dict_table_stats_lock(table, RW_S_LATCH);
-
 	dict_stats_assert_initialized(table);
 
 	dict_table_t*	t;
@@ -807,8 +801,6 @@ dict_stats_snapshot_create(
 	t->stats_sample_pages = table->stats_sample_pages;
 	t->stats_bg_flag = table->stats_bg_flag;
 
-	dict_table_stats_unlock(table, RW_S_LATCH);
-
 	mutex_exit(&dict_sys->mutex);
 
 	return(t);
@@ -848,10 +840,14 @@ dict_stats_update_transient_for_index(
 		Initialize some bogus index cardinality
 		statistics, so that the data can be queried in
 		various means, also via secondary indexes. */
+		mutex_enter(&dict_sys->mutex);
 		dict_stats_empty_index(index, false);
+		mutex_exit(&dict_sys->mutex);
 #if defined UNIV_DEBUG || defined UNIV_IBUF_DEBUG
 	} else if (ibuf_debug && !dict_index_is_clust(index)) {
+		mutex_enter(&dict_sys->mutex);
 		dict_stats_empty_index(index, false);
+		mutex_exit(&dict_sys->mutex);
 #endif /* UNIV_DEBUG || UNIV_IBUF_DEBUG */
 	} else {
 		mtr_t	mtr;
@@ -874,7 +870,9 @@ dict_stats_update_transient_for_index(
 
 		switch (size) {
 		case ULINT_UNDEFINED:
+			mutex_enter(&dict_sys->mutex);
 			dict_stats_empty_index(index, false);
+			mutex_exit(&dict_sys->mutex);
 			return;
 		case 0:
 			/* The root node of the tree is a leaf */
@@ -886,11 +884,23 @@ dict_stats_update_transient_for_index(
 		/* Do not continue if table decryption has failed or
 		table is already marked as corrupted. */
 		if (index->is_readable()) {
-			/* We don't handle the return value since it
-			will be false only when some thread is
-			dropping the table and we don't have to empty
-			the statistics of the to be dropped index */
-			btr_estimate_number_of_different_key_vals(index);
+			std::vector<index_field_stats_t> stats
+				= btr_estimate_number_of_different_key_vals(
+					index);
+
+			if (!stats.empty()) {
+				ut_ad(!mutex_own(&dict_sys->mutex));
+				mutex_enter(&dict_sys->mutex);
+				for (size_t i = 0; i < stats.size(); ++i) {
+					index->stat_n_diff_key_vals[i]
+						= stats[i].n_diff_key_vals;
+					index->stat_n_sample_sizes[i]
+						= stats[i].n_sample_sizes;
+					index->stat_n_non_null_key_vals[i]
+						= stats[i].n_non_null_key_vals;
+				}
+				mutex_exit(&dict_sys->mutex);
+			}
 		}
 	}
 }
@@ -907,6 +917,8 @@ dict_stats_update_transient(
 /*========================*/
 	dict_table_t*	table)	/*!< in/out: table */
 {
+	ut_ad(!mutex_own(&dict_sys->mutex));
+
 	dict_index_t*	index;
 	ulint		sum_of_index_sizes	= 0;
 
@@ -932,27 +944,25 @@ dict_stats_update_transient(
 
 		ut_ad(!dict_index_is_ibuf(index));
 
-		if (index->type & DICT_FTS || dict_index_is_spatial(index)) {
+		if (index->type & (DICT_FTS | DICT_SPATIAL)) {
 			continue;
 		}
 
-		dict_stats_empty_index(index, false);
-
-		if (dict_stats_should_ignore_index(index)) {
+		if (dict_stats_should_ignore_index(index)
+		    || !index->is_readable()) {
+			mutex_enter(&dict_sys->mutex);
+			dict_stats_empty_index(index, false);
+			mutex_exit(&dict_sys->mutex);
 			continue;
 		}
 
-		/* Do not continue if table decryption has failed or
-		table is already marked as corrupted. */
-		if (!index->is_readable()) {
-			break;
-		}
-
 		dict_stats_update_transient_for_index(index);
 
 		sum_of_index_sizes += index->stat_index_size;
 	}
 
+	mutex_enter(&dict_sys->mutex);
+
 	index = dict_table_get_first_index(table);
 
 	table->stat_n_rows = index->stat_n_diff_key_vals[
@@ -968,6 +978,8 @@ dict_stats_update_transient(
 	table->stat_modified_counter = 0;
 
 	table->stat_initialized = TRUE;
+
+	mutex_exit(&dict_sys->mutex);
 }
 
 /* @{ Pseudo code about the relation between the following functions
@@ -1808,16 +1820,31 @@ dict_stats_analyze_index_for_n_prefix(
 	btr_pcur_close(&pcur);
 }
 
+/** statistics for an index */
+struct index_stats_t
+{
+  std::vector<index_field_stats_t> stats;
+  ulint index_size;
+  ulint n_leaf_pages;
+
+  index_stats_t(ulint n_uniq) : index_size(1), n_leaf_pages(1)
+  {
+    stats.reserve(n_uniq);
+    for (ulint i= 0; i < n_uniq; ++i)
+      stats.push_back(index_field_stats_t(0, 1, 0));
+  }
+};
+
 /** Set dict_index_t::stat_n_diff_key_vals[] and stat_n_sample_sizes[].
 @param[in]	n_diff_data	input data to use to derive the results
-@param[in,out]	index		index whose stat_n_diff_key_vals[] to set */
+@param[in,out]	index_stats	index stats to set */
 UNIV_INLINE
 void
 dict_stats_index_set_n_diff(
 	const n_diff_data_t*	n_diff_data,
-	dict_index_t*		index)
+	index_stats_t&		index_stats)
 {
-	for (ulint n_prefix = dict_index_get_n_unique(index);
+	for (ulint n_prefix = index_stats.stats.size();
 	     n_prefix >= 1;
 	     n_prefix--) {
 		/* n_diff_all_analyzed_pages can be 0 here if
@@ -1848,14 +1875,14 @@ dict_stats_index_set_n_diff(
 			that the total number of ordinary leaf pages is
 			T * D / (D + E). */
 			n_ordinary_leaf_pages
-				= index->stat_n_leaf_pages
+				= index_stats.n_leaf_pages
 				* data->n_leaf_pages_to_analyze
 				/ (data->n_leaf_pages_to_analyze
 				   + data->n_external_pages_sum);
 		}
 
 		/* See REF01 for an explanation of the algorithm */
-		index->stat_n_diff_key_vals[n_prefix - 1]
+		index_stats.stats[n_prefix - 1].n_diff_key_vals
 			= n_ordinary_leaf_pages
 
 			* data->n_diff_on_level
@@ -1864,7 +1891,7 @@ dict_stats_index_set_n_diff(
 			* data->n_diff_all_analyzed_pages
 			/ data->n_leaf_pages_to_analyze;
 
-		index->stat_n_sample_sizes[n_prefix - 1]
+		index_stats.stats[n_prefix - 1].n_sample_sizes
 			= data->n_leaf_pages_to_analyze;
 
 		DEBUG_PRINTF("    %s(): n_diff=" UINT64PF
@@ -1873,9 +1900,9 @@ dict_stats_index_set_n_diff(
 			     " * " UINT64PF " / " UINT64PF
 			     " * " UINT64PF " / " UINT64PF ")\n",
 			     __func__,
-			     index->stat_n_diff_key_vals[n_prefix - 1],
+			     index_stats.stats[n_prefix - 1].n_diff_key_vals,
 			     n_prefix,
-			     index->stat_n_leaf_pages,
+			     index_stats.n_leaf_pages,
 			     data->n_diff_on_level,
 			     data->n_recs_on_level,
 			     data->n_diff_all_analyzed_pages,
@@ -1883,15 +1910,12 @@ dict_stats_index_set_n_diff(
 	}
 }
 
-/*********************************************************************//**
-Calculates new statistics for a given index and saves them to the index
+/** Calculates new statistics for a given index and saves them to the index
 members stat_n_diff_key_vals[], stat_n_sample_sizes[], stat_index_size and
-stat_n_leaf_pages. This function could be slow. */
-static
-void
-dict_stats_analyze_index(
-/*=====================*/
-	dict_index_t*	index)	/*!< in/out: index to analyze */
+stat_n_leaf_pages. This function can be slow.
+@param[in]	index	index to analyze
+@return index stats */
+static index_stats_t dict_stats_analyze_index(dict_index_t* index)
 {
 	ulint		root_level;
 	ulint		level;
@@ -1902,20 +1926,22 @@ dict_stats_analyze_index(
 	ib_uint64_t	total_pages;
 	mtr_t		mtr;
 	ulint		size;
+	index_stats_t	result(index->n_uniq);
 	DBUG_ENTER("dict_stats_analyze_index");
 
 	DBUG_PRINT("info", ("index: %s, online status: %d", index->name(),
 			    dict_index_get_online_status(index)));
 
+	ut_ad(!mutex_own(&dict_sys->mutex)); // because this function is slow
+	ut_ad(index->table->get_ref_count());
+
 	/* Disable update statistic for Rtree */
 	if (dict_index_is_spatial(index)) {
-		DBUG_VOID_RETURN;
+		DBUG_RETURN(result);
 	}
 
 	DEBUG_PRINTF("  %s(index=%s)\n", __func__, index->name());
 
-	dict_stats_empty_index(index, false);
-
 	mtr_start(&mtr);
 
 	mtr_s_lock(dict_index_get_lock(index), &mtr);
@@ -1923,7 +1949,7 @@ dict_stats_analyze_index(
 	size = btr_get_size(index, BTR_TOTAL_SIZE, &mtr);
 
 	if (size != ULINT_UNDEFINED) {
-		index->stat_index_size = size;
+		result.index_size = size;
 		size = btr_get_size(index, BTR_N_LEAF_PAGES, &mtr);
 	}
 
@@ -1933,13 +1959,13 @@ dict_stats_analyze_index(
 	switch (size) {
 	case ULINT_UNDEFINED:
 		dict_stats_assert_initialized_index(index);
-		DBUG_VOID_RETURN;
+		DBUG_RETURN(result);
 	case 0:
 		/* The root node of the tree is a leaf */
 		size = 1;
 	}
 
-	index->stat_n_leaf_pages = size;
+	result.n_leaf_pages = size;
 
 	mtr_start(&mtr);
 
@@ -1980,14 +2006,18 @@ dict_stats_analyze_index(
 					       NULL /* boundaries not needed */,
 					       &mtr);
 
+		mtr_commit(&mtr);
+
+		mutex_enter(&dict_sys->mutex);
 		for (ulint i = 0; i < n_uniq; i++) {
-			index->stat_n_sample_sizes[i] = total_pages;
+			result.stats[i].n_diff_key_vals = index->stat_n_diff_key_vals[i];
+			result.stats[i].n_sample_sizes = total_pages;
+			result.stats[i].n_non_null_key_vals = index->stat_n_non_null_key_vals[i];
 		}
+		result.n_leaf_pages = index->stat_n_leaf_pages;
+		mutex_exit(&dict_sys->mutex);
 
-		mtr_commit(&mtr);
-
-		dict_stats_assert_initialized_index(index);
-		DBUG_VOID_RETURN;
+		DBUG_RETURN(result);
 	}
 
 	/* For each level that is being scanned in the btree, this contains the
@@ -2179,13 +2209,12 @@ found_level:
 	/* n_prefix == 0 means that the above loop did not end up prematurely
 	due to tree being changed and so n_diff_data[] is set up. */
 	if (n_prefix == 0) {
-		dict_stats_index_set_n_diff(n_diff_data, index);
+		dict_stats_index_set_n_diff(n_diff_data, result);
 	}
 
 	UT_DELETE_ARRAY(n_diff_data);
 
-	dict_stats_assert_initialized_index(index);
-	DBUG_VOID_RETURN;
+	DBUG_RETURN(result);
 }
 
 /*********************************************************************//**
@@ -2203,7 +2232,7 @@ dict_stats_update_persistent(
 
 	DEBUG_PRINTF("%s(table=%s)\n", __func__, table->name);
 
-	dict_table_stats_lock(table, RW_X_LATCH);
+	DEBUG_SYNC_C("dict_stats_update_persistent");
 
 	/* analyze the clustered index first */
 
@@ -2214,7 +2243,6 @@ dict_stats_update_persistent(
 	    || (index->type | DICT_UNIQUE) != (DICT_CLUSTERED | DICT_UNIQUE)) {
 
 		/* Table definition is corrupt */
-		dict_table_stats_unlock(table, RW_X_LATCH);
 		dict_stats_empty_table(table, true);
 
 		return(DB_CORRUPTION);
@@ -2222,7 +2250,16 @@ dict_stats_update_persistent(
 
 	ut_ad(!dict_index_is_ibuf(index));
 
-	dict_stats_analyze_index(index);
+	index_stats_t stats = dict_stats_analyze_index(index);
+
+	mutex_enter(&dict_sys->mutex);
+	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;
+	}
 
 	ulint	n_unique = dict_index_get_n_unique(index);
 
@@ -2240,7 +2277,7 @@ dict_stats_update_persistent(
 
 		ut_ad(!dict_index_is_ibuf(index));
 
-		if (index->type & DICT_FTS || dict_index_is_spatial(index)) {
+		if (index->type & (DICT_FTS | DICT_SPATIAL)) {
 			continue;
 		}
 
@@ -2251,7 +2288,20 @@ dict_stats_update_persistent(
 		}
 
 		if (!(table->stats_bg_flag & BG_STAT_SHOULD_QUIT)) {
-			dict_stats_analyze_index(index);
+			mutex_exit(&dict_sys->mutex);
+			stats = dict_stats_analyze_index(index);
+			mutex_enter(&dict_sys->mutex);
+
+			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
@@ -2266,7 +2316,7 @@ dict_stats_update_persistent(
 
 	dict_stats_assert_initialized(table);
 
-	dict_table_stats_unlock(table, RW_X_LATCH);
+	mutex_exit(&dict_sys->mutex);
 
 	return(DB_SUCCESS);
 }
@@ -3094,11 +3144,22 @@ dict_stats_update_for_index(
 	if (dict_stats_is_persistent_enabled(index->table)) {
 
 		if (dict_stats_persistent_storage_check(false)) {
-			dict_table_stats_lock(index->table, RW_X_LATCH);
-			dict_stats_analyze_index(index);
+			index_stats_t stats = dict_stats_analyze_index(index);
+			mutex_enter(&dict_sys->mutex);
+			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->table->stat_sum_of_other_index_sizes
 				+= index->stat_index_size;
-			dict_table_stats_unlock(index->table, RW_X_LATCH);
+			mutex_exit(&dict_sys->mutex);
+
 			dict_stats_save(index->table, &index->id);
 			DBUG_VOID_RETURN;
 		}
@@ -3119,9 +3180,7 @@ dict_stats_update_for_index(
 		}
 	}
 
-	dict_table_stats_lock(index->table, RW_X_LATCH);
 	dict_stats_update_transient_for_index(index);
-	dict_table_stats_unlock(index->table, RW_X_LATCH);
 
 	DBUG_VOID_RETURN;
 }
@@ -3275,7 +3334,7 @@ dict_stats_update(
 		switch (err) {
 		case DB_SUCCESS:
 
-			dict_table_stats_lock(table, RW_X_LATCH);
+			mutex_enter(&dict_sys->mutex);
 
 			/* Pass reset_ignored_indexes=true as parameter
 			to dict_stats_copy. This will cause statictics
@@ -3284,7 +3343,7 @@ dict_stats_update(
 
 			dict_stats_assert_initialized(table);
 
-			dict_table_stats_unlock(table, RW_X_LATCH);
+			mutex_exit(&dict_sys->mutex);
 
 			dict_stats_table_clone_free(t);
 
@@ -3338,13 +3397,8 @@ dict_stats_update(
 	}
 
 transient:
-
-	dict_table_stats_lock(table, RW_X_LATCH);
-
 	dict_stats_update_transient(table);
 
-	dict_table_stats_unlock(table, RW_X_LATCH);
-
 	return(DB_SUCCESS);
 }