MDEV-39548 Cleanup MDL_request boilerplate with ACQUIRE_LOCK macro#5075
MDEV-39548 Cleanup MDL_request boilerplate with ACQUIRE_LOCK macro#5075longjinvan wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request streamlines the process of acquiring Metadata Locks (MDL) by adding new acquire_lock methods to the MDL_context class and introducing ACQUIRE_LOCK and ACQUIRE_LOCK_BY_KEY macros. These additions allow for more concise code by removing the need to manually declare and initialize MDL_request objects in many parts of the server, such as in backup operations, DDL logging, and table handling. The reviewer recommended adding an MDL_ prefix to the new macros to ensure consistency with existing naming patterns and to prevent potential namespace conflicts.
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Nice cleanup. Some more changes and it will be perfect.
c2c8e8e to
a0f3c13
Compare
svoj
left a comment
There was a problem hiding this comment.
Looks very nice, a few comments inline.
| MDL_SHARED_NO_WRITE, thd->variables.lock_wait_timeout, | ||
| MDL_TRANSACTION))) | ||
| goto exit; | ||
| table->mdl_ticket= tl->mdl_request.ticket; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Reverted — keeping the original MDL_REQUEST_INIT + acquire_lock pattern here since subsequent code may rely on tl->mdl_request being initialized.
There was a problem hiding this comment.
I don't see this reverted. Please double check.
There was a problem hiding this comment.
Sorry about that — I forgot to copy the revert from my test workspace to the submission package. Fixed now.
106f9e7 to
433b5cf
Compare
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Please keep working on it for the final review.
svoj
left a comment
There was a problem hiding this comment.
There're a few more cases that can be updated:
dict_acquire_mdl_share()- replace both acquire and try_acquirebackup_lock()it is not right that sql_yacc.yy makes decision which lock type to use, change toMDL_ACQIRE_LOCK()pleaseItem_func_get_lock::val_int()- pre-initializeull_keyinstead ofull_requestlike inItem_func_release_lock::val_int()open_and_lock_for_insert_delayed()-protection_requestcan be replacedtrx_purge_table_acquire()lock_table_names()-global_requestcan be moved
| thd->variables.lock_wait_timeout)) | ||
| if (!(mdl_ticket= thd->mdl_context.MDL_ACQUIRE_LOCK( | ||
| MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, | ||
| MDL_TRANSACTION, thd->variables.lock_wait_timeout))) |
There was a problem hiding this comment.
You don’t need to use mdl_ticket here. Checking the result of MDL_ACQUIRE_LOCK() is sufficient, as in the original code.
There was a problem hiding this comment.
Fixed. Now just checks the return value without assigning to mdl_ticket.
Done. Updated in the latest revision:
|
Add new acquire_lock() overloads on MDL_context that combine MDL_request initialization and lock acquisition into a single call, returning MDL_ticket* directly. Convert applicable call sites across the codebase to use the new ACQUIRE_LOCK macro. 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.
Description
MDL (Metadata Lock) is MariaDB's locking subsystem that protects database objects (tables, schemas, stored procedures, etc.) from concurrent DDL — for example, preventing a table from being dropped while another thread is reading from it. MDL_request is the "lock request form" that callers must create, initialize, submit, and then extract the resulting MDL_ticket from.
Currently, every MDL lock acquisition requires repetitive boilerplate:
This patch introduces MDL_context::ACQUIRE_LOCK() — a set of overloaded methods (wrapped in a macro for FILE/LINE tracking) that combine the above steps into a single call returning MDL_ticket* directly:
This is a step toward making MDL_request a private implementation detail of the MDL subsystem, reducing its exposure to callers and preventing the class of bugs where stale or misused MDL_request objects cause issues (MDEV-39241, MDEV-39184).
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.