From afc9d00c66db946c8240fe1fa6b345a3a8b6fec1 Mon Sep 17 00:00:00 2001 From: Eugene Kosov Date: Tue, 27 Oct 2020 12:24:55 +0300 Subject: MDEV-23991 dict_table_stats_lock() has unnecessarily long scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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ä --- storage/innobase/dict/dict0dict.cc | 50 --------- storage/innobase/dict/dict0mem.cc | 13 +-- storage/innobase/dict/dict0stats.cc | 210 ++++++++++++++++++++++-------------- 3 files changed, 133 insertions(+), 140 deletions(-) (limited to 'storage/innobase/dict') 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 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 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); } -- cgit v1.2.1