MDEV-39548 Further cleanup of MDL_request boilerplate#5160
Conversation
- Change thd->backup_commit_lock from MDL_request* to MDL_ticket*, and convert all related call sites. - Convert acquire_lock() in partition_info.cc to MDL_ACQUIRE_LOCK. - Convert try_acquire_lock()/acquire_lock() in sql_show.cc to MDL_TRY_ACQUIRE_LOCK/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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors metadata locking (MDL) by replacing local MDL_request stack allocations and manual lock acquisitions with helper macros like MDL_ACQUIRE_LOCK and MDL_REQUEST_LIST_ADD, and changing backup_commit_lock to store an MDL_ticket pointer. The review feedback highlights a code smell in sql_show.cc where a sentinel pointer (MDL_ticket *) 1 is returned to indicate errors; the reviewer suggests returning a boolean instead, populating the ticket directly, and encapsulating key initialization within the helper function.
| static MDL_ticket * | ||
| try_acquire_high_prio_shared_mdl_lock(THD *thd, TABLE_LIST *table, | ||
| bool can_deadlock) | ||
| { | ||
| bool error; | ||
| MDL_REQUEST_INIT(&table->mdl_request, MDL_key::TABLE, table->db.str, | ||
| table->table_name.str, MDL_SHARED_HIGH_PRIO, | ||
| MDL_TRANSACTION); | ||
| MDL_ticket *ticket; |
There was a problem hiding this comment.
Returning (MDL_ticket *) 1 as a sentinel value to indicate an error is a code smell and can lead to undefined behavior or hard-to-debug issues if the caller ever attempts to dereference it.\n\nInstead of using a sentinel pointer, we should keep the return type as bool (returning true on error, false on success) and populate table->mdl_request.ticket directly. This preserves the original clean semantics, avoids sentinel pointer hacks, and ensures table->mdl_request.ticket is correctly populated for any downstream code.\n\nWe should also initialize table->mdl_request.key inside this function using mdl_key_init to keep the implementation details encapsulated.
static bool\ntry_acquire_high_prio_shared_mdl_lock(THD *thd, TABLE_LIST *table,\n bool can_deadlock)\n{\n MDL_ticket *ticket;\n table->mdl_request.key.mdl_key_init(MDL_key::TABLE, table->db.str,\n table->table_name.str);| bool error; | ||
| ticket= thd->mdl_context.MDL_TRY_ACQUIRE_LOCK( | ||
| MDL_key::TABLE, table->db.str, table->table_name.str, | ||
| MDL_SHARED_HIGH_PRIO, MDL_TRANSACTION, error); | ||
| if (error) | ||
| return (MDL_ticket *) 1; /* error indicator */ | ||
| } | ||
| else | ||
| error= thd->mdl_context.acquire_lock(&table->mdl_request, | ||
| thd->variables.lock_wait_timeout); | ||
| { | ||
| ticket= thd->mdl_context.MDL_ACQUIRE_LOCK( | ||
| MDL_key::TABLE, table->db.str, table->table_name.str, | ||
| MDL_SHARED_HIGH_PRIO, MDL_TRANSACTION, | ||
| thd->variables.lock_wait_timeout); | ||
| if (!ticket) | ||
| return (MDL_ticket *) 1; /* error indicator */ | ||
| } | ||
|
|
||
| return error; | ||
| return ticket; | ||
| } |
There was a problem hiding this comment.
Update the return statements and populate table->mdl_request.ticket directly to avoid using the (MDL_ticket *) 1 sentinel pointer.
bool error;\n ticket= thd->mdl_context.MDL_TRY_ACQUIRE_LOCK(\n MDL_key::TABLE, table->db.str, table->table_name.str,\n MDL_SHARED_HIGH_PRIO, MDL_TRANSACTION, error);\n if (error)\n return true;\n }\n else\n {\n ticket= thd->mdl_context.MDL_ACQUIRE_LOCK(\n MDL_key::TABLE, table->db.str, table->table_name.str,\n MDL_SHARED_HIGH_PRIO, MDL_TRANSACTION,\n thd->variables.lock_wait_timeout);\n if (!ticket)\n return true;\n }\n\n table->mdl_request.ticket= ticket;\n return false;| table_list.mdl_request.key.mdl_key_init(MDL_key::TABLE, | ||
| table_list.db.str, | ||
| table_list.table_name.str); | ||
|
|
||
| if (schema_table->i_s_requested_object & OPEN_TRIGGER_ONLY) |
There was a problem hiding this comment.
Since table_list.mdl_request.key is now fully initialized inside try_acquire_high_prio_shared_mdl_lock, we can remove this manual initialization from the caller.
if (schema_table->i_s_requested_object & OPEN_TRIGGER_ONLY)
|
|
||
| MDL_request mdl_request; | ||
| MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_START, | ||
| MDL_EXPLICIT); |
There was a problem hiding this comment.
@knielsen, what was the intention behind taking MDL_BACKUP_START here? It is normally issued by BACKUP STAGE.
Also, does it really have to register itself as a commit lock in thd->backup_commit_lock?
|
Sergey Vojtovich ***@***.***> writes:
@svoj commented on this pull request.
> @@ -4904,22 +4901,19 @@ 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);
@knielsen, what was the intention behind taking `MDL_BACKUP_START` here? It is normally issued by `BACKUP STAGE`.
The intention here is to block RESET MASTER from running concurrently with
the stage in mariabackup that backs up the binlog files. Since it's somewhat
tricky to handle copying the binlog files if the server is removing them
concurrently, and quite unnecessary. BACKUP_START is a minimal lock that
doesn't block most server operations.
The corresponding code in xtrabackup.cc:
bool do_backup_binlogs() {
// Copy InnoDB binlog files.
// Going to BACKUP STAGE START protects against RESET
// MASTER deleting files during the copy, or FLUSH
// BINARY LOGS truncating them.
if (!opt_no_lock)
xb_mysql_query(mysql_connection, "BACKUP STAGE START",
false, false);
if (!m_common_backup.copy_engine_binlogs(opt_binlog_directory,
Also, does it really have to register itself as a commit lock in `thd->backup_commit_lock`?
I don't see that in the piece of code you quoted? But no,
thd->backup_commit_lock doesn't sound like something that would be involved
here in reset_master().
- Kristian.
|
3 participants
Description
This is a follow-up work to PR 5075 ("Cleanup MDL_request boilerplate with ACQUIRE_LOCK macro").
Change
thd->backup_commit_lockfromMDL_request*toMDL_ticket*Previously,
thd->backup_commit_lockstored a pointer to a stack-allocatedMDL_request, creating dangling pointer risks if the function returned early or the code was refactored without clearing the pointer. This patch changes the type toMDL_ticket*, which is managed by the MDL subsystem and does not depend on the caller's stack frame. All related call sites (handler.cc,log.cc,sql_class.cc,xa.cc,sql_reload.cc,sql_repl.cc) are converted to useMDL_ACQUIRE_LOCKdirectly.Convert
partition_info.cctoMDL_ACQUIRE_LOCKInvestigation confirmed that
tl->mdl_requestis not accessed by downstream code after lock acquisition -- onlytable->mdl_ticketis used. The conversion is safe.Add
MDL_REQUEST_LIST_ADD()helperFor batch lock acquisition via
acquire_locks(), callers previously needed 3 steps: allocate anMDL_requestonMEM_ROOT, initialize it, and push it into the list. The newMDL_REQUEST_LIST_ADD()macro combines these into a single call with built-in OOM checking. Call sites insql_base.ccandsp.ccare converted.sql_show.ccinvestigationInvestigation determined that
table_list.mdl_request.keyis used bytdc_acquire_share()for hash lookup after lock acquisition, so this call site cannot be converted toMDL_ACQUIRE_LOCKwithout a larger refactor of the TDC interface.Release Notes
N/A
How can this PR be tested?
Basing the PR against the correct MariaDB version
Copyright
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.