MDEV-26057 Assertion `!vcol->v_indexes.empty() in trx_undo_log_v_idx - #5631
Conversation
|
|
iMineLink
left a comment
iMineLink
left a comment
There was a problem hiding this comment.
I failed in having the MTR test reproduce the assertion on a build with the storage/ hunks reverted: I get a timeout instead. Code change looks sensible but I hope the test could reproduce the issue un an unpatched build.
There was a problem hiding this comment.
🟢 Approval recommended
The race is addressed within the dictionary-latch critical section and covered by a targeted regression test, with only a minor annotation typo remaining.
Pull request overview
Fixes MDEV-26057 by atomically resetting stale index metadata during failed INPLACE ALTER rollback.
Changes:
- Resets
ord_partwhile holdingdict_sys.latch. - Removes obsolete committed-index filtering.
- Adds a synchronized concurrency regression test.
File summaries
| File | Description |
|---|---|
storage/innobase/row/row0merge.cc |
Performs atomic metadata cleanup. |
storage/innobase/handler/handler0alter.cc |
Removes delayed cleanup and obsolete parameter. |
storage/innobase/trx/trx0rec.cc |
Adds test synchronization. |
mysql-test/suite/innodb/t/innodb-virtual-columns-debug.test |
Reproduces concurrent rollback and DML. |
mysql-test/suite/innodb/r/innodb-virtual-columns-debug.result |
Records expected regression-test output. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4621f4e to
a97c81d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The index scan can preserve stale ord_part for FTS columns, and the PR contains substantial undocumented unrelated work.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 26/26 changed files
- Comments generated: 2
- Review effort level: Balanced
iMineLink
left a comment
iMineLink
left a comment
There was a problem hiding this comment.
I have only a change request on the test file, to make if fail with assertion if the fix is not included, and think Copilot's suggestion to use index->n_uniq in row_merge_col_is_indexed() is sound. Thanks.
Problem: ======== - Rollback of an INPLACE ALTER TABLE is executed while holding only a shared metadata lock on the table, so DML can run concurrently. rollback_inplace_alter_table() resets dict_col_t::ord_part in a critical section of its own, after row_merge_drop_indexes() already removed the aborted indexes from the dictionary cache and emptied dict_v_col_t::v_indexes. During this time, DML statement can see a virtual column with ord_part set and an empty v_indexes, which makes assert failure in trx_undo_report_insert_virtual(). Solution: ======== row_merge_reset_ord_part(): Added a function to reset dict_col_t::ord_part for the columns that are no longer a field of any index remaining in the dictionary cache. For virtual columns the decision is based on dict_v_col_t::v_indexes being empty, and no element is ever removed from that list. row_merge_drop_indexes(): Added a call to row_merge_reset_ord_part() in the branch that removes the indexes from the cache, in the same dict_sys.latch critical section. That branch is taken only when MDL_EXCLUSIVE is held or when this is the only handle to the table, so no concurrent DML can observe the intermediate state. In the lazy drop branch the indexes and their v_indexes entries stay in the cache and nothing is reset; that is done later, when the indexes are dropped while holding MDL_EXCLUSIVE. check_col_exists_in_indexes(): Removed the only_committed parameter, which no longer has any caller. row_quiesce_col_ord_part(): Added a function to get dict_col_t::ord_part and dict_col_t::max_prefix of a column from the committed indexes that are present in the dictionary cache. row_quiesce_write_table(): Write the row_quiesce_col_ord_part() return values to the .cfg file instead of the cached dict_col_t fields, because a rolled back ADD INDEX leaves ord_part set until the aborted index is removed by a later DDL, and max_prefix is never reset when an index is dropped, which makes IMPORT TABLESPACE reject the tablespace with a bogus schema mismatch.
a97c81d to
42038e1
Compare
iMineLink
left a comment
iMineLink
left a comment
There was a problem hiding this comment.
LGTM, thank you for addressing the review points.
Problem:
Solution:
row_merge_reset_ord_part(): Added a function to reset dict_col_t::ord_part for the columns that are no longer a field of any index remaining in the dictionary cache.
For virtual columns the decision is based on dict_v_col_t::v_indexes being empty, and no element is ever removed from that list.
row_merge_drop_indexes(): Added a call to row_merge_reset_ord_part() in the branch that removes the indexes from the cache, in the same dict_sys.latch critical section. That branch is taken only when MDL_EXCLUSIVE is held or when this is the only handle to the table, so no concurrent DML can observe the intermediate state. In the lazy drop branch the indexes and their v_indexes entries stay in the cache and nothing is reset; that is done later, when the indexes are dropped while holding MDL_EXCLUSIVE.
check_col_exists_in_indexes(): Removed the only_committed parameter, which no longer has any caller.