Conversation
|
|
|
Hi @pranavktiwari, Thank you for the patch! Now I see that change for MDL_BIT has negative effect on performance which is unwanted. I'd better tell in the description that it is investigation task and should be evaluated for push. Nonetheless, if we rework It seems that I cannot recognize the changes for the second part of the ticket (about |
We have 16 bits and we have enums till 15 (0 to 15) even after increment. So if I leave MDL_BIT unchanged (do not subtract 1), it will work as bit masking is supported and in the range.
…_KEY. 2- Modified different arrays due to the new value added in the enum. 3- enums are used to calculate the hash and due to a new enum, hash values have changed and this causes ordering issue in the MDL_locks store. Subtracted one from enum where hash is computed to stick to oringal values of the hash.
Adding NOT_INITIALIZED=0 shifted all existing namespace enum values by +1, changing their hash_value() results. Since m_locks uses a Split-Ordered List (lock-free hash table) which maintains a single linked list sorted by my_reverse_bits(hash_value()), even a +1 shift causes completely different bit-reversed positions, changing iteration order in metadata_lock_info. Fix: Updated result file to match new iteration order instead of using the fragile -1 workaround in hash_value(), since iteration order is an internal implementation detail of the lock-free hash table and has no impact on MDL correctness.
There was a problem hiding this comment.
Pull request overview
This PR reworks MDL enum constant values so that 0 represents an uninitialized MDL state (instead of mapping to real lock types), and updates related mappings/output so metadata lock reporting continues to work.
Changes:
- Shifted
enum_mdl_type/ backup-stage lock constants soMDL_NOT_INITIALIZEDbecomes0. - Introduced
MDL_key::NOT_INITIALIZED=0namespace and updated handlers/mappings accordingly. - Updated incompatibility matrices, metadata-lock name tables, and adjusted mysql-test expected results.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/sp.h | Adds handling for MDL_key::NOT_INITIALIZED in namespace→handler switch. |
| sql/mdl.h | Updates MDL type and namespace enums and shifts backup lock constants. |
| sql/mdl.cc | Adds NOT_INITIALIZED PSI stage entry, pads name tables, and pads incompatibility arrays. |
| plugin/metadata_lock_info/metadata_lock_info.cc | Adds “Not initialised” entry to namespace name table. |
| mysql-test/main/mdl_sync.result | Updates expected output ordering for MDL metadata_lock_info. |
| mysql-test/main/mdl.result | Updates expected output ordering for MDL metadata_lock_info. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sql/mdl.cc
Outdated
|
|
||
| PSI_stage_info MDL_key::m_namespace_to_wait_state_name[NAMESPACE_END]= | ||
| { | ||
| {0, "Not initilised", 0}, |
There was a problem hiding this comment.
The new stage name string has a spelling error: "Not initilised". This ends up visible in processlist/performance schema stage output; please fix the typo (and ideally keep spelling consistent with the information_schema.metadata_lock_info strings).
| {0, "Not initilised", 0}, | |
| {0, "Not initialised", 0}, |
sql/mdl.cc
Outdated
|
|
||
| static const LEX_STRING lock_types[]= | ||
| { | ||
| {}, |
There was a problem hiding this comment.
The 0th entry in lock_types is now an empty LEX_STRING (str==NULL). If any code path ends up calling get_mdl_lock_name()/get_type_name() with MDL_NOT_INITIALIZED (e.g., debug logging on an uninitialized MDL_request), this can lead to undefined behavior when printing "%s". Consider providing a real name like "MDL_NOT_INITIALIZED" instead of an empty initializer to keep it safe and debuggable.
| {}, | |
| { C_STRING_WITH_LEN("MDL_NOT_INITIALIZED") }, |
sql/mdl.cc
Outdated
|
|
||
| static const LEX_STRING backup_lock_types[]= | ||
| { | ||
| {}, |
There was a problem hiding this comment.
Same issue as lock_types[]: backup_lock_types[0] is now an empty LEX_STRING (str==NULL). If a backup-namespace MDL_request/ticket is ever inspected or logged with type==0, printing the name can dereference/format a NULL string. Using an explicit "MDL_NOT_INITIALIZED" (or similar) entry at index 0 would avoid surprises.
| {}, | |
| { C_STRING_WITH_LEN("MDL_BACKUP_NOT_INITIALIZED") }, |
Incorporated code review comments of copilot.
fixes MDEV-39184
Problem:
MDL constants values of 0 assign some default values (MDL_INTENTION_EXCLUSIVE, BACKUP) before MDL_REQUEST_INIT(). That does not make sense if MDL_REQUEST_INIT() was not called. Please rework the values of 0 into (MDL_NOT_INITIALIZED, NOT_INITIALIZED). Described by these comments in the code:
Fix
1- Increment all values in enum_mdl_type by 1.
2- Add a 0 at the start of the incompatibility arrays so the enum indices continue to match their corresponding bitmasks.
This ensures that uninitialized MDL constants are correctly handled while preserving existing bitmask mappings.