MDEV-34848 Remove overhead from unnecessary MDL_context_owner inheritance

[kevdn]

Optimize MDL by using THD* directly instead of using abstract MDL_context_owner* interface, use THD* directly to avoid virtual function call overhead in MDL wait loops.

The MDL subsystem only interacts with THD objects, so using THD* directly eliminates unnecessary virtual function calls for enter_cond, exit_cond, is_killed, and notify_shared_lock.

[CLAassistant]

All committers have signed the CLA.

[Copilot]

Pull request overview

This PR refactors parts of the MDL/THD interaction by switching several APIs from the MDL_context_owner* abstraction to direct THD* usage.

Changes:

• Updated notify_shared_lock() to take THD* instead of MDL_context_owner* and adjusted call sites/implementation accordingly.
• Updated MDL_wait::timed_wait() and MDL_context ownership plumbing to use THD* directly.
• Simplified a few get_thd()-mediated field accesses (e.g., rgi_slave, thd_is_connected).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
sql/sql_class.h	Changes THD::notify_shared_lock() signature to accept THD*.
sql/sql_class.cc	Adjusts THD::notify_shared_lock() implementation for new parameter type.
sql/mdl.h	Updates MDL_context_owner::notify_shared_lock(), MDL_wait::timed_wait(), and MDL_context owner storage/accessors to use THD*.
sql/mdl.cc	Updates MDL_wait::timed_wait() implementation and related THD accesses for the new THD* owner type.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[FooBarrior]

I would actually prefer the opposite: architecturally, we should focus on decomposing thd, because it's bloated.

I understand though, that we might want to make de-architecturizing optimizations, if it's worth it.

If this is supposed to be a such, is there any benchmark demonstrating the commitment?

[vaintroub]

Agree with @FooBarrior . For the unmeasurable performance benefit of removing virtual function we'll plant this THD dependency everywhere. We should actually de-THD as much as we can, it is too important class, which purpose nobody can explain well.

[gkodinov]

Thank you for working on this! This is a preliminary review.

First, on the technicalities: Please squash your two commits into a single one and provide a commit message to it that compiles with CODING_STANDARDS.md.

Secondly, I kind of agree with the two developers that have commented that you need a strong case to remove layering like you did. A benchmark or profile info would help. And even in that case, there probably are alternatives to removing a base class for THD.

[svoj]

MDL_context_owner was implemented by mysql/mysql-server@b9766fe68bab, MySQL Bug #59309 with the purpose of "doing standalone unit testing of the MDL module". MariaDB doesn't adapt neither gunit test framework nor mdl tests specifically.

enter_cond(), exit_cond(), is_killed() are not owned by the MDL subsystem. notify_shared_lock() is used by MDL exclusively, but still it doesn't relate to MDL. All of them were historically part of THD. Having them encapsulated as MDL_context_owner is misleading.

Said the above, I see mysql/mysql-server@b9766fe68bab as a step towards a wrong direction and I'm happy to see it reverted.

I won't blindly buy the "performance benefit of removing virtual function". But still I see it as a valid encapsulation cleanup and it must make THD class 48(?) bytes smaller, which you guys are after.

[svoj]

Looks, nice. A few requests inline.

[svoj]

What kind of backward compatibility? No, please remove it altogether.

[svoj]

Please rename it to in_use, such that you don't have to update the rest of the method.

[svoj]

You claim that you de-virtualized MDL_context_owner methods, but you didn't. You have to remove inheritance from MDL_context_owner and remove override keyword from all relevant methods.