MDEV-38056 Assertion 'bpage->state() >= buf_page_t::UNFIXED' in buf_p…

[iMineLink]

…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. In row_check_index() the freeze spans the whole comparison, which
fetches one field at a time and may evaluate a virtual column expression; for
the latest version it is also confined to a delete-marked record, the only
kind that need not own what it points at. 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; trx_undo_prev_version_build()
states the condition and why testing the oldest writer applied suffices.

The first two callers can treat that test as an invariant and end the walk
where it fails. row_check_index() cannot, because it decides reachability from
the lagging purge_sys.end_view on purpose, and that lag is how it finds orphan
secondary index records: where the test fails it stops and reports nothing
rather than treating the record as an orphan, and no report is lost for good,
end_view eventually reaching the same point. Its two purge_sys.is_purgeable()
tests now read the frozen view wherever a fetch follows, which makes them
atomic with the fetch they guard.

trx_undo_prev_version_build(): 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.

row_log_table_get_pk(): document why the online ALTER path may dereference
without a freeze.

Debug-only keywords. purge_hold_cleanup parks a purge batch between its last
purged record and purge_sys_t::batch_cleanup(), the window in which a reader
that goes by purge_sys.end_view can still reach history the batch has removed;
a batch opens and closes it without ever returning to the test.
purge_no_blob_freeze sends a version that does have externally stored columns
down the path one without any takes, which is what all four call sites did
before this change, and reproduces the assertion above.
row_vers_impl_x_locked_purgeable, row_undo_mod_sec_is_unsafe_purgeable and
row_check_index_purgeable force the re-validation to fail, reaching exits that
purge_sys.view advancing mid-walk otherwise produces. The first reports no
implicit lock for a row that a live transaction still holds, so a test may
only check that nothing breaks; the other two make the server more cautious.

Tests. old_blob and old_blob_updel cover the implicit lock check,
old_blob_rollback the rollback, old_blob_check 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 and needs purge_hold_cleanup
to reach it; old_blob_rollback parks at a reference that the version merely
inherited. All four fail with the original assertion under
debug_dbug=+d,purge_no_blob_freeze, and skip above a 16k page size, which
ROW_FORMAT=COMPRESSED requires. old_blob_purgeable drives the three
re-validation exits, needs no synchronisation, and runs at every page size.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Copilot]

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.view across 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.

[Thirunarayanan]

Did you consider online ALTER path (for deferencing?) row_log_table_get_pk()?
I haven't reviewed the test case yet.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

[iMineLink]

Thanks for the review, I addressed it in the latest commit, which adds as well a DBUG keyword for negative control which should allow to disable the fix and get the crash:

./build/Debug/mysql-test/mtr --mem --force --max-test-fail=0 --mysqld=--debug-dbug=+d,purge_no_blob_freeze innodb.old_blob innodb.old_blob_updel innodb.old_blob_rollback innodb.old_blob_check

row_log_table_get_pk() should be safe as purge cannot operate on the to-be-dereferenced blobs in that case.

[dr-m]

This touches MVCC and purge and adds some comments. I did not look at the test cases or the actual fix yet. For now, my only complaint is about duplicated assertions.

[dr-m]

Very good work! I did not run the tests yet. Would I have to apply some instrumentation patch in order to run the tests without the code fix? Alternatively, which parts should be reverted to just revert the bug fixes?

[iMineLink]

Thank you @dr-m. To reproduce the crashes the purge_no_blob_freeze keyword can be used as in example below:

./build/Debug/mysql-test/mtr --mem --force --max-test-fail=0 --mysqld=--debug-dbug=+d,purge_no_blob_freeze innodb.old_blob innodb.old_blob_updel innodb.old_blob_rollback innodb.old_blob_check

All tests will fail with assertion in buf_page_get_zip().

I'm addressing the comments.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.