From f40ea8f4e6084dcda6ae621928e4e966f54348f6 Mon Sep 17 00:00:00 2001 From: Bill Jin Date: Wed, 27 May 2026 23:51:41 +0000 Subject: [PATCH] MDEV-39548 Further cleanup of MDL_request boilerplate - Change thd->backup_commit_lock from MDL_request* to MDL_ticket*, and convert related call sites in handler.cc, log.cc, sql_class.cc, and xa.cc to use MDL_ACQUIRE_LOCK. - Convert reload_acl_and_cache() in sql_reload.cc, purge_master_logs() and reset_master() in sql_repl.cc to MDL_ACQUIRE_LOCK, holding the ticket in a local MDL_ticket* (these functions used a local MDL_request originally and should not touch thd->backup_commit_lock). - Convert acquire_lock() in partition_info.cc to MDL_ACQUIRE_LOCK. - Add MDL_REQUEST_LIST_ADD() helper for enqueuing lock requests into MDL_request_list, and convert call sites in sql_base.cc and sp.cc. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc. --- sql/handler.cc | 19 ++++++++--------- sql/log.cc | 47 ++++++++++++++++++------------------------- sql/mdl.h | 20 ++++++++++++++++++ sql/partition_info.cc | 9 ++++----- sql/sp.cc | 11 ++++++---- sql/sql_base.cc | 8 +++----- sql/sql_class.cc | 16 ++++++++------- sql/sql_class.h | 2 +- sql/sql_reload.cc | 20 ++++++------------ sql/sql_repl.cc | 28 +++++++++----------------- sql/xa.cc | 28 ++++++++++---------------- 11 files changed, 99 insertions(+), 109 deletions(-) diff --git a/sql/handler.cc b/sql/handler.cc index 955de44940a..12dc4240c63 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -1852,7 +1852,7 @@ int ha_commit_trans(THD *thd, bool all) &no_rollback); /* rw_trans is TRUE when we in a transaction changing data */ bool rw_trans= is_real_trans && rw_ha_count > 0; - MDL_request mdl_backup; + bool backup_commit_lock_acquired= false; DBUG_PRINT("info", ("is_real_trans: %d rw_trans: %d rw_ha_count: %d", is_real_trans, rw_trans, rw_ha_count)); @@ -1872,19 +1872,17 @@ 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 (thd->mdl_context.acquire_lock(&mdl_backup, - thd->variables.lock_wait_timeout)) + if (!(thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, + MDL_EXPLICIT, 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; + backup_commit_lock_acquired= true; } DEBUG_SYNC(thd, "ha_commit_trans_after_acquire_commit_lock"); } @@ -2134,8 +2132,8 @@ err: thd->rgi_slave->is_parallel_exec); } end: - // reset the pointer to the ticket when it's stack instantiated - if (thd->backup_commit_lock == &mdl_backup) + // release the backup commit lock if we acquired it in this function + if (backup_commit_lock_acquired && thd->backup_commit_lock) { /* We do not always immediately release transactional locks @@ -2143,8 +2141,7 @@ end: thus we release the commit blocker lock as soon as it's not needed. */ - if (mdl_backup.ticket) - thd->mdl_context.release_lock(mdl_backup.ticket); + thd->mdl_context.release_lock(thd->backup_commit_lock); thd->backup_commit_lock= 0; } #ifdef WITH_WSREP diff --git a/sql/log.cc b/sql/log.cc index 04ad4ac4258..c1ad12605e7 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -8811,19 +8811,15 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate) } else { - MDL_request mdl_request; - - 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->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, + MDL_EXPLICIT, thd->variables.lock_wait_timeout))) 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); + if (thd->backup_commit_lock) + thd->mdl_context.release_lock(thd->backup_commit_lock); thd->backup_commit_lock= 0; DBUG_RETURN(res); } @@ -8833,8 +8829,8 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate) prev_binlog_id= current_binlog_id; res= write_gtid_event(thd, nullptr, true, using_trans, commit_id, false, false, false); - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); + if (thd->backup_commit_lock) + thd->mdl_context.release_lock(thd->backup_commit_lock); thd->backup_commit_lock= 0; if (res) goto err; @@ -8944,7 +8940,6 @@ err: &cache_data->engine_binlog_info; engine_context->gtid_offset= my_b_tell(file); my_off_t binlog_total_bytes; - MDL_request mdl_request; int res; if (engine_context->out_of_band_offset) @@ -8957,17 +8952,15 @@ err: goto engine_fail; } - 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->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, + MDL_EXPLICIT, thd->variables.lock_wait_timeout))) goto engine_fail; - thd->backup_commit_lock= &mdl_request; if (thd->wait_for_prior_commit()) { - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); + if (thd->backup_commit_lock) + thd->mdl_context.release_lock(thd->backup_commit_lock); thd->backup_commit_lock= 0; goto engine_fail; } @@ -8985,8 +8978,8 @@ err: mysql_mutex_lock(&LOCK_log); res= write_gtid_event(thd, cache_data, true, using_trans, commit_id, false, false, false); - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); + if (thd->backup_commit_lock) + thd->mdl_context.release_lock(thd->backup_commit_lock); thd->backup_commit_lock= 0; if (res) { @@ -10258,12 +10251,11 @@ 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 (thd->backup_commit_lock) { backup_lock_released= 1; - thd->mdl_context.release_lock(thd->backup_commit_lock->ticket); - thd->backup_commit_lock->ticket= 0; + thd->mdl_context.release_lock(thd->backup_commit_lock); + thd->backup_commit_lock= 0; } /* @@ -10527,8 +10519,9 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) end: if (backup_lock_released) - thd->mdl_context.acquire_lock(thd->backup_commit_lock, - thd->variables.lock_wait_timeout); + thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, + MDL_EXPLICIT, thd->variables.lock_wait_timeout); DBUG_RETURN(result); } diff --git a/sql/mdl.h b/sql/mdl.h index f48a4931460..dbc6148c7a0 100644 --- a/sql/mdl.h +++ b/sql/mdl.h @@ -574,6 +574,10 @@ typedef void (*mdl_cached_object_release_hook)(void *); #define MDL_TRY_ACQUIRE_LOCK(NAMESPACE, DB, NAME, TYPE, DURATION, ERROR) \ try_acquire_lock(NAMESPACE, DB, NAME, TYPE, DURATION, ERROR, __FILE__, __LINE__) +#define MDL_REQUEST_LIST_ADD(LIST, MEM_ROOT, NAMESPACE, DB, NAME, TYPE, DURATION) \ + mdl_request_list_add(LIST, MEM_ROOT, NAMESPACE, DB, NAME, TYPE, DURATION, \ + __FILE__, __LINE__) + /** An abstract class for inspection of a connected @@ -794,6 +798,22 @@ typedef I_P_List MDL_request_list; +inline bool mdl_request_list_add(MDL_request_list *list, MEM_ROOT *mem_root, + MDL_key::enum_mdl_namespace mdl_namespace, + const char *db, const char *name, + enum_mdl_type mdl_type, + enum_mdl_duration mdl_duration, + const char *src_file, uint src_line) +{ + MDL_request *r= new (mem_root) MDL_request; + if (!r) + return true; + r->init_with_source(mdl_namespace, db, name, mdl_type, mdl_duration, + src_file, src_line); + list->push_front(r); + return false; +} + /** Context of the owner of metadata locks. I.e. each server connection has such a context. diff --git a/sql/partition_info.cc b/sql/partition_info.cc index 7666997dc5e..a55a0840bf2 100644 --- a/sql/partition_info.cc +++ b/sql/partition_info.cc @@ -926,12 +926,11 @@ bool vers_create_partitions(THD *thd, TABLE_LIST* tl, uint num_parts) create_info.alter_info= &alter_info; Alter_table_ctx alter_ctx(thd, tl, 1, &table->s->db, &table->s->table_name); - MDL_REQUEST_INIT(&tl->mdl_request, MDL_key::TABLE, tl->db.str, - tl->table_name.str, MDL_SHARED_NO_WRITE, MDL_TRANSACTION); - if (thd->mdl_context.acquire_lock(&tl->mdl_request, - thd->variables.lock_wait_timeout)) + if (!(table->mdl_ticket= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::TABLE, tl->db.str, tl->table_name.str, + MDL_SHARED_NO_WRITE, MDL_TRANSACTION, + thd->variables.lock_wait_timeout))) goto exit; - table->mdl_ticket= tl->mdl_request.ticket; create_info.db_type= table->s->db_type(); create_info.options|= HA_VERSIONED_TABLE; diff --git a/sql/sp.cc b/sql/sp.cc index cbaf72b9398..9c47d55e691 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -1876,14 +1876,17 @@ bool lock_db_routines(THD *thd, const Lex_ident_db_normalized &db) continue; longlong sp_type= table->field[MYSQL_PROC_MYSQL_TYPE]->val_int(); - MDL_request *mdl_request= new (thd->mem_root) MDL_request; const Sp_handler *sph= Sp_handler::handler((enum_sp_type) sp_type); if (!sph) sph= &sp_handler_procedure; - MDL_REQUEST_INIT(mdl_request, sph->get_mdl_type(), db.str, sp_name, - MDL_EXCLUSIVE, MDL_TRANSACTION); - mdl_requests.push_front(mdl_request); + if (MDL_REQUEST_LIST_ADD(&mdl_requests, thd->mem_root, + sph->get_mdl_type(), db.str, sp_name, + MDL_EXCLUSIVE, MDL_TRANSACTION)) + { + table->file->ha_index_end(); + goto error; + } } while (! (nxtres= table->file->ha_index_next_same(table->record[0], keybuf, key_len))); } table->file->ha_index_end(); diff --git a/sql/sql_base.cc b/sql/sql_base.cc index d1d876985f8..54b40938197 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -4490,12 +4490,10 @@ lock_table_names(THD *thd, const DDL_options_st &options, /* Scoped locks: Take intention exclusive locks on all involved schemas. */ if (!(flags & MYSQL_OPEN_SKIP_SCOPED_MDL_LOCK)) { - MDL_request *schema_request= new (thd->mem_root) MDL_request; - if (schema_request == NULL) + if (MDL_REQUEST_LIST_ADD(&mdl_requests, thd->mem_root, + MDL_key::SCHEMA, table->db.str, "", + MDL_INTENTION_EXCLUSIVE, MDL_TRANSACTION)) DBUG_RETURN(TRUE); - MDL_REQUEST_INIT(schema_request, MDL_key::SCHEMA, table->db.str, "", - MDL_INTENTION_EXCLUSIVE, MDL_TRANSACTION); - mdl_requests.push_front(schema_request); } mdl_requests.push_front(&table->mdl_request); diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 7dd1e1810ee..70c571f14e6 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -8565,11 +8565,11 @@ 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) + if (thd->backup_commit_lock) { 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->backup_commit_lock); + thd->backup_commit_lock= 0; } mysql_mutex_lock(&LOCK_wait_commit); @@ -8625,15 +8625,17 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill) */ DEBUG_SYNC(thd, "wait_for_prior_commit_killed"); if (unlikely(backup_lock_released)) - thd->mdl_context.acquire_lock(thd->backup_commit_lock, - thd->variables.lock_wait_timeout); + thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, + MDL_EXPLICIT, 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->variables.lock_wait_timeout); + thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, + MDL_EXPLICIT, thd->variables.lock_wait_timeout); return wakeup_error; } diff --git a/sql/sql_class.h b/sql/sql_class.h index 86bb474c85d..156abed5207 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3369,7 +3369,7 @@ public: /* 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; + MDL_ticket *backup_commit_lock; void reset_for_next_command(bool do_clear_errors= 1); diff --git a/sql/sql_reload.cc b/sql/sql_reload.cc index f4916537edc..f3e52732654 100644 --- a/sql/sql_reload.cc +++ b/sql/sql_reload.cc @@ -173,18 +173,14 @@ bool reload_acl_and_cache(THD *thd, unsigned long long options, tmp_write_to_binlog= 0; if (mysql_bin_log.is_open()) { - MDL_request mdl_request; - MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_START, - MDL_EXPLICIT); + MDL_ticket *ticket= NULL; if (thd && - thd->mdl_context.acquire_lock(&mdl_request, - thd->variables.lock_wait_timeout)) + !(ticket= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_START, + MDL_EXPLICIT, thd->variables.lock_wait_timeout))) result= 1; else { - if (thd) - thd->backup_commit_lock= &mdl_request; - DYNAMIC_ARRAY *drop_gtid_domain= (thd && (thd->lex->delete_gtid_domain.elements > 0)) ? &thd->lex->delete_gtid_domain : NULL; @@ -198,12 +194,8 @@ bool reload_acl_and_cache(THD *thd, unsigned long long options, /* Wait for last binlog checkpoint event to be logged. */ mysql_bin_log.wait_for_last_checkpoint_event(); } - if (thd) - { - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); - thd->backup_commit_lock= 0; - } + if (ticket) + thd->mdl_context.release_lock(ticket); } } } diff --git a/sql/sql_repl.cc b/sql/sql_repl.cc index 7dbd646a174..af96ec74e2f 100644 --- a/sql/sql_repl.cc +++ b/sql/sql_repl.cc @@ -800,13 +800,11 @@ bool purge_master_logs(THD* thd, const char* to_log) return FALSE; } - MDL_request mdl_request; - MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_START, - MDL_EXPLICIT); - if (thd->mdl_context.acquire_lock(&mdl_request, - thd->variables.lock_wait_timeout)) + MDL_ticket *ticket; + if (!(ticket= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_START, + MDL_EXPLICIT, thd->variables.lock_wait_timeout))) return TRUE; - thd->backup_commit_lock= &mdl_request; int res; if (!opt_binlog_engine_hton) @@ -849,9 +847,7 @@ bool purge_master_logs(THD* thd, const char* to_log) purge_info.nonpurge_filename, true); } - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); - thd->backup_commit_lock= 0; + thd->mdl_context.release_lock(ticket); return purge_error_message(thd, res); } @@ -4904,13 +4900,11 @@ int reset_master(THD* thd, rpl_gtid *init_state, uint32 init_state_len, #endif /* WITH_WSREP */ bool ret= 0; - MDL_request mdl_request; - MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_START, - MDL_EXPLICIT); - if (thd->mdl_context.acquire_lock(&mdl_request, - thd->variables.lock_wait_timeout)) + MDL_ticket *ticket; + if (!(ticket= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_START, + MDL_EXPLICIT, thd->variables.lock_wait_timeout))) return 1; - thd->backup_commit_lock= &mdl_request; /* Temporarily disable master semisync before resetting master. */ repl_semisync_master.before_reset_master(); @@ -4918,9 +4912,7 @@ int reset_master(THD* thd, rpl_gtid *init_state, uint32 init_state_len, next_log_number); repl_semisync_master.after_reset_master(); - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); - thd->backup_commit_lock= 0; + thd->mdl_context.release_lock(ticket); DBUG_EXECUTE_IF("crash_after_reset_master", DBUG_SUICIDE();); diff --git a/sql/xa.cc b/sql/xa.cc index 2712516f146..538faf3e1e6 100644 --- a/sql/xa.cc +++ b/sql/xa.cc @@ -526,15 +526,13 @@ bool trans_xa_end(THD *thd) to reset thd->backup_commit_lock before returning! */ -static bool trans_xa_get_backup_lock(THD *thd, MDL_request *mdl_request) +static bool trans_xa_get_backup_lock(THD *thd) { DBUG_ASSERT(thd->backup_commit_lock == 0); - 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->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK( + MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, + MDL_EXPLICIT, thd->variables.lock_wait_timeout))) return 1; - thd->backup_commit_lock= mdl_request; return 0; } @@ -542,7 +540,7 @@ static inline void trans_xa_release_backup_lock(THD *thd) { if (thd->backup_commit_lock) { - thd->mdl_context.release_lock(thd->backup_commit_lock->ticket); + thd->mdl_context.release_lock(thd->backup_commit_lock); thd->backup_commit_lock= 0; } } @@ -570,11 +568,10 @@ bool trans_xa_prepare(THD *thd) my_error(ER_XAER_NOTA, MYF(0)); else { - MDL_request mdl_request; - if (trans_xa_get_backup_lock(thd, &mdl_request) || + if (trans_xa_get_backup_lock(thd) || ha_prepare(thd)) { - if (!mdl_request.ticket) + if (!thd->backup_commit_lock) /* Failed to get the backup lock */ ha_rollback_trans(thd, TRUE); thd->variables.option_bits&= ~(OPTION_BEGIN | OPTION_BINLOG_THIS_TRX); @@ -659,7 +656,6 @@ bool trans_xa_commit(THD *thd) if (auto xs= xid_cache_search(thd, thd->lex->xid)) { bool xid_deleted= false; - MDL_request mdl_request; bool rw_trans= (xs->rm_error != ER_XA_RBROLLBACK); if (rw_trans && thd->check_read_only_with_error()) @@ -668,7 +664,7 @@ bool trans_xa_commit(THD *thd) goto _end_external_xid; } res= xa_trans_rolled_back(xs); - if (trans_xa_get_backup_lock(thd, &mdl_request)) + if (trans_xa_get_backup_lock(thd)) { /* We can't rollback an XA transaction on lock failure due to @@ -724,14 +720,13 @@ bool trans_xa_commit(THD *thd) else if (thd->transaction->xid_state.xid_cache_element->xa_state == XA_PREPARED) { - MDL_request mdl_request; if (thd->lex->xa_opt != XA_NONE) { my_error(ER_XAER_INVAL, MYF(0)); DBUG_RETURN(TRUE); } - if (trans_xa_get_backup_lock(thd, &mdl_request)) + if (trans_xa_get_backup_lock(thd)) { /* We can't rollback an XA transaction on lock failure due to @@ -796,7 +791,6 @@ bool trans_xa_commit(THD *thd) bool trans_xa_rollback(THD *thd) { XID_STATE &xid_state= thd->transaction->xid_state; - MDL_request mdl_request; bool error; DBUG_ENTER("trans_xa_rollback"); @@ -826,7 +820,7 @@ bool trans_xa_rollback(THD *thd) goto _end_external_xid; } - if (trans_xa_get_backup_lock(thd, &mdl_request)) + if (trans_xa_get_backup_lock(thd)) { /* We can't rollback an XA transaction on lock failure due to @@ -869,7 +863,7 @@ bool trans_xa_rollback(THD *thd) DBUG_RETURN(TRUE); } - if (trans_xa_get_backup_lock(thd, &mdl_request)) + if (trans_xa_get_backup_lock(thd)) { /* We can't rollback an XA transaction on lock failure due to