MDEV-40591 Unexpected ER_NOT_KEYFILE or MSAN error in heap_check_heap

[arcivanov]

Problem

ha_heap::external_lock() verifies the table with heap_check_heap() at F_UNLCK. That is safe on the ordinary unlock path, where mysql_unlock_tables() calls unlock_external() before thr_multi_unlock() and the THR_LOCK is still held. It is not safe on either path that unlocks after a failed lock attempt, where the caller holds nothing at all while another connection is writing:

1. mysql_lock_tables() calls unlock_external() to balance the external locks it already took, because thr_multi_lock() timed out (sql/lock.cc:403-404 -- frame Add executable bit to scripts that are supposed to have it. #5 of the reported MSAN stack).
2. lock_external() unwinds the tables it has already locked, because a later table refused (sql/lock.cc:443-452) -- all before thr_multi_lock() runs at all. ha_partition::external_lock() unwinds its partitions the same way.

MEMORY has no row-level concurrency control, so a scan taken outside the THR_LOCK sees a writer's intermediate state by construction: hp_alloc_from_tail() publishes total_records at allocation time, before the slot is written, while the checker scans [0, total_records + deleted) and reads every slot's flags byte. Under MSAN that is a use of uninitialised my_malloc() memory; otherwise it is a spurious total_records mismatch.

heap_check_heap() ends with heap_mark_crashed(), which sets HEAP_STATE_CRASHED in the shared HP_SHARE, so one bogus mid-write observation poisons a healthy table for every connection using it -- the reported ER_NOT_KEYFILE.

MDEV-21373 disabled this check in 2021 for exactly this reason, by gating it on EXTRA_DEBUG. MDEV-38975 changed the gate to EXTRA_HEAP_DEBUG and defined that for every debug build, reviving the race.

Fix

Rather than switch the check off wholesale again, ask whether the handle actually holds the lock. The requested lock type cannot answer that on its own, because ha_heap::store_lock() records it at get_lock_data() time, before anything is locked: on path 2 it is set while nothing is held. So HEAP records the grant itself.

path	granted	type	held?
mysql_unlock_tables -> unlock_external before thr_multi_unlock	1	set	yes
thr_multi_lock failed, never granted	0	TL_UNLOCK	no
thr_multi_lock failed after granting, rolled back by thr_unlock	1	TL_UNLOCK	no
lock_external unwind, before any lock attempt	0	set	no

Both terms are load-bearing, so hp_lock_is_held() is lock_granted && lock.type != TL_UNLOCK.

Deriving this in the engine rather than repairing lock_external() also covers ha_partition, which reimplements the same unwind and bypasses sql/lock.cc entirely.

The verification is gated on hp_may_check_heap_on_unlock(), and the redemption of parked blob chains -- which puts records back on the shared free list -- is gated on hp_lock_is_held() too.

ha_heap::reset() gains an assertion that nothing is parked without the lock. It cannot be violated: chains are parked only by heap_delete() and heap_update(), only for a table that is not internal (an internal one is never binlogged, so it frees its chains outright), and always under the lock; they are redeemed before it is released -- by external_lock(F_UNLCK) for an ordinary statement, and by heap_reset() itself under LOCK TABLES, reached from mark_used_tables_as_free_for_reuse() with the lock still held.

Testing

hp_test_unlock_check-t (new, 15 assertions, ~2 s) reproduces all of this deterministically, by driving thr_multi_lock()/thr_multi_unlock() directly instead of racing. A holder thread takes TL_WRITE and keeps it until told to let go, so the contending request is guaranteed to time out, and it parks the share in the state a writer passes through mid-row so the verification has something to wrongly find. It covers all four rows of the table above, and asserts that the table is provably consistent once the writer finishes -- i.e. that what was removed was a false positive, not a real detection.

• unit-test-first: the plumbing was landed with the predicate unchanged, the new assertion failed, and the one-line predicate change turned it green with the test untouched
• all 10 heap unit tests pass
• full main+heap MTR sweep: 1426/1426, no retries
• the new assertion did not fire across ~2850 debug-build test executions
• the original report reproduced at 2/20 and 6/25 repeats before the fix, and 40/40 after, with CHECK TABLE reporting status OK every time

[gkodinov]

Thanks for fixing this! This is a preliminary review.

LGTM. Please stand by for the final review.