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.

hp_may_check_heap_on_unlock() gates the verification on that.

The grant is set, not counted

thr_lock() does not call get_status once per ha_heap::external_lock(). A delayed insert is granted TL_WRITE_DELAYED and calls it a second time on the same request when thr_upgrade_write_delay_lock() promotes that to a real write lock, with no external_lock() in between -- mysys/thr_lock.c says so on the queued branch: "We don't have to do get_status here as we will do it when we change the delayed lock to a real write lock." HEAP is eligible because it advertises HA_CAN_INSERT_DELAYED.

Counting would therefore leave a permanent residue (2+ increments, 1 decrement per delayed cycle) and the second INSERT DELAYED aborts on Assertion '!hp_lock_is_held(file)' failed. Setting is idempotent, so N grants and one release are correct by construction.

Redeeming parked blob chains: the condition is privacy, not the lock

Redemption puts records back on the shared free list, so it needs the same protection -- but what makes it safe is that no other connection can reach the share, and holding the THR_LOCK is only one way to get there. Two handles are private without ever holding it:

• A user CREATE TEMPORARY TABLE ... ENGINE=MEMORY parks, because the parking gate is HP_SHARE::internal (HA_OPEN_INTERNAL_TABLE, the optimizer's own table) while lock-set membership is TABLE_SHARE::tmp_table -- get_lock_data() drops every non-transactional TEMPORARY table entirely, so it never runs store_lock/external_lock/thr_multi_lock at all.
• copy_data_between_tables() locks the ALTER copy target with a direct handler::ha_external_lock() (sql/sql_table.cc:12641, released at :13219) instead of through the lock set, so thr_lock() never grants that handle anything even while an online ALTER replays concurrent deletes onto it.

So redemption stays unconditional in both ha_heap::external_lock(F_UNLCK) and ha_heap::reset() (zero behaviour change), and each asserts the property that makes it safe: a parked chain is either lock-protected or on a table no other connection can reach.

Testing

hp_test_unlock_check-t (new, 17 assertions, ~2 s) reproduces the lock states 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.

thr_multi_lock() sorts its request array by THR_LOCK address, so the test pins which share is contended rather than letting malloc decide; otherwise the granted-then-rolled-back row of the table above is only reached on some runs. Each conjunct of the predicate now has a dedicated killer: dropping lock.type != TL_UNLOCK fails assertion 13, dropping lock_granted fails assertion 16 -- both verified by mutation.

Four MTR tests, one per shape, all mutation-verified:

test	shape
heap.blob_tmp_table	blob UPDATE/DELETE on a user TEMPORARY MEMORY table
heap.blob_delayed_insert	the repeated get_status from INSERT DELAYED
heap.blob_online_alter	chains parked on the online-ALTER copy target
heap.blob_lock_twice	one share locked twice in a lock set, via two aliased entries

None of these shapes had any coverage before: of 46 heap-suite files, zero did a blob UPDATE or DELETE on a user TEMPORARY MEMORY table.

heap.blob_lock_twice is the only test anywhere in which a handle is locked, released, and locked again, so it is what makes the release edge observable: remove hp_lock_released()'s clear and the stale grant from the first cycle aborts the server on the second LOCK TABLE, at the acquire-side assertion. All four tests fail against that mutant.

• unit-test-first throughout: the plumbing was landed with the predicate unchanged so the new assertion failed, and the one-line predicate change turned it green with the test untouched
• all 10 heap unit tests pass, hp_test_unlock_check-t at 17/17
• full main+heap MTR sweep: 1433/1433, no retries
• 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 -- so what was removed was a false positive, not a real detection

[gkodinov]

Thanks for fixing this! This is a preliminary review.

LGTM. Please stand by for the final review.