MDEV-34986: storage/innobase/dict/dict0dict: add a RAII class to freeze a dict_sys_t

[MaxKellermann]

https://jira.mariadb.org/browse/MDEV-34986

Description

This replaces a lot of manual freeze() and unfreeze() call. Doing it with RAII is safer and easier.

I did not replace all freeze()/unfreeze() pairs because some callers unfreeze and re-freeze in the middle of a scope. Sometimes, adding a new scope can be added just for such a RAII object. Refactoring that can be done later.

Notes:

• Instead of using the global variable dict_sys, I decided to pass a
  dict_sys_t reference parameter, because I believe it will be
  necessary to eliminate that global variable eventually (in order to
  have a per-catalog instance). Hard-coding this global variable here
  would generate identical (not better) machine code and would be a
  step in the wrong direction.

• The new macros SRW_LOCK_ARGS2 and SRW_LOCK_CALL2 were necessary
  because this is the first time those debug-only parameters are
  forwarded from a function that has more parameters (i.e. the
  dict_sys_t reference).

Release Notes

Nothing. Internal code change only with no runtime effect.

How can this PR be tested?

No runtime effect.

Basing the PR against the correct MariaDB version

• This is a new feature or a refactoring, and the PR is based against the main branch.
• This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

• I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
• For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

[CLAassistant]

All committers have signed the CLA.

[MaxKellermann]

CI failures due to a dumb typo; I force-pushed an amended version.

[dr-m]

Thank you for your contribution. It was great to hear your related talk in Berlin last Tuesday. It is a pity that we did not have a chance to talk after that. I did not have a chance to check the related iovisor/bcc#5108 yet.

With @montywi’s current (not yet published) implementation of catalogs, there will continue to be a single global InnoDB infrastructure, which would include the singleton dict_sys object for which I removed pointer indirection in 5fd7502.

For catalogs or multi-tenancy to make any sense, I think that at the minimum, the buffer pool must be shared. I understood that implementations in other RDBMS may have per-tenant file structure, such as separate write-ahead, transaction metadata and data dictionary for each tenant. That kind of a solution would require pushing down a catalog or tenant ID to the lower level, such as the buffer pool and mini-transactions. It could incur a considerable overhead for the case that there is only a single tenant or catalog. It would also be more complex to implement than the what @montywi currently has shared with your organization.

Long story short, I think that the singleton dict_sys object is there to stay.

The bug that was featured in your talk would not have been prevented by this refactoring. The innodb_sync_debug that was removed in db006a9 and ff5d306 might have. 5f2dcd1 improved the situation a little, by allowing us to keep track of all srw_lock::rd_lock() holders and to add debug assertions that a particular latch is not being held in shared (or any) mode by the current thread.

The fix b2654ba moves some logic that used to be duplicated into a non-inline function lock_table_children(). The problem was not that we’d forget to release a latch; the problem was that we were holding a latch while entering a wait for a transactional lock, which may block for a very long time (by default, or 50 seconds), even indefinitely. All this because FOREIGN KEY constraints are being handled at a too low level, or because there are two metadata caches that are covered by slightly different locks: TABLE_SHARE and dict_table_t.

I think that there can be some benefit of having RAII around synchronization primitives. We are using it in b08448d and a variant of it in mtr_t::m_memo. But I am not fully convinced about the usefulness of this change. If we went for it, I would like to assess if it has any impact on the generated code.

[MaxKellermann]

The bug that was featured in your talk would not have been prevented by this refactoring.

Of course not! This is just a source-code level change with no runtime effect. It helps keeping the code readable; and of course RAII is safer, less prone to human mistakes.

While MariaDB is formally written in C++, it could go much further to take advantage of C++'s features to make it safer. When I read the code to try to understand the dict_sys.latch problem last week, I kept wondering why it was that way (so many raw pointers, manual resource/lock management) - too error prone for my taste.
This PR is a tiny step in that direction; my suggestion to do more C++.

[MaxKellermann]

If we went for it, I would like to assess if it has any impact on the generated code.

The generated code is mostly identical. In a perfect world, all of this would collapse to exactly identical machine code; but there are compiler imperfections.

For example, GCC is a bit clumsy about the trivial method ha_innobase::referenced_by_foreign_key() and adds one extra instruction for no obvious reason.

Other functions may grow larger because RAII gives you stronger guarantees; i.e. the generated code is a bit longer, but actually better than before. RAII guarantees that the destructor will be called even if an exception was thrown. That requires a tiny bit of extra code for stack unwinding.

People have different opinions about C++ exceptions. For my own code, I'm more than willing to pay the tiny price for this stack unwinding code (with overhead that is hardly even measurable). But fact about MariaDB is that C++ exceptions are not disabled, therefore you are already paying that price. Exceptions are indeed used in very few places, but 99% of the code misses noexcept declarations, which forces the compiler to make everything stack-unwinding-safe, even where it's never necessary. (Of course, adding noexcept does have disadvantages of its own. Oh yes, that perfect world, I'd like to have it.)

My point is: the code is not 100% identical, but it is either

• better: due to stronger guarantees (that may have actual runtime effects because you now get exception-safety for free)
• negligible overhead due to compiler clumsiness

[MaxKellermann]

With this PR, the executable grows by 572 bytes.

But if I apply #3531 first, then this PR adds only 256 bytes, because the compiler can now omit some of the unnecessary stack unwinding safety.

[dr-m]

Would the executable size grow less if you did not add any data member to your RAII guard, but instead made it refer directly to the singleton dict_sys, similar to how LockMutexGuard and LockGuard are referring to lock_sys.latch?

[MaxKellermann]

Would the executable size grow less if you did not add any data member to your RAII guard, but instead made it refer directly to the singleton dict_sys, similar to how LockMutexGuard and LockGuard are referring to lock_sys.latch?

In all cases I analyzed in the disassembly, this reference was optimized away by GCC, because GCC knew it was guaranteed to always point to the global variable.

There could be optimization pitfalls in other/similar cases, though: if this was not a global object, but a global pointer to an object, GCC would not be disallowed to optimize it away because the pointer value could change meanwhile. But in that case, the generated code would still be better because GCC would (likely) cache the pointer in a register instead of having to access RAM again to reload the pointer.

Edit: I just verified - the binary is exactly identical whether I use the global dict_sys variable or store a dict_sys_t reference in the RAII class.

[dr-m]

Edit: I just verified - the binary is exactly identical whether I use the global dict_sys variable or store a dict_sys_t reference in the RAII class.

Thank you. With clang (which is the compiler on FreeBSD and macOS) it might be different, or maybe they have improved in recent releases.

By the way, while the dict_sys.latch is global, it is being used much less than its precursors dict_sys.mutex and dict_operation_lock used to be. In most cases, table metadata is being protected by reference counts or metadata locks (MDL, a lock on the name). Similarly, InnoDB data dictionary tables are being protected by transactional table locks.

The purpose of MDL or the old use of "global MDL" as in dict_sys.latch is to prevent a race condition between DDL and any concurrent operations on the table. Because the layer above InnoDB is largely ignorant about FOREIGN KEY, InnoDB is forced to acquire dict_sys.latch. Things could be much cleaner if the metadata storage was moved closer to the MDL logic, but implementing that is far from trivial.

[MaxKellermann]

I added a JIRA ticket. Do you want me to rebase this PR on a different branch?