Skip to content

MDEV-39548 Cleanup MDL_request boilerplate with ACQUIRE_LOCK macro #5075

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
longjinvan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MDEV-39548 Cleanup MDL_request boilerplate with ACQUIRE_LOCK macro #5075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions sql/backup.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ bool run_backup_stage(THD *thd, backup_stages stage)

static bool backup_start(THD *thd)
{
MDL_request mdl_request;
DBUG_ENTER("backup_start");

thd->current_backup_stage= BACKUP_FINISHED; // For next test
Expand All @@ -182,20 +181,20 @@ static bool backup_start(THD *thd)
Wait for old backup to finish and block ddl's so that we can start the
ddl logger
*/
MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_BLOCK_DDL,
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_BLOCK_DDL,
MDL_EXPLICIT, thd->variables.lock_wait_timeout)))
DBUG_RETURN(1);

if (start_ddl_logging())
{
thd->mdl_context.release_lock(mdl_request.ticket);
thd->mdl_context.release_lock(ticket);
DBUG_RETURN(1);
}

DBUG_ASSERT(backup_flush_ticket == 0);
backup_flush_ticket= mdl_request.ticket;
backup_flush_ticket= ticket;

/* Downgrade lock to only block other backups */
backup_flush_ticket->downgrade_lock(MDL_BACKUP_START);
Expand Down
14 changes: 6 additions & 8 deletions sql/ddl_log.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,16 +1252,14 @@ static void rename_triggers(THD *thd, DDL_LOG_ENTRY *ddl_log_entry,
We have to create a MDL lock as change_table_names() checks that we
have a mdl locks for the table
*/
MDL_request mdl_request;
MDL_ticket *mdl_ticket;
TRIGGER_RENAME_PARAM trigger_param;
int error __attribute__((unused));
MDL_REQUEST_INIT(&mdl_request, MDL_key::TABLE,
mdl_ticket= thd->mdl_context.MDL_ACQUIRE_LOCK(MDL_key::TABLE,
from_db.str,
from_converted_name.str,
MDL_EXCLUSIVE, MDL_EXPLICIT);
error= thd->mdl_context.acquire_lock(&mdl_request, 1);
/* acquire_locks() should never fail during recovery */
DBUG_ASSERT(error == 0);
MDL_EXCLUSIVE, MDL_EXPLICIT, 1);
/* acquire_lock() should never fail during recovery */
DBUG_ASSERT(mdl_ticket != NULL);
Copy link
Copy Markdown
Contributor

@svoj svoj May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this whole lock is there just to satisfy assertion in Table_triggers_list::change_table_name(), otherwise it is redundant. We could probably relax this assertion and remove the lock. Feel free to research this in a separate task/pr if you like.

Otherwise this change looks alright, just remove unused error indeed.

Copy link
Copy Markdown
Contributor Author

@longjinvan longjinvan May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the unused error variable. We can have another task for investigating whether this lock can be removed entirely.


(void) Table_triggers_list::prepare_for_rename(thd,
&trigger_param,
Expand All @@ -1277,7 +1275,7 @@ static void rename_triggers(THD *thd, DDL_LOG_ENTRY *ddl_log_entry,
&from_converted_name,
&to_db,
&to_table);
thd->mdl_context.release_lock(mdl_request.ticket);
thd->mdl_context.release_lock(mdl_ticket);
}
}

Expand Down
8 changes: 3 additions & 5 deletions sql/handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,13 +1080,11 @@ bool handler::log_not_redoable_operation(const char *operation)
new log entry (and re-copy the table if needed).
*/
THD *thd= table->in_use;
MDL_request mdl_backup;
backup_log_info ddl_log;

MDL_REQUEST_INIT(&mdl_backup, MDL_key::BACKUP, "", "", MDL_BACKUP_DDL,
MDL_STATEMENT);
if (thd->mdl_context.acquire_lock(&mdl_backup,
thd->variables.lock_wait_timeout))
if (!thd->mdl_context.MDL_ACQUIRE_LOCK(MDL_key::BACKUP, "", "", MDL_BACKUP_DDL,
MDL_STATEMENT,
thd->variables.lock_wait_timeout))
DBUG_RETURN(1);

bzero(&ddl_log, sizeof(ddl_log));
Expand Down
12 changes: 4 additions & 8 deletions sql/lock.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,8 +1109,6 @@ bool Global_read_lock::lock_global_read_lock(THD *thd)
if (!m_state)
{
MDL_deadlock_and_lock_abort_error_handler mdl_deadlock_handler;
MDL_request mdl_request;
bool result;

if (thd->current_backup_stage != BACKUP_FINISHED)
{
Expand All @@ -1129,22 +1127,20 @@ bool Global_read_lock::lock_global_read_lock(THD *thd)
MDL_BACKUP_FTWRL1));
DBUG_ASSERT(! thd->mdl_context.is_lock_owner(MDL_key::BACKUP, "", "",
MDL_BACKUP_FTWRL2));
MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_FTWRL1,
MDL_EXPLICIT);

do
{
mdl_deadlock_handler.init();
thd->push_internal_handler(&mdl_deadlock_handler);
result= thd->mdl_context.acquire_lock(&mdl_request,
thd->variables.lock_wait_timeout);
m_mdl_global_read_lock= thd->mdl_context.MDL_ACQUIRE_LOCK(
MDL_key::BACKUP, "", "", MDL_BACKUP_FTWRL1,
MDL_EXPLICIT, thd->variables.lock_wait_timeout);
thd->pop_internal_handler();
} while (mdl_deadlock_handler.need_reopen());

if (result)
if (!m_mdl_global_read_lock)
DBUG_RETURN(true);

m_mdl_global_read_lock= mdl_request.ticket;
m_state= GRL_ACQUIRED;
}
/*
Expand Down
33 changes: 33 additions & 0 deletions sql/mdl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3075,6 +3075,39 @@ MDL_context::acquire_lock(MDL_request *mdl_request, double lock_wait_timeout)
}


MDL_ticket *
MDL_context::acquire_lock(MDL_key::enum_mdl_namespace mdl_namespace,
const char *db, const char *name,
enum_mdl_type mdl_type,
enum_mdl_duration mdl_duration,
double lock_wait_timeout,
const char *src_file, uint src_line)
{
MDL_request mdl_request;
mdl_request.init_with_source(mdl_namespace, db, name, mdl_type, mdl_duration,
src_file, src_line);
if (acquire_lock(&mdl_request, lock_wait_timeout))
return NULL;
return mdl_request.ticket;
}


MDL_ticket *
MDL_context::acquire_lock(const MDL_key *key,
enum_mdl_type mdl_type,
enum_mdl_duration mdl_duration,
double lock_wait_timeout,
const char *src_file, uint src_line)
{
MDL_request mdl_request;
mdl_request.init_by_key_with_source(key, mdl_type, mdl_duration,
src_file, src_line);
if (acquire_lock(&mdl_request, lock_wait_timeout))
return NULL;
return mdl_request.ticket;
}


extern "C" int mdl_request_ptr_cmp(const void* ptr1, const void* ptr2)
{
MDL_request *req1= *(MDL_request**)ptr1;
Expand Down
18 changes: 18 additions & 0 deletions sql/mdl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,12 @@ typedef void (*mdl_cached_object_release_hook)(void *);
#define MDL_REQUEST_INIT_BY_KEY(R, P1, P2, P3) \
(*R).init_by_key_with_source(P1, P2, P3, __FILE__, __LINE__)

#define MDL_ACQUIRE_LOCK(NAMESPACE, DB, NAME, TYPE, DURATION, TIMEOUT) \
acquire_lock(NAMESPACE, DB, NAME, TYPE, DURATION, TIMEOUT, __FILE__, __LINE__)

#define MDL_ACQUIRE_LOCK_BY_KEY(KEY, TYPE, DURATION, TIMEOUT) \
acquire_lock(KEY, TYPE, DURATION, TIMEOUT, __FILE__, __LINE__)


/**
An abstract class for inspection of a connected
Expand Down Expand Up @@ -811,6 +817,18 @@ class MDL_context
enum_mdl_type new_type,
double lock_wait_timeout);

MDL_ticket *acquire_lock(MDL_key::enum_mdl_namespace mdl_namespace,
const char *db, const char *name,
enum_mdl_type mdl_type,
enum_mdl_duration mdl_duration,
double lock_wait_timeout,
const char *src_file, uint src_line);
MDL_ticket *acquire_lock(const MDL_key *key,
enum_mdl_type mdl_type,
enum_mdl_duration mdl_duration,
double lock_wait_timeout,
const char *src_file, uint src_line);

bool clone_ticket(MDL_request *mdl_request);

void release_all_locks_for_name(MDL_ticket *ticket);
Expand Down
9 changes: 4 additions & 5 deletions sql/partition_info.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Copy Markdown
Contributor

@svoj svoj May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno, I'd avoid this change as we can't trivially verify that subsequently executed code doesn't rely on initialized tl->mdl_request. Or did you verify this?

Copy link
Copy Markdown
Contributor Author

@longjinvan longjinvan May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted — keeping the original MDL_REQUEST_INIT + acquire_lock pattern here since subsequent code may rely on tl->mdl_request being initialized.


create_info.db_type= table->s->db_type();
create_info.options|= HA_VERSIONED_TABLE;
Expand Down
42 changes: 16 additions & 26 deletions sql/sql_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,6 @@ bool close_cached_tables(THD *thd, TABLE_LIST *tables,
Get an explicit MDL lock for all requested tables to ensure they are
not used by any other thread
*/
MDL_request_list mdl_requests;

DBUG_PRINT("info", ("Waiting for other threads to close their open tables"));
DEBUG_SYNC(thd, "after_flush_unlock");
Expand All @@ -440,19 +439,16 @@ bool close_cached_tables(THD *thd, TABLE_LIST *tables,

for (TABLE_LIST *table= tables; table; table= table->next_local)
{
MDL_request *mdl_request= new (thd->mem_root) MDL_request;
if (mdl_request == NULL)
if (MDL_ticket *ticket= thd->mdl_context.MDL_ACQUIRE_LOCK(
MDL_key::TABLE, table->db.str, table->table_name.str,
MDL_EXCLUSIVE, MDL_EXPLICIT, timeout))
{
tdc_remove_table(thd, table->db.str, table->table_name.str);
thd->mdl_context.release_lock(ticket);
}
else
DBUG_RETURN(true);
MDL_REQUEST_INIT_BY_KEY(mdl_request, &table->mdl_request.key,
MDL_EXCLUSIVE, MDL_STATEMENT);
mdl_requests.push_front(mdl_request);
}

if (thd->mdl_context.acquire_locks(&mdl_requests, timeout))
DBUG_RETURN(true);

for (TABLE_LIST *table= tables; table; table= table->next_local)
tdc_remove_table(thd, table->db.str, table->table_name.str);
}
DBUG_RETURN(false);
}
Expand Down Expand Up @@ -618,13 +614,9 @@ bool flush_tables(THD *thd, flush_tables_type flag)
In this case we cannot sending the HA_EXTRA_FLUSH signal.
*/

MDL_request mdl_request;
MDL_REQUEST_INIT(&mdl_request, MDL_key::TABLE,
share->db.str,
share->table_name.str,
MDL_SHARED, MDL_EXPLICIT);

if (!thd->mdl_context.acquire_lock(&mdl_request, 0))
if (MDL_ticket *mdl_ticket= thd->mdl_context.MDL_ACQUIRE_LOCK(
MDL_key::TABLE, share->db.str, share->table_name.str,
MDL_SHARED, MDL_EXPLICIT, 0))
{
/*
HA_OPEN_FOR_FLUSH is used to allow us to open the table even if
Expand All @@ -646,7 +638,7 @@ bool flush_tables(THD *thd, flush_tables_type flag)
*/
closefrm(tmp_table);
}
thd->mdl_context.release_lock(mdl_request.ticket);
thd->mdl_context.release_lock(mdl_ticket);
}
}
tdc_release_share(share);
Expand Down Expand Up @@ -2403,7 +2395,6 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
MYSQL_OPEN_SKIP_SCOPED_MDL_LOCK)) &&
! ot_ctx->has_protection_against_grl(mdl_type))
{
MDL_request protection_request;
MDL_deadlock_handler mdl_deadlock_handler(ot_ctx);

if (thd->has_read_only_protection())
Expand All @@ -2414,16 +2405,15 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
DBUG_RETURN(TRUE);
}

MDL_REQUEST_INIT(&protection_request, MDL_key::BACKUP, "", "", mdl_type,
MDL_STATEMENT);

/*
Install error handler which if possible will convert deadlock error
into request to back-off and restart process of opening tables.
*/
thd->push_internal_handler(&mdl_deadlock_handler);
bool result= thd->mdl_context.acquire_lock(&protection_request,
ot_ctx->get_timeout());
bool result= !thd->mdl_context.MDL_ACQUIRE_LOCK(MDL_key::BACKUP, "", "",
mdl_type,
MDL_STATEMENT,
ot_ctx->get_timeout());
thd->pop_internal_handler();

if (result)
Expand Down
23 changes: 9 additions & 14 deletions sql/sql_sequence.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,6 @@ int SEQUENCE::read_initial_values(TABLE *table)
{
int error= 0;
enum thr_lock_type save_lock_type;
MDL_request mdl_request; // Empty constructor!
DBUG_ENTER("SEQUENCE::read_initial_values");

if (likely(initialized != SEQ_UNINTIALIZED))
Expand All @@ -633,7 +632,7 @@ int SEQUENCE::read_initial_values(TABLE *table)
if (likely(initialized == SEQ_UNINTIALIZED))
{
MYSQL_LOCK *lock;
bool mdl_lock_used= 0;
MDL_ticket *mdl_ticket_seq= NULL;
THD *thd= table->in_use;
bool has_active_transaction= !thd->transaction->stmt.is_empty();
/*
Expand All @@ -643,19 +642,15 @@ int SEQUENCE::read_initial_values(TABLE *table)
*/
if (table->mdl_ticket == 0)
{
MDL_request_list mdl_requests;
mdl_lock_used= 1;
/*
This happens if first request is SHOW CREATE TABLE or LIST FIELDS
where we don't have a mdl lock on the table
*/

MDL_REQUEST_INIT(&mdl_request, MDL_key::TABLE, table->s->db.str,
table->s->table_name.str, MDL_SHARED_READ,
MDL_EXPLICIT);
mdl_requests.push_front(&mdl_request);
if (thd->mdl_context.acquire_locks(&mdl_requests,
thd->variables.lock_wait_timeout))
if (!(mdl_ticket_seq= thd->mdl_context.MDL_ACQUIRE_LOCK(
MDL_key::TABLE, table->s->db.str, table->s->table_name.str,
MDL_SHARED_READ, MDL_EXPLICIT,
thd->variables.lock_wait_timeout)))
{
write_unlock(table);
DBUG_RETURN(HA_ERR_LOCK_WAIT_TIMEOUT);
Expand All @@ -666,8 +661,8 @@ int SEQUENCE::read_initial_values(TABLE *table)
if (!(lock= mysql_lock_tables(thd, &table, 1,
MYSQL_LOCK_IGNORE_GLOBAL_READ_ONLY)))
{
if (mdl_lock_used)
thd->mdl_context.release_lock(mdl_request.ticket);
if (mdl_ticket_seq)
thd->mdl_context.release_lock(mdl_ticket_seq);
write_unlock(table);

if (!has_active_transaction && !thd->transaction->stmt.is_empty() &&
Expand All @@ -679,8 +674,8 @@ int SEQUENCE::read_initial_values(TABLE *table)
if (likely(!(error= read_stored_values(table))))
initialized= SEQ_READY_TO_USE;
mysql_unlock_tables(thd, lock);
if (mdl_lock_used)
thd->mdl_context.release_lock(mdl_request.ticket);
if (mdl_ticket_seq)
thd->mdl_context.release_lock(mdl_ticket_seq);

/* Reset value to default */
table->reginfo.lock_type= save_lock_type;
Expand Down
Loading
Loading