diff options
author | Krunal Bauskar <mysqlonarm@gmail.com> | 2021-03-03 20:07:13 +0800 |
---|---|---|
committer | Marko Mäkelä <marko.makela@mariadb.com> | 2021-03-05 14:26:14 +0200 |
commit | 33cf577ad86f3a7dd6ff86fee4ecd9c535161f02 (patch) | |
tree | 7d16a12d4c6c09ab6d630f35f7541cc642119d68 | |
parent | 7947c549aac03980a884fbc7a1fadfe8efe1a442 (diff) | |
download | mariadb-git-33cf577ad86f3a7dd6ff86fee4ecd9c535161f02.tar.gz |
MDEV-25029: Reduce dict_sys mutex contention for read-only workload
In theory, read-only workload shouldn't have anything to do with
dict_sys mutex. dict_sys mutex is meant to protect database metadata.
But then why does dict_sys mutex shows up as part of a read-only workload?
This workload needs to fetch stats for query processing and while reading
these stats dict_sys mutex is taken.
Is this really needed?
No. For the traditional reasons, it was the default global mutex used.
Based on 10.6 changes, flow can now use table->lock_mutex to protect
update/access of these stats. table mutexes being table specific
global contention arising out of dict_sys is reduced.
Thanks to Marko Makela for his early suggestion around proposed alternative
and review of the draft patch.
-rw-r--r-- | storage/innobase/dict/dict0dict.cc | 2 | ||||
-rw-r--r-- | storage/innobase/dict/dict0stats.cc | 92 | ||||
-rw-r--r-- | storage/innobase/handler/ha_innodb.cc | 11 | ||||
-rw-r--r-- | storage/innobase/handler/i_s.cc | 9 | ||||
-rw-r--r-- | storage/innobase/include/dict0mem.h | 13 | ||||
-rw-r--r-- | storage/innobase/include/dict0stats.ic | 4 |
6 files changed, 79 insertions, 52 deletions
diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc index 2d86bdd26c2..046490e713c 100644 --- a/storage/innobase/dict/dict0dict.cc +++ b/storage/innobase/dict/dict0dict.cc @@ -334,7 +334,9 @@ dict_table_close( if (last_handle && strchr(table->name.m_name, '/') != NULL && dict_stats_is_persistent_enabled(table)) { + table->stats_mutex_lock(); dict_stats_deinit(table); + table->stats_mutex_unlock(); } MONITOR_DEC(MONITOR_TABLE_REFERENCE); diff --git a/storage/innobase/dict/dict0stats.cc b/storage/innobase/dict/dict0stats.cc index d8ab4a462a6..f302bbd4ddd 100644 --- a/storage/innobase/dict/dict0stats.cc +++ b/storage/innobase/dict/dict0stats.cc @@ -406,7 +406,9 @@ dict_stats_table_clone_create( dict_table_t* t; - t = (dict_table_t*) mem_heap_alloc(heap, sizeof(*t)); + t = (dict_table_t*) mem_heap_zalloc(heap, sizeof(*t)); + + t->stats_mutex_init(); MEM_CHECK_DEFINED(&table->id, sizeof(table->id)); t->id = table->id; @@ -434,7 +436,7 @@ dict_stats_table_clone_create( dict_index_t* idx; - idx = (dict_index_t*) mem_heap_alloc(heap, sizeof(*idx)); + idx = (dict_index_t*) mem_heap_zalloc(heap, sizeof(*idx)); MEM_CHECK_DEFINED(&index->id, sizeof(index->id)); idx->id = index->id; @@ -452,7 +454,7 @@ dict_stats_table_clone_create( idx->n_uniq = index->n_uniq; - idx->fields = (dict_field_t*) mem_heap_alloc( + idx->fields = (dict_field_t*) mem_heap_zalloc( heap, idx->n_uniq * sizeof(idx->fields[0])); for (ulint i = 0; i < idx->n_uniq; i++) { @@ -463,15 +465,15 @@ dict_stats_table_clone_create( /* hook idx into t->indexes */ UT_LIST_ADD_LAST(t->indexes, idx); - idx->stat_n_diff_key_vals = (ib_uint64_t*) mem_heap_alloc( + idx->stat_n_diff_key_vals = (ib_uint64_t*) mem_heap_zalloc( heap, idx->n_uniq * sizeof(idx->stat_n_diff_key_vals[0])); - idx->stat_n_sample_sizes = (ib_uint64_t*) mem_heap_alloc( + idx->stat_n_sample_sizes = (ib_uint64_t*) mem_heap_zalloc( heap, idx->n_uniq * sizeof(idx->stat_n_sample_sizes[0])); - idx->stat_n_non_null_key_vals = (ib_uint64_t*) mem_heap_alloc( + idx->stat_n_non_null_key_vals = (ib_uint64_t*) mem_heap_zalloc( heap, idx->n_uniq * sizeof(idx->stat_n_non_null_key_vals[0])); ut_d(idx->magic_n = DICT_INDEX_MAGIC_N); @@ -494,6 +496,7 @@ dict_stats_table_clone_free( /*========================*/ dict_table_t* t) /*!< in: dummy table object to free */ { + t->stats_mutex_destroy(); mem_heap_free(t->heap); } @@ -510,7 +513,7 @@ dict_stats_empty_index( { ut_ad(!(index->type & DICT_FTS)); ut_ad(!dict_index_is_ibuf(index)); - dict_sys.assert_locked(); + ut_ad(index->table->stats_mutex_is_owner()); ulint n_uniq = index->n_uniq; @@ -540,7 +543,9 @@ dict_stats_empty_table( bool empty_defrag_stats) /*!< in: whether to empty defrag stats */ { - dict_sys.mutex_lock(); + /* Initialize table/index level stats is now protected by + table level lock_mutex.*/ + table->stats_mutex_lock(); /* Zero the stats members */ table->stat_n_rows = 0; @@ -566,7 +571,7 @@ dict_stats_empty_table( } table->stat_initialized = TRUE; - dict_sys.mutex_unlock(); + table->stats_mutex_unlock(); } /*********************************************************************//** @@ -665,7 +670,8 @@ dict_stats_copy( to have the same statistics as if the table was empty */ { - dict_sys.assert_locked(); + ut_ad(src->stats_mutex_is_owner()); + ut_ad(dst->stats_mutex_is_owner()); dst->stats_last_recalc = src->stats_last_recalc; dst->stat_n_rows = src->stat_n_rows; @@ -790,8 +796,14 @@ dict_stats_snapshot_create( t = dict_stats_table_clone_create(table); + table->stats_mutex_lock(); + ut_d(t->stats_mutex_lock()); + dict_stats_copy(t, table, false); + ut_d(t->stats_mutex_unlock()); + table->stats_mutex_unlock(); + t->stat_persistent = table->stat_persistent; t->stats_auto_recalc = table->stats_auto_recalc; t->stats_sample_pages = table->stats_sample_pages; @@ -836,14 +848,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. */ - dict_sys.mutex_lock(); + index->table->stats_mutex_lock(); dict_stats_empty_index(index, false); - dict_sys.mutex_unlock(); + index->table->stats_mutex_unlock(); #if defined UNIV_DEBUG || defined UNIV_IBUF_DEBUG } else if (ibuf_debug && !dict_index_is_clust(index)) { - dict_sys.mutex_lock(); + index->table->stats_mutex_lock(); dict_stats_empty_index(index, false); - dict_sys.mutex_unlock(); + index->table->stats_mutex_unlock(); #endif /* UNIV_DEBUG || UNIV_IBUF_DEBUG */ } else { mtr_t mtr; @@ -864,9 +876,9 @@ dict_stats_update_transient_for_index( switch (size) { case ULINT_UNDEFINED: - dict_sys.mutex_lock(); + index->table->stats_mutex_lock(); dict_stats_empty_index(index, false); - dict_sys.mutex_unlock(); + index->table->stats_mutex_unlock(); return; case 0: /* The root node of the tree is a leaf */ @@ -883,7 +895,7 @@ dict_stats_update_transient_for_index( index); if (!stats.empty()) { - dict_sys.mutex_lock(); + index->table->stats_mutex_lock(); for (size_t i = 0; i < stats.size(); ++i) { index->stat_n_diff_key_vals[i] = stats[i].n_diff_key_vals; @@ -892,7 +904,7 @@ dict_stats_update_transient_for_index( index->stat_n_non_null_key_vals[i] = stats[i].n_non_null_key_vals; } - dict_sys.mutex_unlock(); + index->table->stats_mutex_unlock(); } } } @@ -910,7 +922,7 @@ dict_stats_update_transient( /*========================*/ dict_table_t* table) /*!< in/out: table */ { - dict_sys.assert_not_locked(); + ut_ad(!table->stats_mutex_is_owner()); dict_index_t* index; ulint sum_of_index_sizes = 0; @@ -943,9 +955,9 @@ dict_stats_update_transient( if (dict_stats_should_ignore_index(index) || !index->is_readable()) { - dict_sys.mutex_lock(); + index->table->stats_mutex_lock(); dict_stats_empty_index(index, false); - dict_sys.mutex_unlock(); + index->table->stats_mutex_unlock(); continue; } @@ -954,7 +966,7 @@ dict_stats_update_transient( sum_of_index_sizes += index->stat_index_size; } - dict_sys.mutex_lock(); + table->stats_mutex_lock(); index = dict_table_get_first_index(table); @@ -972,7 +984,7 @@ dict_stats_update_transient( table->stat_initialized = TRUE; - dict_sys.mutex_unlock(); + table->stats_mutex_unlock(); } /* @{ Pseudo code about the relation between the following functions @@ -1930,7 +1942,7 @@ static index_stats_t dict_stats_analyze_index(dict_index_t* index) DBUG_PRINT("info", ("index: %s, online status: %d", index->name(), dict_index_get_online_status(index))); - dict_sys.assert_not_locked(); // this function is slow + ut_ad(!index->table->stats_mutex_is_owner()); ut_ad(index->table->get_ref_count()); /* Disable update statistic for Rtree */ @@ -2002,14 +2014,14 @@ static index_stats_t dict_stats_analyze_index(dict_index_t* index) mtr.commit(); - dict_sys.mutex_lock(); + index->table->stats_mutex_lock(); for (ulint i = 0; i < n_uniq; i++) { 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; - dict_sys.mutex_unlock(); + index->table->stats_mutex_unlock(); DBUG_RETURN(result); } @@ -2243,13 +2255,13 @@ dict_stats_update_persistent( } ut_ad(!dict_index_is_ibuf(index)); - dict_sys.mutex_lock(); + table->stats_mutex_lock(); dict_stats_empty_index(index, false); - dict_sys.mutex_unlock(); + table->stats_mutex_unlock(); index_stats_t stats = dict_stats_analyze_index(index); - dict_sys.mutex_lock(); + 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) { @@ -2285,9 +2297,9 @@ dict_stats_update_persistent( } if (!(table->stats_bg_flag & BG_STAT_SHOULD_QUIT)) { - dict_sys.mutex_unlock(); + table->stats_mutex_unlock(); stats = dict_stats_analyze_index(index); - dict_sys.mutex_lock(); + table->stats_mutex_lock(); index->stat_index_size = stats.index_size; index->stat_n_leaf_pages = stats.n_leaf_pages; @@ -2313,7 +2325,7 @@ dict_stats_update_persistent( dict_stats_assert_initialized(table); - dict_sys.mutex_unlock(); + table->stats_mutex_unlock(); return(DB_SUCCESS); } @@ -3123,13 +3135,11 @@ dict_stats_update_for_index( { DBUG_ENTER("dict_stats_update_for_index"); - dict_sys.assert_not_locked(); - if (dict_stats_is_persistent_enabled(index->table)) { if (dict_stats_persistent_storage_check(false)) { index_stats_t stats = dict_stats_analyze_index(index); - dict_sys.mutex_lock(); + 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) { @@ -3142,7 +3152,7 @@ dict_stats_update_for_index( } index->table->stat_sum_of_other_index_sizes += index->stat_index_size; - dict_sys.mutex_unlock(); + index->table->stats_mutex_unlock(); dict_stats_save(index->table, &index->id); DBUG_VOID_RETURN; @@ -3183,7 +3193,7 @@ dict_stats_update( the persistent statistics storage */ { - dict_sys.assert_not_locked(); + ut_ad(!table->stats_mutex_is_owner()); if (!table->is_readable()) { return (dict_stats_report_error(table)); @@ -3318,7 +3328,10 @@ dict_stats_update( switch (err) { case DB_SUCCESS: - dict_sys.mutex_lock(); + table->stats_mutex_lock(); + /* t is localized to this thread so no need to + take stats mutex lock (limiting it to debug only) */ + ut_d(t->stats_mutex_lock()); /* Pass reset_ignored_indexes=true as parameter to dict_stats_copy. This will cause statictics @@ -3327,7 +3340,8 @@ dict_stats_update( dict_stats_assert_initialized(table); - dict_sys.mutex_unlock(); + ut_d(t->stats_mutex_unlock()); + table->stats_mutex_unlock(); dict_stats_table_clone_free(t); diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 61559b23ee2..b57ef44fc39 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -14044,7 +14044,7 @@ ha_innobase::info_low( ulint stat_clustered_index_size; ulint stat_sum_of_other_index_sizes; - dict_sys.mutex_lock(); + ib_table->stats_mutex_lock(); ut_a(ib_table->stat_initialized); @@ -14056,7 +14056,7 @@ ha_innobase::info_low( stat_sum_of_other_index_sizes = ib_table->stat_sum_of_other_index_sizes; - dict_sys.mutex_unlock(); + ib_table->stats_mutex_unlock(); /* The MySQL optimizer seems to assume in a left join that n_rows @@ -14172,10 +14172,9 @@ ha_innobase::info_low( stats.create_time = (ulong) stat_info.ctime; } - struct Locking { - Locking() { dict_sys.mutex_lock(); } - ~Locking() { dict_sys.mutex_unlock(); } - } locking; + ib_table->stats_mutex_lock(); + auto _ = make_scope_exit([ib_table]() { + ib_table->stats_mutex_unlock(); }); ut_a(ib_table->stat_initialized); diff --git a/storage/innobase/handler/i_s.cc b/storage/innobase/handler/i_s.cc index 32c4379a3ca..e839f3ff1b8 100644 --- a/storage/innobase/handler/i_s.cc +++ b/storage/innobase/handler/i_s.cc @@ -56,6 +56,7 @@ Created July 18, 2007 Vasil Dimov #include "fil0fil.h" #include "fil0crypt.h" #include "dict0crea.h" +#include "scope.h" /** The latest successfully looked up innodb_fts_aux_table */ UNIV_INTERN table_id_t innodb_ft_aux_table_id; @@ -5021,11 +5022,9 @@ i_s_dict_fill_sys_tablestats( table->name.m_name)); { - struct Locking - { - Locking() { dict_sys.mutex_lock(); } - ~Locking() { dict_sys.mutex_unlock(); } - } locking; + table->stats_mutex_lock(); + auto _ = make_scope_exit([table]() { + table->stats_mutex_unlock(); }); OK(fields[SYS_TABLESTATS_INIT]->store(table->stat_initialized, true)); diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h index d0d82bf7e56..2d87d7f689f 100644 --- a/storage/innobase/include/dict0mem.h +++ b/storage/innobase/include/dict0mem.h @@ -1962,6 +1962,9 @@ struct dict_table_t { /** @return whether the current thread holds the lock_mutex */ bool lock_mutex_is_owner() const { return lock_mutex_owner == os_thread_get_curr_id(); } + /** @return whether the current thread holds the stats_mutex (lock_mutex) */ + bool stats_mutex_is_owner() const + { return lock_mutex_owner == os_thread_get_curr_id(); } #endif /* UNIV_DEBUG */ void lock_mutex_init() { lock_mutex.init(); } void lock_mutex_destroy() { lock_mutex.destroy(); } @@ -1986,6 +1989,16 @@ struct dict_table_t { ut_ad(lock_mutex_owner.exchange(0) == os_thread_get_curr_id()); lock_mutex.wr_unlock(); } + + /* stats mutex lock currently defaults to lock_mutex but in the future, + there could be a use-case to have separate mutex for stats. + extra indirection (through inline so no performance hit) should + help simplify code and increase long-term maintainability */ + void stats_mutex_init() { lock_mutex_init(); } + void stats_mutex_destroy() { lock_mutex_destroy(); } + void stats_mutex_lock() { lock_mutex_lock(); } + void stats_mutex_unlock() { lock_mutex_unlock(); } + private: /** Initialize instant->field_map. @param[in] table table definition to copy from */ diff --git a/storage/innobase/include/dict0stats.ic b/storage/innobase/include/dict0stats.ic index cfc3c0e7e21..68592404253 100644 --- a/storage/innobase/include/dict0stats.ic +++ b/storage/innobase/include/dict0stats.ic @@ -148,7 +148,7 @@ dict_stats_init( /*============*/ dict_table_t* table) /*!< in/out: table */ { - dict_sys.assert_not_locked(); + ut_ad(!table->stats_mutex_is_owner()); if (table->stat_initialized) { return; @@ -174,7 +174,7 @@ dict_stats_deinit( /*==============*/ dict_table_t* table) /*!< in/out: table */ { - dict_sys.assert_locked(); + ut_ad(table->stats_mutex_is_owner()); ut_a(table->get_ref_count() == 0); |