MDEV-39548 Cleanup MDL_request boilerplate with ACQUIRE_LOCK macro by longjinvan · Pull Request #5075 · MariaDB/server

longjinvan · 2026-05-13T23:11:48Z

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.

gemini-code-assist

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.

gemini-code-assist · 2026-05-13T23:13:56Z

+#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__)


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

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

Ideally all these macros will go away when we start using C++20 and std::source_location default.
Until then, and, in preparation for this let's indeed use the MDL_ prefix for these macros.

Done. Renamed to MDL_ACQUIRE_LOCK / MDL_ACQUIRE_LOCK_BY_KEY.

gkodinov

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

Nice cleanup. Some more changes and it will be perfect.

gkodinov · 2026-05-14T09:08:57Z

+#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__)


Ideally all these macros will go away when we start using C++20 and std::source_location default.
Until then, and, in preparation for this let's indeed use the MDL_ prefix for these macros.

gkodinov · 2026-05-14T09:19:50Z

-    MDL_request mdl_request;
+    MDL_ticket *mdl_ticket;
    TRIGGER_RENAME_PARAM trigger_param;
    int error __attribute__((unused));


Error is unused here. Please remove. This will fix some of the buildbot failures.

svoj

Looks very nice, a few comments inline.

svoj · 2026-05-14T17:43:54Z

  (*R).init_by_key_with_source(P1, P2, P3, __FILE__, __LINE__)

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


Here and everywhere else, please move DURATION before TIMEOUT for consistency with acquire_lock(MDL_context, ...). I missed duratiton argument in the jira issue, mea culpa.

Done. Moved DURATION before TIMEOUT in the macro, declarations, implementations, and all call sites.

svoj · 2026-05-14T18:01:57Z

-    DBUG_ASSERT(error == 0);
+                     MDL_EXCLUSIVE, 1, MDL_EXPLICIT);
+    /* acquire_lock() should never fail during recovery */
+    DBUG_ASSERT(mdl_ticket != NULL);


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.

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

svoj · 2026-05-14T18:16:46Z

+          thd->variables.lock_wait_timeout, MDL_EXPLICIT);
      thd->pop_internal_handler();
-    } while (mdl_deadlock_handler.need_reopen());
+    } while (!m_mdl_global_read_lock && mdl_deadlock_handler.need_reopen());


Check for m_mdl_global_read_lock is redundant here, that's why it was omitted in the first place.

Done. Removed the !m_mdl_global_read_lock check from the while condition — need_reopen() already implies failure.

svoj · 2026-05-14T18:29:14Z

+            MDL_SHARED_NO_WRITE, thd->variables.lock_wait_timeout,
+            MDL_TRANSACTION)))
      goto exit;
-    table->mdl_ticket= tl->mdl_request.ticket;


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?

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

svoj · 2026-05-14T18:41:33Z

-
-      if (!thd->mdl_context.acquire_lock(&mdl_request, 0))
+      MDL_ticket *mdl_ticket;
+      if ((mdl_ticket= thd->mdl_context.MDL_ACQUIRE_LOCK(


I'd write it as...

if (MDL_ticket *ticket= thd->mdl_context.ACQUIRE_LOCK(...))

...to further reduce ticket scope.

Done. Changed to if (MDL_ticket *ticket= ...) form.

svoj · 2026-05-14T18:49:02Z

It'd be nice if you could rewrite the above 15 lines as something like:

for (TABLE_LIST *table= tables; table; table= table->next_local) { /* Get an exclusive MDL lock for the table, close all table instances and remove it from the table definition cache. */ if (MDL_ticket *ticket= thd->mdl_context.ACQUIRE_LOCK(..., MDL_EXPLICIT, ...)) { tdc_remove_table(thd, table->db.str, table->table_name.str); thd->mdl_context.release_lock(ticket); } else result= true; }

It is a 1 year old queued change in my stash, nice to have it done now.

Done. Replaced the MDL_request_list + batch acquire_locks with a per-table MDL_ACQUIRE_LOCK + tdc_remove_table + release_lock loop.

svoj · 2026-05-14T18:56:12Z

  int error= 0;
  enum thr_lock_type save_lock_type;
-  MDL_request mdl_request;                      // Empty constructor!
+  MDL_ticket *mdl_ticket_seq= NULL;


Please move mdl_ticket_seq declaration down to mdl_lock_used declaration. And remove the latter.

Done. Moved mdl_ticket_seq down into the inner scope and replaced mdl_lock_used with mdl_ticket_seq != NULL checks.

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.

gemini-code-assist Bot reviewed May 13, 2026

View reviewed changes

gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label May 14, 2026

svoj self-requested a review May 14, 2026 09:08

gkodinov requested changes May 14, 2026

View reviewed changes

gkodinov self-assigned this May 14, 2026

longjinvan force-pushed the MDEV-39548 branch 2 times, most recently from c2c8e8e to a0f3c13 Compare May 14, 2026 18:19

svoj requested changes May 14, 2026

View reviewed changes

longjinvan force-pushed the MDEV-39548 branch from a0f3c13 to 106f9e7 Compare May 14, 2026 20:54

longjinvan force-pushed the MDEV-39548 branch from 106f9e7 to 433b5cf Compare May 14, 2026 22:23

Uh oh!

Conversation

longjinvan commented May 13, 2026

Description

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

Copyright

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkodinov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants