From d1ecf5cc5f5e452c6c4d368b74c83b30f592526d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Fri, 28 Jun 2024 15:57:07 +0300 Subject: [PATCH] MDEV-32176 Contention in ha_innobase::info_low() During a Sysbench oltp_point_select workload with 1 table and 400 concurrent connections, a bottleneck on dict_table_t::lock_mutex was observed in ha_innobase::info_low(). dict_table_t::lock_latch: Replaces lock_mutex. In ha_innobase::info_low() and several other places, we will acquire a shared dict_table_t::lock_latch or we may elide the latch if hardware memory transactions are available. innobase_build_v_templ(): Remove the parameter "bool locked", and require the caller to hold exclusive dict_table_t::lock_latch (instead of holding an exclusive dict_sys.latch). Tested by: Vladislav Vaintroub Reviewed by: Vladislav Vaintroub --- storage/innobase/handler/ha_innodb.cc | 77 ++++++++++++----------- storage/innobase/handler/ha_innodb.h | 6 +- storage/innobase/handler/handler0alter.cc | 22 ++++--- storage/innobase/handler/i_s.cc | 4 +- storage/innobase/include/dict0dict.h | 53 ++++++++-------- storage/innobase/include/dict0dict.inl | 50 --------------- storage/innobase/include/dict0mem.h | 71 ++++++++++----------- storage/innobase/lock/lock0lock.cc | 6 +- 8 files changed, 119 insertions(+), 170 deletions(-) diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index b0f69c06d0c..084b7fa350f 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -3267,9 +3267,9 @@ static bool innobase_query_caching_table_check_low( } #endif - table->lock_mutex_lock(); + table->lock_shared_lock(); auto len= UT_LIST_GET_LEN(table->locks); - table->lock_mutex_unlock(); + table->lock_shared_unlock(); return len == 0; } @@ -5596,15 +5596,13 @@ is done when the table first opened. @param[in] ib_table InnoDB dict_table_t @param[in,out] s_templ InnoDB template structure @param[in] add_v new virtual columns added along with - add index call -@param[in] locked true if dict_sys.latch is held */ + add index call */ void innobase_build_v_templ( const TABLE* table, const dict_table_t* ib_table, dict_vcol_templ_t* s_templ, - const dict_add_v_col_t* add_v, - bool locked) + const dict_add_v_col_t* add_v) { ulint ncol = unsigned(ib_table->n_cols) - DATA_N_SYS_COLS; ulint n_v_col = ib_table->n_v_cols; @@ -5612,6 +5610,7 @@ innobase_build_v_templ( DBUG_ENTER("innobase_build_v_templ"); ut_ad(ncol < REC_MAX_N_FIELDS); + ut_ad(ib_table->lock_mutex_is_owner()); if (add_v != NULL) { n_v_col += add_v->n_v_col; @@ -5619,20 +5618,7 @@ innobase_build_v_templ( ut_ad(n_v_col > 0); - if (!locked) { - dict_sys.lock(SRW_LOCK_CALL); - } - -#if 0 - /* This does not (need to) hold for ctx->new_table in - alter_rebuild_apply_log() */ - ut_ad(dict_sys.locked()); -#endif - if (s_templ->vtempl) { - if (!locked) { - dict_sys.unlock(); - } DBUG_VOID_RETURN; } @@ -5736,12 +5722,9 @@ innobase_build_v_templ( j++; } - if (!locked) { - dict_sys.unlock(); - } - s_templ->db_name = table->s->db.str; s_templ->tb_name = table->s->table_name.str; + DBUG_VOID_RETURN; } @@ -6019,15 +6002,15 @@ ha_innobase::open(const char* name, int, uint) key_used_on_scan = m_primary_key; if (ib_table->n_v_cols) { - dict_sys.lock(SRW_LOCK_CALL); + ib_table->lock_mutex_lock(); + if (ib_table->vc_templ == NULL) { ib_table->vc_templ = UT_NEW_NOKEY(dict_vcol_templ_t()); innobase_build_v_templ( - table, ib_table, ib_table->vc_templ, NULL, - true); + table, ib_table, ib_table->vc_templ); } - dict_sys.unlock(); + ib_table->lock_mutex_unlock(); } if (!check_index_consistency(table, ib_table)) { @@ -14731,7 +14714,7 @@ fsp_get_available_space_in_free_extents(const fil_space_t& space) Returns statistics information of the table to the MySQL interpreter, in various fields of the handle object. @return HA_ERR_* error code or 0 */ - +TRANSACTIONAL_TARGET int ha_innobase::info_low( /*==================*/ @@ -14812,19 +14795,37 @@ ha_innobase::info_low( ulint stat_clustered_index_size; ulint stat_sum_of_other_index_sizes; - ib_table->stats_mutex_lock(); - ut_a(ib_table->stat_initialized); - n_rows = ib_table->stat_n_rows; +#if !defined NO_ELISION && !defined SUX_LOCK_GENERIC + if (xbegin()) { + if (ib_table->stats_mutex_is_locked()) + xabort(); - stat_clustered_index_size - = ib_table->stat_clustered_index_size; + n_rows = ib_table->stat_n_rows; - stat_sum_of_other_index_sizes - = ib_table->stat_sum_of_other_index_sizes; + stat_clustered_index_size + = ib_table->stat_clustered_index_size; - ib_table->stats_mutex_unlock(); + stat_sum_of_other_index_sizes + = ib_table->stat_sum_of_other_index_sizes; + + xend(); + } else +#endif + { + ib_table->stats_shared_lock(); + + n_rows = ib_table->stat_n_rows; + + stat_clustered_index_size + = ib_table->stat_clustered_index_size; + + stat_sum_of_other_index_sizes + = ib_table->stat_sum_of_other_index_sizes; + + ib_table->stats_shared_unlock(); + } /* The MySQL optimizer seems to assume in a left join that n_rows @@ -14944,9 +14945,9 @@ ha_innobase::info_low( stats.create_time = (ulong) stat_info.ctime; } - ib_table->stats_mutex_lock(); + ib_table->stats_shared_lock(); auto _ = make_scope_exit([ib_table]() { - ib_table->stats_mutex_unlock(); }); + ib_table->stats_shared_unlock(); }); ut_a(ib_table->stat_initialized); diff --git a/storage/innobase/handler/ha_innodb.h b/storage/innobase/handler/ha_innodb.h index 50ac423f3a5..40396564589 100644 --- a/storage/innobase/handler/ha_innodb.h +++ b/storage/innobase/handler/ha_innodb.h @@ -880,15 +880,13 @@ innodb_rec_per_key( @param[in] ib_table InnoDB dict_table_t @param[in,out] s_templ InnoDB template structure @param[in] add_v new virtual columns added along with - add index call -@param[in] locked true if innobase_share_mutex is held */ + add index call */ void innobase_build_v_templ( const TABLE* table, const dict_table_t* ib_table, dict_vcol_templ_t* s_templ, - const dict_add_v_col_t* add_v, - bool locked); + const dict_add_v_col_t* add_v = nullptr); /** callback used by MySQL server layer to initialized the table virtual columns' template diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc index 15d616ad640..09f541a75d9 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -6521,16 +6521,16 @@ acquire_lock: acquiring an InnoDB table lock even for online operation, to ensure that the rollback of recovered transactions will not run concurrently with online ADD INDEX. */ - user_table->lock_mutex_lock(); + user_table->lock_shared_lock(); for (lock_t *lock = UT_LIST_GET_FIRST(user_table->locks); lock; lock = UT_LIST_GET_NEXT(un_member.tab_lock.locks, lock)) { if (lock->trx->is_recovered) { - user_table->lock_mutex_unlock(); + user_table->lock_shared_unlock(); goto acquire_lock; } } - user_table->lock_mutex_unlock(); + user_table->lock_shared_unlock(); } if (fts_exist) { @@ -8781,10 +8781,11 @@ ok_exit: } s_templ = UT_NEW_NOKEY(dict_vcol_templ_t()); + ctx->new_table->lock_mutex_lock(); innobase_build_v_templ( - altered_table, ctx->new_table, s_templ, NULL, false); - + altered_table, ctx->new_table, s_templ); ctx->new_table->vc_templ = s_templ; + ctx->new_table->lock_mutex_unlock(); } else if (ctx->num_to_add_vcol > 0 && ctx->num_to_drop_vcol == 0) { /* if there is ongoing drop virtual column, then we disallow inplace add index on newly added virtual column, so it does @@ -8799,10 +8800,12 @@ ok_exit: add_v->v_col = ctx->add_vcol; add_v->v_col_name = ctx->add_vcol_name; + ctx->new_table->lock_mutex_lock(); innobase_build_v_templ( - altered_table, ctx->new_table, s_templ, add_v, false); + altered_table, ctx->new_table, s_templ, add_v); old_templ = ctx->new_table->vc_templ; ctx->new_table->vc_templ = s_templ; + ctx->new_table->lock_mutex_unlock(); } /* Drop virtual column without rebuild will keep dict table @@ -11075,11 +11078,10 @@ static bool alter_rebuild_apply_log( if (ctx->new_table->n_v_cols > 0) { s_templ = UT_NEW_NOKEY( dict_vcol_templ_t()); - s_templ->vtempl = NULL; - - innobase_build_v_templ(altered_table, ctx->new_table, s_templ, - NULL, true); + ctx->new_table->lock_mutex_lock(); + innobase_build_v_templ(altered_table, ctx->new_table, s_templ); ctx->new_table->vc_templ = s_templ; + ctx->new_table->lock_mutex_unlock(); } dberr_t error = row_log_table_apply( diff --git a/storage/innobase/handler/i_s.cc b/storage/innobase/handler/i_s.cc index 0e520a174da..604ce85e93e 100644 --- a/storage/innobase/handler/i_s.cc +++ b/storage/innobase/handler/i_s.cc @@ -4986,9 +4986,9 @@ i_s_dict_fill_sys_tablestats(THD* thd, dict_table_t *table, Field **fields= table_to_fill->field; { - table->stats_mutex_lock(); + table->stats_shared_lock(); auto _ = make_scope_exit([table]() { - table->stats_mutex_unlock(); dict_sys.unlock(); }); + table->stats_shared_unlock(); dict_sys.unlock(); }); OK(fields[SYS_TABLESTATS_ID]->store(longlong(table->id), TRUE)); diff --git a/storage/innobase/include/dict0dict.h b/storage/innobase/include/dict0dict.h index 6f9a0efe02c..8394d746bc4 100644 --- a/storage/innobase/include/dict0dict.h +++ b/storage/innobase/include/dict0dict.h @@ -700,35 +700,32 @@ bool dict_table_has_indexed_v_cols( const dict_table_t* table); -/********************************************************************//** -Gets the approximately estimated number of rows in the table. +TPOOL_SUPPRESS_TSAN +/** Get the estimated number of rows in the table. @return estimated number of rows */ -UNIV_INLINE -ib_uint64_t -dict_table_get_n_rows( -/*==================*/ - const dict_table_t* table) /*!< in: table */ - MY_ATTRIBUTE((warn_unused_result)); -/********************************************************************//** -Increment the number of rows in the table by one. -Notice that this operation is not protected by any latch, the number is -approximate. */ -UNIV_INLINE -void -dict_table_n_rows_inc( -/*==================*/ - dict_table_t* table) /*!< in/out: table */ - MY_ATTRIBUTE((nonnull)); -/********************************************************************//** -Decrement the number of rows in the table by one. -Notice that this operation is not protected by any latch, the number is -approximate. */ -UNIV_INLINE -void -dict_table_n_rows_dec( -/*==================*/ - dict_table_t* table) /*!< in/out: table */ - MY_ATTRIBUTE((nonnull)); +inline uint64_t dict_table_get_n_rows(const dict_table_t *table) +{ + ut_ad(table->stat_initialized); + return table->stat_n_rows; +} + +/** Increment the number of rows in the table by one. +Note that this operation is not protected by any latch, +the number is approximate. */ +TPOOL_SUPPRESS_TSAN inline void dict_table_n_rows_inc(dict_table_t *table) +{ + if (auto n_rows= table->stat_n_rows + 1) + table->stat_n_rows= n_rows; +} + +/** Decrement the number of rows in the table by one. +Note that this operation is not protected by any latch, +the number is approximate. */ +TPOOL_SUPPRESS_TSAN inline void dict_table_n_rows_dec(dict_table_t *table) +{ + if (auto n_rows= table->stat_n_rows) + table->stat_n_rows= n_rows - 1; +} /** Get nth virtual column @param[in] table target table diff --git a/storage/innobase/include/dict0dict.inl b/storage/innobase/include/dict0dict.inl index a210c839020..5c760f1e89d 100644 --- a/storage/innobase/include/dict0dict.inl +++ b/storage/innobase/include/dict0dict.inl @@ -306,56 +306,6 @@ dict_table_has_indexed_v_cols( return(false); } -/********************************************************************//** -Gets the approximately estimated number of rows in the table. -@return estimated number of rows */ -UNIV_INLINE -ib_uint64_t -dict_table_get_n_rows( -/*==================*/ - const dict_table_t* table) /*!< in: table */ -{ - ut_ad(table->stat_initialized); - - return(table->stat_n_rows); -} - -/********************************************************************//** -Increment the number of rows in the table by one. -Notice that this operation is not protected by any latch, the number is -approximate. */ -UNIV_INLINE -void -dict_table_n_rows_inc( -/*==================*/ - dict_table_t* table) /*!< in/out: table */ -{ - if (table->stat_initialized) { - ib_uint64_t n_rows = table->stat_n_rows; - if (n_rows < 0xFFFFFFFFFFFFFFFFULL) { - table->stat_n_rows = n_rows + 1; - } - } -} - -/********************************************************************//** -Decrement the number of rows in the table by one. -Notice that this operation is not protected by any latch, the number is -approximate. */ -UNIV_INLINE -void -dict_table_n_rows_dec( -/*==================*/ - dict_table_t* table) /*!< in/out: table */ -{ - if (table->stat_initialized) { - ib_uint64_t n_rows = table->stat_n_rows; - if (n_rows > 0) { - table->stat_n_rows = n_rows - 1; - } - } -} - #ifdef UNIV_DEBUG /********************************************************************//** Gets the nth column of a table. diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h index f8b83919f83..751d7f1a9e4 100644 --- a/storage/innobase/include/dict0mem.h +++ b/storage/innobase/include/dict0mem.h @@ -2018,38 +2018,36 @@ struct dict_table_t { #ifdef UNIV_DEBUG /** @return whether the current thread holds the lock_mutex */ - bool lock_mutex_is_owner() const - { return lock_mutex_owner == pthread_self(); } + bool lock_mutex_is_owner() const { return lock_latch.have_wr(); } /** @return whether the current thread holds the stats_mutex (lock_mutex) */ - bool stats_mutex_is_owner() const - { return lock_mutex_owner == pthread_self(); } + bool stats_mutex_is_owner() const { return lock_latch.have_wr(); } #endif /* UNIV_DEBUG */ - void lock_mutex_init() { lock_mutex.init(); } - void lock_mutex_destroy() { lock_mutex.destroy(); } - /** Acquire lock_mutex */ - void lock_mutex_lock() + void lock_mutex_init() { - ut_ad(!lock_mutex_is_owner()); - lock_mutex.wr_lock(); - ut_ad(!lock_mutex_owner.exchange(pthread_self())); - } - /** Try to acquire lock_mutex */ - bool lock_mutex_trylock() - { - ut_ad(!lock_mutex_is_owner()); - bool acquired= lock_mutex.wr_lock_try(); - ut_ad(!acquired || !lock_mutex_owner.exchange(pthread_self())); - return acquired; - } - /** Release lock_mutex */ - void lock_mutex_unlock() - { - ut_ad(lock_mutex_owner.exchange(0) == pthread_self()); - lock_mutex.wr_unlock(); +#ifdef UNIV_DEBUG + lock_latch.SRW_LOCK_INIT(0); +#else + lock_latch.init(); +#endif } + void lock_mutex_destroy() { lock_latch.destroy(); } + /** Acquire exclusive lock_latch */ + void lock_mutex_lock() { lock_latch.wr_lock(ut_d(SRW_LOCK_CALL)); } + /** Try to acquire exclusive lock_latch */ + bool lock_mutex_trylock() { return lock_latch.wr_lock_try(); } + /** Release exclusive lock_latch */ + void lock_mutex_unlock() { lock_latch.wr_unlock(); } + /** Acquire shared lock_latch */ + void lock_shared_lock() { lock_latch.rd_lock(ut_d(SRW_LOCK_CALL)); } + /** Release shared lock_latch */ + void lock_shared_unlock() { lock_latch.rd_unlock(); } + #ifndef SUX_LOCK_GENERIC - /** @return whether the lock mutex is held by some thread */ - bool lock_mutex_is_locked() const noexcept { return lock_mutex.is_locked(); } + /** @return whether an exclusive lock_latch is held by some thread */ + bool lock_mutex_is_locked() const noexcept + { return lock_latch.is_write_locked(); } + bool stats_mutex_is_locked() const noexcept + { return lock_latch.is_write_locked(); } #endif /* stats mutex lock currently defaults to lock_mutex but in the future, @@ -2060,6 +2058,8 @@ struct dict_table_t { void stats_mutex_destroy() { lock_mutex_destroy(); } void stats_mutex_lock() { lock_mutex_lock(); } void stats_mutex_unlock() { lock_mutex_unlock(); } + void stats_shared_lock() { lock_shared_lock(); } + void stats_shared_unlock() { lock_shared_unlock(); } /** Rename the data file. @param new_name name of the table @@ -2342,18 +2342,19 @@ public: /** Mutex protecting autoinc and freed_indexes. */ srw_spin_mutex autoinc_mutex; private: - /** Mutex protecting locks on this table. */ - srw_spin_mutex lock_mutex; #ifdef UNIV_DEBUG - /** The owner of lock_mutex (0 if none) */ - Atomic_relaxed lock_mutex_owner{0}; + typedef srw_lock_debug lock_latch_type; +#else + typedef srw_spin_lock_low lock_latch_type; #endif + /** RW-lock protecting locks and statistics on this table */ + lock_latch_type lock_latch; public: /** Autoinc counter value to give to the next inserted row. */ uint64_t autoinc; /** The transaction that currently holds the the AUTOINC lock on this table. - Protected by lock_mutex. + Protected by lock_latch. The thread that is executing autoinc_trx may read this field without holding a latch, in row_lock_table_autoinc_for_mysql(). Only the autoinc_trx thread may clear this field; it cannot be @@ -2412,9 +2413,9 @@ public: /** Magic number. */ ulint magic_n; #endif /* UNIV_DEBUG */ - /** mysql_row_templ_t for base columns used for compute the virtual - columns */ - dict_vcol_templ_t* vc_templ; + /** mysql_row_templ_t for base columns used for compute the virtual + columns; protected by lock_latch */ + dict_vcol_templ_t *vc_templ; /* @return whether the table has any other transcation lock other than the given transaction */ diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index 3e3d573ba5b..bae24011fe4 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -6388,7 +6388,7 @@ resolve_table_lock: if (!table->lock_mutex_trylock()) { /* The correct latching order is: - lock_sys.latch, table->lock_mutex_lock(), lock_sys.wait_mutex. + lock_sys.latch, table->lock_latch, lock_sys.wait_mutex. Thus, we must release lock_sys.wait_mutex for a blocking wait. */ mysql_mutex_unlock(&lock_sys.wait_mutex); table->lock_mutex_lock(); @@ -6585,9 +6585,9 @@ bool lock_table_has_locks(dict_table_t *table) else #endif { - table->lock_mutex_lock(); + table->lock_shared_lock(); len= UT_LIST_GET_LEN(table->locks); - table->lock_mutex_unlock(); + table->lock_shared_unlock(); } if (len) return true;