Hi! Review of: commit 5b0b5f529f760021e97bc20f33cb777733863bf2 Author: Marko Mäkelä <marko.makela@mariadb.com> Date: Fri Apr 11 14:43:07 2025 +0300 MDEV-19749 MDL scalability regression after backup locks diff --git a/sql/backup.cc b/sql/backup.cc index f634a11f867..a312904dc70 100644 --- a/sql/backup.cc +++ b/sql/backup.cc @@ -48,29 +48,49 @@ static const char *stage_names[]= TYPELIB backup_stage_names= { array_elements(stage_names)-1, "", stage_names, 0 }; -static MDL_ticket *backup_flush_ticket; +static MDL_ticket *backup_flush_ticket= 0; +std::atomic<uint> THD::backup_commit_lock_enabled; + static File volatile backup_log= -1; static int backup_log_error= 0; -static bool backup_start(THD *thd); -static bool backup_flush(THD *thd); -static bool backup_block_ddl(THD *thd); -static bool backup_block_commit(THD *thd); -static bool start_ddl_logging(); -static void stop_ddl_logging(); +static bool backup_start(THD *thd) noexcept; +static bool backup_flush(THD *thd) noexcept; +static bool backup_block_ddl(THD *thd) noexcept; +static bool backup_block_commit(THD *thd) noexcept; +static bool start_ddl_logging() noexcept; +static void stop_ddl_logging() noexcept; Please do not do the above not critical changes (adding noexcept) in this commit that has nothing to with the above code. Try to keep commits in active trees to minimal! If you want to do the above, do that in a different commit, preferably in 12.0 tree. We could do this file by file. -/** - Run next stage of backup -*/ -void backup_init() +/* Initialize backup variables */ + +void backup_init() noexcept { Remove noexcept - backup_flush_ticket= 0; + DBUG_ASSERT(backup_flush_ticket == 0); + DBUG_ASSERT(THD::backup_commit_lock_enabled == 0); Please reset all variables in init. This is good practice to as it ensures things works better with the embedded server! It also shows clearly which variables are used by backup. backup_log= -1; backup_log_error= 0; } -bool run_backup_stage(THD *thd, backup_stages stage) + +#ifndef DBUG_OFF +void backup_reset() noexcept +{ + /* + Ensure that disable_backup_commit_locks() has been called for all + calls to enable_backup_commit_locks(). + */ + DBUG_ASSERT(THD::backup_commit_lock_enabled == 0); + DBUG_ASSERT(backup_flush_ticket == 0); +} +#endif Remove DBUG_OFF and noexcept. It is good to to have a function to reset / check things at the end. The above code is also needed when compiling without DBUG but have DBUG_ASSERT enabled as printf, something we do support and can be useful for debugging things at customers. + +/** + Run next stage of backup +*/ + +bool run_backup_stage(THD *thd, backup_stages stage) noexcept Remove noexcept here and in other places for this patch. + +/** @return whether a thread is in "fast path" THD::protect_against_backup() */ Please split comments over many rows +static my_bool check_if_unblocked(THD *thd, void *) noexcept +{ + return thd->backup_commit_lock.load(std::memory_order_acquire) == + THD::IN_COMMIT_NO_LOCK; +} Why change the name of the function ? I think that check_if_thd_in_commit() is a more clear name. check_if_thd_is_excuting_commit would be even better. Remove also noexcept ! + +/* + Wait until all active commits are done and all server threads knows + that backup has started. + This will cause ha_commit_trans() to use MDL locks to enable protection + commits under FTWRL and BACKUP STAGE BLOCK_COMMIT +*/ + +bool THD::enable_backup_commit_locks() noexcept +{ + /* Wait for all THD to start taking backup MDL locks during commit */ + THD_STAGE_INFO(this, stage_backup_setup_mdl_locks); + + /* Notify THD::protect_against_backup() that we are interested in them. */ + backup_commit_lock_enabled.fetch_add(1, std::memory_order_release); + + /* + Ensure that all threads are doing commits with mdl backup locks active. + */ + while (server_threads.iterate(check_if_unblocked, static_cast<void*>(0))) + { + if (killed) + { + disable_backup_commit_locks(); + return 1; + } + my_sleep(100); + } + THD_STAGE_INFO(this, stage_backup_got_commit_lock); + return 0; +} + + +void THD::disable_backup_commit_locks() noexcept +{ + IF_DBUG(auto l=,) Please do not use auto variables ! I REALLY dislike them! + backup_commit_lock_enabled.fetch_sub(1, std::memory_order_relaxed); + DBUG_ASSERT(l); +} + + Please add back my comment: /* Mark that the THD is about to execute a commit. If backup is already running, the caller needs to take a mdl lock instead */ +ATTRIBUTE_COLD bool THD::protect_against_backup_slow() noexcept +{ + /* The slow path: First, update our state, then wait for mdl_backup. */ + DBUG_ASSERT(this == current_thd); Remove assert. not needed and possible not required. + + backup_commit_lock.store(IN_COMMIT_WITH_LOCK, std::memory_order_release); + if (likely(!mdl_context.acquire_lock + (&mdl_backup, variables.lock_wait_timeout))) Fix indentation: if (likely(!mdl_context.acquire_lock(&mdl_backup, variables.lock_wait_timeout))) + return false; + + backup_commit_lock.store(OUTSIDE_COMMIT, std::memory_order_release); + return true; +} cut> -void backup_set_alter_copy_lock(THD *thd, TABLE *table) +void backup_set_alter_copy_lock(THD *thd, TABLE *table) noexcept { - MDL_ticket *ticket= thd->mdl_backup_ticket; - - /* Ticket maybe NULL in case of LOCK TABLES or for temporary tables*/ - DBUG_ASSERT(ticket || thd->locked_tables_mode || - table->s->tmp_table != NO_TMP_TABLE); - if (ticket) + if (MDL_ticket *ticket= thd->mdl_backup_ticket) ticket->downgrade_lock(MDL_BACKUP_ALTER_COPY); + else + /* Ticket maybe NULL in case of LOCK TABLES or for temporary tables*/ + DBUG_ASSERT(thd->locked_tables_mode || + table->s->tmp_table != NO_TMP_TABLE); } Please revert code. Old one was easier to read. <cut> index 2e5c3a58ba2..bd235dcea22 100644 --- a/sql/backup.h +++ b/sql/backup.h Remove all noexcept @@ -35,13 +35,18 @@ struct backup_log_info { bool new_partitioned; }; -void backup_init(); -bool run_backup_stage(THD *thd, backup_stages stage); -bool backup_end(THD *thd); -void backup_set_alter_copy_lock(THD *thd, TABLE *altered_table); -bool backup_reset_alter_copy_lock(THD *thd); - -bool backup_lock(THD *thd, TABLE_LIST *table); -void backup_unlock(THD *thd); -void backup_log_ddl(const backup_log_info *info); +void backup_init() noexcept; +#ifndef DBUG_OFF +void backup_reset() noexcept; +#else +# define backup_reset() /* empty */ +#endif Remove the above, keep original code. (See comment later) +bool run_backup_stage(THD *thd, backup_stages stage) noexcept; +bool backup_end(THD *thd) noexcept; +void backup_set_alter_copy_lock(THD *thd, TABLE *altered_table) noexcept; +bool backup_reset_alter_copy_lock(THD *thd) noexcept; + +bool backup_lock(THD *thd, TABLE_LIST *table) noexcept; +void backup_unlock(THD *thd) noexcept; +void backup_log_ddl(const backup_log_info *info) noexcept; #endif /* BACKUP_INCLUDED */ diff --git a/sql/handler.cc b/sql/handler.cc index 0088b84a2ec..1ec60f0e811 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -1785,19 +1784,11 @@ int ha_commit_trans(THD *thd, bool all) We allow the owner of FTWRL to COMMIT; we assume that it knows what it does. */ - MDL_REQUEST_INIT(&mdl_backup, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, - MDL_EXPLICIT); - - if (!WSREP(thd)) + if (!WSREP(thd) && thd->protect_against_backup()) { - if (thd->mdl_context.acquire_lock(&mdl_backup, - thd->variables.lock_wait_timeout)) - { - my_error(ER_ERROR_DURING_COMMIT, MYF(0), 1); - ha_rollback_trans(thd, all); - DBUG_RETURN(1); - } - thd->backup_commit_lock= &mdl_backup; + my_error(ER_ERROR_DURING_COMMIT, MYF(0), 1); + ha_rollback_trans(thd, all); + DBUG_RETURN(1); } DEBUG_SYNC(thd, "ha_commit_trans_after_acquire_commit_lock"); } @@ -2047,17 +2038,14 @@ int ha_commit_trans(THD *thd, bool all) thd->rgi_slave->is_parallel_exec); } end: - if (mdl_backup.ticket) - { - /* - We do not always immediately release transactional locks - after ha_commit_trans() (see uses of ha_enable_transaction()), - thus we release the commit blocker lock as soon as it's - not needed. - */ - thd->mdl_context.release_lock(mdl_backup.ticket); - thd->backup_commit_lock= 0; - } + /* + We do not always immediately release transactional locks + after ha_commit_trans() (see uses of ha_enable_transaction()), + thus we release the commit blocker lock as soon as it's + not needed. + */ + thd->unprotect_against_backup(); + #ifdef WITH_WSREP if (wsrep_is_active(thd) && is_real_trans && !error && (rw_ha_count == 0 || all) && diff --git a/sql/lock.cc b/sql/lock.cc index ef8c2ba3c86..f32b1a57011 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -1125,6 +1125,7 @@ bool Global_read_lock::lock_global_read_lock(THD *thd) void Global_read_lock::unlock_global_read_lock(THD *thd) { + bool have_backup_commit_lock; DBUG_ENTER("unlock_global_read_lock"); DBUG_ASSERT(m_mdl_global_read_lock && m_state); @@ -1138,7 +1139,16 @@ void Global_read_lock::unlock_global_read_lock(THD *thd) } } + /* + The backup commit lock was only taken and hold in + make_global_read_lock_block_commit() if the current lock is + FTWRL2 + */ + have_backup_commit_lock= (m_mdl_global_read_lock->get_type() == + MDL_BACKUP_FTWRL2); thd->mdl_context.release_lock(m_mdl_global_read_lock); + if (have_backup_commit_lock) + thd->disable_backup_commit_locks(); #ifdef WITH_WSREP if (m_state == GRL_ACQUIRED_AND_BLOCKS_COMMIT && @@ -1199,10 +1209,16 @@ bool Global_read_lock::make_global_read_lock_block_commit(THD *thd) if (m_state != GRL_ACQUIRED) DBUG_RETURN(0); + if (thd->enable_backup_commit_locks()) + DBUG_RETURN(TRUE); + if (thd->mdl_context.upgrade_shared_lock(m_mdl_global_read_lock, MDL_BACKUP_FTWRL2, thd->variables.lock_wait_timeout)) + { + thd->disable_backup_commit_locks(); DBUG_RETURN(TRUE); + } m_state= GRL_ACQUIRED_AND_BLOCKS_COMMIT; diff --git a/sql/log.cc b/sql/log.cc index 2f9eab73812..b97d8af19e2 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -6951,20 +6951,12 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate) uint64 commit_id= 0; MDL_request mdl_request; DBUG_PRINT("info", ("direct is set")); - DBUG_ASSERT(!thd->backup_commit_lock); - MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, - MDL_EXPLICIT); - if (thd->mdl_context.acquire_lock(&mdl_request, - thd->variables.lock_wait_timeout)) + if (thd->protect_against_backup()) DBUG_RETURN(1); - thd->backup_commit_lock= &mdl_request; - if ((res= thd->wait_for_prior_commit())) { - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); - thd->backup_commit_lock= 0; + thd->unprotect_against_backup(); DBUG_RETURN(res); } file= &log_file; @@ -6982,9 +6974,7 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate) commit_id= entry->val_int(&null_value); }); res= write_gtid_event(thd, true, using_trans, commit_id); - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); - thd->backup_commit_lock= 0; + thd->unprotect_against_backup(); if (res) goto err; } @@ -8088,7 +8078,7 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) wait_for_commit *wfc; bool backup_lock_released= 0; int result= 0; - THD *thd= orig_entry->thd; + THD *const thd= orig_entry->thd; DBUG_ENTER("MYSQL_BIN_LOG::queue_for_group_commit"); DBUG_ASSERT(thd == current_thd); @@ -8100,7 +8090,7 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) another safe check under lock, to avoid the race where the other transaction wakes us up between the check and the wait. */ - wfc= orig_entry->thd->wait_for_commit_ptr; + wfc= thd->wait_for_commit_ptr; orig_entry->queued_by_other= false; if (wfc && wfc->waitee.load(std::memory_order_acquire)) { @@ -8129,12 +8119,14 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) yet have the MDL_BACKUP_COMMIT_LOCK) and any threads using BACKUP LOCK BLOCK_COMMIT. */ - if (thd->backup_commit_lock && thd->backup_commit_lock->ticket && - !backup_lock_released) + + if (!backup_lock_released && + thd->backup_commit_lock.load(std::memory_order_relaxed) == + THD::IN_COMMIT_WITH_LOCK) { - backup_lock_released= 1; - thd->mdl_context.release_lock(thd->backup_commit_lock->ticket); - thd->backup_commit_lock->ticket= 0; + backup_lock_released= true; + thd->mdl_context.release_lock(thd->mdl_backup.ticket); + thd->mdl_backup.ticket= nullptr; } /* @@ -8152,13 +8144,12 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) we have been woken up. */ wfc->opaque_pointer= orig_entry; - DEBUG_SYNC(orig_entry->thd, "group_commit_waiting_for_prior"); - orig_entry->thd->ENTER_COND(&wfc->COND_wait_commit, - &wfc->LOCK_wait_commit, - &stage_waiting_for_prior_transaction_to_commit, - &old_stage); + DEBUG_SYNC(thd, "group_commit_waiting_for_prior"); + thd->ENTER_COND(&wfc->COND_wait_commit, &wfc->LOCK_wait_commit, + &stage_waiting_for_prior_transaction_to_commit, + &old_stage); while ((loc_waitee= wfc->waitee.load(std::memory_order_relaxed)) && - !orig_entry->thd->check_killed(1)) + !thd->check_killed(1)) mysql_cond_wait(&wfc->COND_wait_commit, &wfc->LOCK_wait_commit); wfc->opaque_pointer= NULL; DBUG_PRINT("info", ("After waiting for prior commit, queued_by_other=%d", @@ -8189,19 +8180,19 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) */ wfc->waitee.store(NULL, std::memory_order_relaxed); - orig_entry->thd->EXIT_COND(&old_stage); + thd->EXIT_COND(&old_stage); /* Interrupted by kill. */ - DEBUG_SYNC(orig_entry->thd, "group_commit_waiting_for_prior_killed"); - wfc->wakeup_error= orig_entry->thd->killed_errno(); + DEBUG_SYNC(thd, "group_commit_waiting_for_prior_killed"); + wfc->wakeup_error= thd->killed_errno(); if (!wfc->wakeup_error) wfc->wakeup_error= ER_QUERY_INTERRUPTED; my_message(wfc->wakeup_error, - ER_THD(orig_entry->thd, wfc->wakeup_error), MYF(0)); + ER_THD(thd, wfc->wakeup_error), MYF(0)); result= -1; goto end; } } - orig_entry->thd->EXIT_COND(&old_stage); + thd->EXIT_COND(&old_stage); } else mysql_mutex_unlock(&wfc->LOCK_wait_commit); @@ -8222,8 +8213,8 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) } /* Now enqueue ourselves in the group commit queue. */ - DEBUG_SYNC(orig_entry->thd, "commit_before_enqueue"); - orig_entry->thd->clear_wakeup_ready(); + DEBUG_SYNC(thd, "commit_before_enqueue"); + thd->clear_wakeup_ready(); mysql_mutex_lock(&LOCK_prepare_ordered); orig_queue= group_commit_queue; @@ -8268,9 +8259,9 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) if (entry->cache_mngr->using_xa) { - DEBUG_SYNC(orig_entry->thd, "commit_before_prepare_ordered"); + DEBUG_SYNC(thd, "commit_before_prepare_ordered"); run_prepare_ordered(entry->thd, entry->all); - DEBUG_SYNC(orig_entry->thd, "commit_after_prepare_ordered"); + DEBUG_SYNC(thd, "commit_after_prepare_ordered"); } if (cur) @@ -8391,14 +8382,14 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) if (opt_binlog_commit_wait_count > 0 && orig_queue != NULL) mysql_cond_signal(&COND_prepare_ordered); mysql_mutex_unlock(&LOCK_prepare_ordered); - DEBUG_SYNC(orig_entry->thd, "commit_after_release_LOCK_prepare_ordered"); + DEBUG_SYNC(thd, "commit_after_release_LOCK_prepare_ordered"); DBUG_PRINT("info", ("Queued for group commit as %s", (orig_queue == NULL) ? "leader" : "participant")); end: if (backup_lock_released) - thd->mdl_context.acquire_lock(thd->backup_commit_lock, + thd->mdl_context.acquire_lock(&thd->mdl_backup, thd->variables.lock_wait_timeout); DBUG_RETURN(result); } diff --git a/sql/mysqld.cc b/sql/mysqld.cc index e7a0daedc41..c817f7e8307 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -742,7 +742,7 @@ mysql_mutex_t LOCK_thread_id; server may be fairly high, we need a dedicated lock. */ mysql_mutex_t LOCK_prepared_stmt_count; -mysql_mutex_t LOCK_backup_log; +mysql_mutex_t LOCK_backup_log, LOCK_backup_commit; mysql_rwlock_t LOCK_grant, LOCK_sys_init_connect, LOCK_sys_init_slave; mysql_rwlock_t LOCK_ssl_refresh; mysql_rwlock_t LOCK_all_status_vars; @@ -921,7 +921,7 @@ PSI_mutex_key key_BINLOG_LOCK_index, key_BINLOG_LOCK_xid_list, key_LOCK_crypt, key_LOCK_delayed_create, key_LOCK_delayed_insert, key_LOCK_delayed_status, key_LOCK_error_log, key_LOCK_gdl, key_LOCK_global_system_variables, - key_LOCK_manager, key_LOCK_backup_log, + key_LOCK_manager, key_LOCK_backup_log, key_LOCK_backup_commit, key_LOCK_prepared_stmt_count, key_LOCK_rpl_status, key_LOCK_server_started, key_LOCK_status, key_LOCK_temp_pool, @@ -985,6 +985,7 @@ static PSI_mutex_info all_server_mutexes[]= { &key_hash_filo_lock, "hash_filo::lock", 0}, { &key_LOCK_active_mi, "LOCK_active_mi", PSI_FLAG_GLOBAL}, { &key_LOCK_backup_log, "LOCK_backup_log", PSI_FLAG_GLOBAL}, + { &key_LOCK_backup_commit, "LOCK_backup_commit", PSI_FLAG_GLOBAL}, { &key_LOCK_temp_pool, "LOCK_temp_pool", PSI_FLAG_GLOBAL}, { &key_LOCK_thread_id, "LOCK_thread_id", PSI_FLAG_GLOBAL}, { &key_LOCK_crypt, "LOCK_crypt", PSI_FLAG_GLOBAL}, @@ -2049,6 +2050,7 @@ static void clean_up(bool print_message) logger.cleanup_end(); sys_var_end(); free_charsets(); + backup_reset(); my_free(const_cast<char*>(log_bin_basename)); my_free(const_cast<char*>(log_bin_index)); @@ -2140,6 +2142,7 @@ static void clean_up_mutexes() mysql_mutex_destroy(&LOCK_active_mi); mysql_rwlock_destroy(&LOCK_ssl_refresh); mysql_mutex_destroy(&LOCK_backup_log); + mysql_mutex_destroy(&LOCK_backup_commit); mysql_mutex_destroy(&LOCK_temp_pool); mysql_rwlock_destroy(&LOCK_sys_init_connect); mysql_rwlock_destroy(&LOCK_sys_init_slave); @@ -4502,6 +4505,7 @@ static int init_thread_environment() MY_MUTEX_INIT_SLOW); mysql_mutex_init(key_LOCK_backup_log, &LOCK_backup_log, MY_MUTEX_INIT_FAST); mysql_mutex_init(key_LOCK_temp_pool, &LOCK_temp_pool, MY_MUTEX_INIT_FAST); + mysql_mutex_init(key_LOCK_backup_commit, &LOCK_backup_commit, MY_MUTEX_INIT_FAST); #ifdef HAVE_OPENSSL #ifdef HAVE_des @@ -9391,6 +9395,8 @@ PSI_stage_info stage_waiting_for_deadlock_kill= { 0, "Waiting for parallel repli PSI_stage_info stage_starting= { 0, "starting", 0}; PSI_stage_info stage_waiting_for_flush= { 0, "Waiting for non trans tables to be flushed", 0}; PSI_stage_info stage_waiting_for_ddl= { 0, "Waiting for DDLs", 0}; +PSI_stage_info stage_backup_setup_mdl_locks= { 0, "Enabling backup commit lock", 0}; +PSI_stage_info stage_backup_got_commit_lock= { 0, "Backup commit lock enabled", 0}; #ifdef WITH_WSREP // Aditional Galera thread states diff --git a/sql/mysqld.h b/sql/mysqld.h index 7dcf14e3532..6a90e8be2b0 100644 --- a/sql/mysqld.h +++ b/sql/mysqld.h @@ -692,6 +692,9 @@ extern PSI_stage_info stage_slave_background_process_request; extern PSI_stage_info stage_slave_background_wait_request; extern PSI_stage_info stage_waiting_for_deadlock_kill; extern PSI_stage_info stage_starting; +extern PSI_stage_info stage_backup_setup_mdl_locks; +extern PSI_stage_info stage_backup_got_commit_lock; + #ifdef WITH_WSREP // Aditional Galera thread states extern PSI_stage_info stage_waiting_isolation; @@ -768,7 +771,8 @@ extern mysql_mutex_t LOCK_error_log, LOCK_delayed_insert, LOCK_short_uuid_generator, LOCK_delayed_status, LOCK_delayed_create, LOCK_crypt, LOCK_timezone, LOCK_active_mi, LOCK_manager, LOCK_user_conn, - LOCK_prepared_stmt_count, LOCK_error_messages, LOCK_backup_log; + LOCK_prepared_stmt_count, LOCK_error_messages, + LOCK_backup_log, LOCK_backup_commit; extern MYSQL_PLUGIN_IMPORT mysql_mutex_t LOCK_global_system_variables; extern mysql_rwlock_t LOCK_all_status_vars; extern mysql_mutex_t LOCK_start_thread; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 20ba2ca7a66..88da104ddd5 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -1345,7 +1345,9 @@ void THD::init() first_successful_insert_id_in_prev_stmt_for_binlog= 0; first_successful_insert_id_in_cur_stmt= 0; current_backup_stage= BACKUP_FINISHED; - backup_commit_lock= 0; + backup_commit_lock.store(OUTSIDE_COMMIT, std::memory_order_relaxed); + MDL_REQUEST_INIT(&mdl_backup, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, + MDL_EXPLICIT); #ifdef WITH_WSREP wsrep_last_query_id= 0; wsrep_xid.null(); @@ -8304,7 +8306,8 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill) { PSI_stage_info old_stage; wait_for_commit *loc_waitee; - bool backup_lock_released= false; + + DBUG_ASSERT(thd == current_thd); /* Release MDL_BACKUP_COMMIT LOCK while waiting for other threads to commit @@ -8312,11 +8315,14 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill) yet have the MDL_BACKUP_COMMIT_LOCK) and any threads using BACKUP LOCK BLOCK_COMMIT. */ - if (thd->backup_commit_lock && thd->backup_commit_lock->ticket) + const bool backup_lock_released= + thd->backup_commit_lock.load(std::memory_order_relaxed) == + THD::IN_COMMIT_WITH_LOCK; + + if (backup_lock_released) { - backup_lock_released= true; - thd->mdl_context.release_lock(thd->backup_commit_lock->ticket); - thd->backup_commit_lock->ticket= 0; + thd->mdl_context.release_lock(thd->mdl_backup.ticket); + thd->mdl_backup.ticket= nullptr; } mysql_mutex_lock(&LOCK_wait_commit); @@ -8331,7 +8337,9 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill) { if (wakeup_error) my_error(ER_PRIOR_COMMIT_FAILED, MYF(0)); - goto end; + end: + thd->EXIT_COND(&old_stage); + goto func_exit; } /* Wait was interrupted by kill. We need to unregister our wait and give the @@ -8367,15 +8375,9 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill) use within enter_cond/exit_cond. */ DEBUG_SYNC(thd, "wait_for_prior_commit_killed"); +func_exit: if (unlikely(backup_lock_released)) - thd->mdl_context.acquire_lock(thd->backup_commit_lock, - thd->variables.lock_wait_timeout); - return wakeup_error; - -end: - thd->EXIT_COND(&old_stage); - if (unlikely(backup_lock_released)) - thd->mdl_context.acquire_lock(thd->backup_commit_lock, + thd->mdl_context.acquire_lock(&thd->mdl_backup, thd->variables.lock_wait_timeout); return wakeup_error; } diff --git a/sql/sql_class.h b/sql/sql_class.h index 98cd1e0d0c8..dde5d1ebca1 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2736,10 +2736,49 @@ class THD: public THD_count, /* this must be first */ rpl_io_thread_info *rpl_io_info; rpl_sql_thread_info *rpl_sql_info; } system_thread_info; - /* Used for BACKUP LOCK */ - MDL_ticket *mdl_backup_ticket, *mdl_backup_lock; - /* Used to register that thread has a MDL_BACKUP_WAIT_COMMIT lock */ - MDL_request *backup_commit_lock; + /** Used in lock_table_names() */ + MDL_ticket *mdl_backup_ticket; + /** Used in BACKUP STAGE */ + MDL_ticket *mdl_backup_lock; + /** Pre-initialized MDL request for protect_against_backup() */ + MDL_request mdl_backup; + + /** the state of protect_against_backup() and unprotect_against_backup() */ + enum backup_locking_state { + OUTSIDE_COMMIT= 0, IN_COMMIT_NO_LOCK, IN_COMMIT_WITH_LOCK + }; + + /** whether the thread holds a MDL_BACKUP_WAIT_COMMIT lock */ + std::atomic<backup_locking_state> backup_commit_lock{OUTSIDE_COMMIT}; + + /** the difference of enable_backup_commit_locks() and + disable_backup_commit_locks() calls */ Please change to: /* The difference of enable_backup_commit_locks() and disable_backup_commit_locks() calls */ + static std::atomic<uint> backup_commit_lock_enabled; + + /** @return whether we failed to acquire mdl_backup when it is necessary */ + bool protect_against_backup() noexcept + { + DBUG_ASSERT(!backup_commit_lock.load(std::memory_order_relaxed)); + backup_commit_lock.store(IN_COMMIT_NO_LOCK, std::memory_order_release); + /* We must have updated our state before the load below, to guarantee that + enable_backup_commit_locks() will account for us. */ Fix comment indentation + return + unlikely(backup_commit_lock_enabled.load(std::memory_order_acquire)) && + protect_against_backup_slow(); use return (...) and indent accordingly. + } There is an issue with your patch here. You are using IN_COMMIT_NO_LOCK even if backup MDL locks are enabled and the other thread waitin for no threads with IN_COMMIT_NO_LOCK. On a busy server, it is in theory possible that THD::enable_backup_commit_locks() will never return as there is always a thread between setting IN_COMMIT_NO_LOCK and changing it to IN_COMMIT_NO_LOCK. A safer strategy would be to check first for backup_commit_lock_enabled.load(std::memory_order_acquire) and if yes set it to IN_COMMIT_LOCK and request MDL lock. There is no problem if a thread would use MDL locks accidently while unprotect_against_backup() is running. Add double empty lines between functions to make this part easier to read. + void unprotect_against_backup() noexcept + { + backup_locking_state s= backup_commit_lock.load(std::memory_order_relaxed); + backup_commit_lock.store(OUTSIDE_COMMIT, std::memory_order_release); + if (unlikely(s == IN_COMMIT_WITH_LOCK)) + unprotect_against_backup_slow(); + } add 2 empty lines here +private: + bool protect_against_backup_slow() noexcept; + void unprotect_against_backup_slow() noexcept; +public: + bool enable_backup_commit_locks() noexcept; + static void disable_backup_commit_locks() noexcept; void reset_for_next_command(bool do_clear_errors= 1);