MDEV-38056 Assertion 'bpage->state() >= buf_page_t::UNFIXED' in buf_p… - #5473
Open
iMineLink wants to merge 1 commit into
Open
MDEV-38056 Assertion 'bpage->state() >= buf_page_t::UNFIXED' in buf_p…#5473iMineLink wants to merge 1 commit into
iMineLink wants to merge 1 commit into
Conversation
…age_get_zip() Three callers reconstruct old versions of a clustered index record, derive secondary index entries from them, and dereference the BLOB pointers of externally stored columns: row_vers_impl_x_locked_low() for the implicit lock check, row_undo_mod_sec_is_unsafe() for rollback, and row_check_index() for CHECK TABLE ... EXTENDED. purge_sys.view is what keeps those pages allocated, but purge_sys_t::view_guard froze it only for the duration of trx_undo_prev_version_build(), and the dereference happens after. On ROW_FORMAT=COMPRESSED this trips the assertion above; in a release build the freed page is read anyway, which is silent corruption. All three now hold purge_sys.latch across the dereference, and only where the version has externally stored columns, since row_build() dereferences nothing otherwise. Freezing that late means the view may have advanced since the walk decided a version was reachable, so each caller re-establishes that under the freeze, by testing the oldest writer whose undo log record it applied. That covers the newer ones: a newer version can only have been written once the older writer released the exclusive lock on the record. The first two can treat the test as an invariant, and end the walk if it fails. row_check_index() cannot: it decides reachability from purge_sys.end_view, which lags behind, and that lag is how it finds orphan secondary index records. It therefore stops where the test fails, as if the version chain had ended, which is how a version that can no longer be rebuilt is treated as well. Its two purge_sys.is_purgeable() tests now read the frozen view, which makes them atomic with the fetch they guard. trx_undo_prev_version(): remove the gate that was meant to stop CHECK TABLE ... EXTENDED from fetching BLOBs it may no longer own, and view_guard::is_extended() with it, which no guard mode could satisfy. That decision belongs to the caller, the only one that knows whether it will dereference anything. trx_purge(): a debug-only keyword that holds a batch open between its last purged record and purge_sys_t::batch_cleanup(), which is the window where a reader that goes by purge_sys.end_view can still reach history the batch has removed. innodb.old_blob_updel needs that window, and it is not otherwise addressable from a test, because it opens and closes within one batch. Tests: old_blob and old_blob_updel for the implicit lock check, old_blob_rollback for rollback, old_blob_check for CHECK TABLE ... EXTENDED. Each parks a walk at a dereference, makes the BLOB freeable, and asserts that the counter of purged update records, which is what would free it, stays at zero while parked and advances once the walk is over. old_blob_updel covers an undo log record that stores only the 20-byte reference; old_blob_rollback parks at a reference that the version merely inherited. All four fail with the original assertion when the freeze is removed, and skip above a 16k page size, which ROW_FORMAT=COMPRESSED requires.
|
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a race in InnoDB MVCC version-chain walks where callers rebuild old clustered-index record versions and then dereference externally stored (BLOB) columns after purge_sys.view may have advanced, which can trigger assertions on ROW_FORMAT=COMPRESSED and can lead to silent corruption in release builds. The change shifts responsibility to the callers to freeze purge_sys.view across any external-column dereference and to re-validate reachability under that freeze.
Changes:
- Ensure the three affected version-chain walks freeze
purge_sys.viewacross external-column dereference and re-check the “oldest writer applied” invariant under the freeze. - Remove the ineffective CHECK TABLE...EXTENDED “gate” from
trx_undo_prev_version()and clarify ownership/visibility rules in comments. - Add a debug-only purge hook (
purge_hold_cleanup) plus new mysql-tests that park at dereference points and assert purge does not free BLOB history while parked.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| storage/innobase/trx/trx0rec.cc | Updates trx_undo_prev_version() commentary and removes the (now-described as unsatisfiable) CHECK TABLE...EXTENDED gating logic so callers decide and enforce safe dereference. |
| storage/innobase/trx/trx0purge.cc | Adds a UNIV_DEBUG-only, capped delay controlled by purge_hold_cleanup to hold a purge batch open for testing the critical window. |
| storage/innobase/row/row0vers.cc | Freezes purge_sys.view only when external columns exist before row_build() dereference in implicit-lock checking. |
| storage/innobase/row/row0umod.cc | Freezes purge_sys.view across dereference in rollback safety logic and revalidates using the oldest-applied writer trx id. |
| storage/innobase/row/row0sel.cc | Makes CHECK TABLE...EXTENDED atomic w.r.t. purge by freezing purge_sys.view across purgeability tests + potential dereference, and stops extended walks when rebuildability can no longer be safely guaranteed. |
| storage/innobase/row/row0purge.cc | Clarifies (comment-only) that purge already runs with a frozen view for the batch. |
| storage/innobase/include/trx0purge.h | Removes view_guard::is_extended() and adds a debug assertion preventing purge_sys.latch re-entrancy for VIEW guards. |
| mysql-test/suite/innodb/t/old_blob.test | New regression test for implicit-lock check path holding the freeze across dereference. |
| mysql-test/suite/innodb/t/old_blob_updel.test | New regression test covering update-vs-delete-mark undo-record reference-only case, requiring an in-progress batch window. |
| mysql-test/suite/innodb/t/old_blob_rollback.test | New regression test for rollback path, including an inherited external reference case. |
| mysql-test/suite/innodb/t/old_blob_check.test | New regression test for CHECK TABLE...EXTENDED path with end_view lag and safe stop behavior. |
| mysql-test/suite/innodb/r/old_blob.result | Expected output for old_blob.test. |
| mysql-test/suite/innodb/r/old_blob_updel.result | Expected output for old_blob_updel.test. |
| mysql-test/suite/innodb/r/old_blob_rollback.result | Expected output for old_blob_rollback.test. |
| mysql-test/suite/innodb/r/old_blob_check.result | Expected output for old_blob_check.test. |
| mysql-test/suite/innodb/include/assert_blob_not_purged.inc | New helper include to assert purge did not advance the freeing counter while a walk is parked at a dereference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…age_get_zip()
Three callers reconstruct old versions of a clustered index record, derive secondary index entries from them, and dereference the BLOB pointers of externally stored columns: row_vers_impl_x_locked_low() for the implicit lock check, row_undo_mod_sec_is_unsafe() for rollback, and row_check_index() for CHECK TABLE ... EXTENDED. purge_sys.view is what keeps those pages allocated, but purge_sys_t::view_guard froze it only for the duration of trx_undo_prev_version_build(), and the dereference happens after. On ROW_FORMAT=COMPRESSED this trips the assertion above; in a release build the freed page is read anyway, which is silent corruption.
All three now hold purge_sys.latch across the dereference, and only where the version has externally stored columns, since row_build() dereferences nothing otherwise. Freezing that late means the view may have advanced since the walk decided a version was reachable, so each caller re-establishes that under the freeze, by testing the oldest writer whose undo log record it applied. That covers the newer ones: a newer version can only have been written once the older writer released the exclusive lock on the record.
The first two can treat the test as an invariant, and end the walk if it fails. row_check_index() cannot: it decides reachability from purge_sys.end_view, which lags behind, and that lag is how it finds orphan secondary index records. It therefore stops where the test fails, as if the version chain had ended, which is how a version that can no longer be rebuilt is treated as well. Its two purge_sys.is_purgeable() tests now read the frozen view, which makes them atomic with the fetch they guard.
trx_undo_prev_version(): remove the gate that was meant to stop CHECK TABLE ... EXTENDED from fetching BLOBs it may no longer own, and view_guard::is_extended() with it, which no guard mode could satisfy. That decision belongs to the caller, the only one that knows whether it will dereference anything.
trx_purge(): a debug-only keyword that holds a batch open between its last purged record and purge_sys_t::batch_cleanup(), which is the window where a reader that goes by purge_sys.end_view can still reach history the batch has removed. innodb.old_blob_updel needs that window, and it is not otherwise addressable from a test, because it opens and closes within one batch.
Tests: old_blob and old_blob_updel for the implicit lock check, old_blob_rollback for rollback, old_blob_check for CHECK TABLE ... EXTENDED. Each parks a walk at a dereference, makes the BLOB freeable, and asserts that the counter of purged update records, which is what would free it, stays at zero while parked and advances once the walk is over. old_blob_updel covers an undo log record that stores only the 20-byte reference; old_blob_rollback parks at a reference that the version merely inherited. All four fail with the original assertion when the freeze is removed, and skip above a 16k page size, which ROW_FORMAT=COMPRESSED requires.