Skip to content

MDEV-39548 Cleanup MDL_request boilerplate with ACQUIRE_LOCK macro#5075

Open
longjinvan wants to merge 1 commit into
MariaDB:mainfrom
longjinvan:MDEV-39548
Open

MDEV-39548 Cleanup MDL_request boilerplate with ACQUIRE_LOCK macro#5075
longjinvan wants to merge 1 commit into
MariaDB:mainfrom
longjinvan:MDEV-39548

Conversation

@longjinvan
Copy link
Copy Markdown
Contributor

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:

> MDL_request mdl_request;
> MDL_REQUEST_INIT(&mdl_request, namespace, db, object, lock_type, duration);
> if (mdl_context.acquire_lock(&mdl_request, lock_wait_timeout))
>   /* error */;
> ticket = mdl_request.ticket;

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:

> if (!(ticket = mdl_context.ACQUIRE_LOCK(namespace, db, object, lock_type, lock_wait_timeout)))
>   /* error */;
> 

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?

  • 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.

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.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread sql/mdl.h
Comment on lines +568 to +572
#define ACQUIRE_LOCK(NAMESPACE, DB, NAME, TYPE, TIMEOUT, DURATION) \
acquire_lock(NAMESPACE, DB, NAME, TYPE, TIMEOUT, DURATION, __FILE__, __LINE__)

#define ACQUIRE_LOCK_BY_KEY(KEY, TYPE, TIMEOUT, DURATION) \
acquire_lock(KEY, TYPE, TIMEOUT, DURATION, __FILE__, __LINE__)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The new macros ACQUIRE_LOCK and ACQUIRE_LOCK_BY_KEY lack the MDL_ prefix, which is inconsistent with existing macros like MDL_REQUEST_INIT. Using a prefix also helps avoid potential name collisions in the global namespace. Consider renaming them to MDL_ACQUIRE_LOCK and MDL_ACQUIRE_LOCK_BY_KEY respectively.

References
  1. Macros should use consistent prefixes (e.g., MDL_) to maintain naming conventions and prevent namespace collisions. (link)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant