MDEV-39548 Further cleanup of MDL_request boilerplate

[longjinvan]

Description

This is a follow-up work to PR 5075 ("Cleanup MDL_request boilerplate with ACQUIRE_LOCK macro").

Change thd->backup_commit_lock from MDL_request* to MDL_ticket*

Previously, thd->backup_commit_lock stored a pointer to a stack-allocated MDL_request, creating dangling pointer risks if the function returned early or the code was refactored without clearing the pointer. This patch changes the type to MDL_ticket*, which is managed by the MDL subsystem and does not depend on the caller's stack frame. Related call sites in handler.cc, log.cc, sql_class.cc, and xa.cc are converted to use MDL_ACQUIRE_LOCK directly.

Convert binlog management functions to local MDL_ticket*

In reload_acl_and_cache() (sql_reload.cc), purge_master_logs() and reset_master() (sql_repl.cc), the original code stored the lock into thd->backup_commit_lock, but this field is only meaningful for the "wait for prior commit" routine and was not actually needed here. These functions are converted to use MDL_ACQUIRE_LOCK with a local MDL_ticket* instead.

Convert partition_info.cc to MDL_ACQUIRE_LOCK

Investigation confirmed that tl->mdl_request is not accessed by downstream code after lock acquisition -- only table->mdl_ticket is used. The conversion is safe.

Add MDL_REQUEST_LIST_ADD() helper

For batch lock acquisition via acquire_locks(), callers previously needed 3 steps: allocate an MDL_request on MEM_ROOT, initialize it, and push it into the list. The new MDL_REQUEST_LIST_ADD() macro combines these into a single call with built-in OOM checking. Call sites in sql_base.cc and sp.cc are converted.

Release Notes

N/A

How can this PR be tested?

• All MTR tests pass, confirming no regressions introduced by this refactoring.

Basing the PR against the correct MariaDB version

• This is a refactoring change, and the PR is based against the latest MariaDB development branch.

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.

[gemini-code-assist]

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.

[knielsen]

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.

[gkodinov]

This is a preliminary review. Thank you for your contribution!

LGTM. Please stand by for Svoj's final review.

[svoj]

Looks nice, thanks! A few minor comments inline.

[svoj]

@knielsen

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.

Sounds like MDL_BACKUP_START may indeed be suitable for this purpose. Though I'd probably add a new lock type specifically for it. Still, I guess we can live with this.

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().

All 3 occurrences in FLUSH BINARY LOGS, PURGE BINARY LOGS and RESET MASTER are having this pattern:

  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))
    return TRUE;
  thd->backup_commit_lock= &mdl_request;

FWICS it is used exclusively by the "wait for the prior commit" routine, which is irrelevant here, right?

[knielsen]

Sergey Vojtovich ***@***.***> writes:

svoj left a comment (MariaDB/server#5160) Sounds like `MDL_BACKUP_START` may indeed be suitable for this purpose. Though I'd probably add a new lock type specifically for it. Still, I guess we can live with this.

This is still new, so I think it's fine to change it if you want, or leaving it as-is is also fine with me.

All 3 occurrences in `FLUSH BINARY LOGS`, `PURGE BINARY LOGS` and `RESET MASTER` are having this pattern: ``` 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)) return TRUE; thd->backup_commit_lock= &mdl_request; ``` FWICS it is used exclusively by the "wait for the prior commit" routine, which is irrelevant here, right?

Agree, I think this must be a copy-paste brainfart by me, there should be no need to mess with backup_commit_lock here. - Kristian.

[svoj]

Comments inline.

[svoj]

@longjinvan thanks for your work! If you're interested to continue improving MDL I can suggest the following steps:

1. replace MDL_request with MDL_key + MDL_ticket combo in Sroutine_hash_entry
2. lock_schema_name() and lock_object_name() - lock directly without lists, use MDL_savepoint
3. I found inconsistencies in compatibility matrices for backup namespace bitmaps. That is comment is not conforming to code bitmaps. Both granted and waiting matrices should be reviewed and fixed. Target version is 10.11. Matrices are in mdl.cc, look for MDL_lock::MDL_backup_lock::m_granted_incompatible.

[svoj]

@longjinvan just checking if you're still interested to continue working on these improvements. If not, I will take it over.