mirror of
https://github.com/MariaDB/server.git
synced 2026-10-23 18:49:29 +03:00
dd896f05d9
`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 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. 2. `lock_external()` unwinds the tables it has already locked, because a later table refused -- 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 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. Rather than switch the check off wholesale again, only verify a table that this handle both holds a lock on and has changed under it: - `HP_INFO::lock_type` remembers the `ha_heap::external_lock()` argument, the way `MARIA_HA` and `MI_INFO` already do; - `HP_INFO::changed` is set by `heap_write()`, `heap_update()` and `heap_delete()`, and cleared by `ha_heap::external_lock()` on every grant, so it means "changed since this lock was taken"; - `table_is_locked_and_changed()` requires both. The change term is what separates the three unlock paths, because the lock type cannot: `ha_heap::external_lock()` records it before `thr_multi_lock()` runs, so it is armed on the two failing paths as well. Neither of them ever ran a row operation, so neither has changed anything. It has to be per handle rather than `HP_SHARE::changed`, which is true on exactly those paths, another connection being the one writing. Requiring a change also makes a debug build cheaper: the verification scans every record and every index, and now runs only after a statement that wrote to the table. Deriving this in the engine rather than repairing `lock_external()` also covers `ha_partition`, which reimplements the same unwind. A temporary table gets `F_EXTRA_LCK` and so counts as always locked: no other connection can reach its share. This covers the user's `CREATE TEMPORARY TABLE` and not only the optimizer's internal one -- an internal table frees its blob chains outright, whereas a user temporary table parks them, and `get_lock_data()` leaves it out of the lock set entirely, so it never reaches `external_lock()` at all. The `ALTER` copy target is temporary too, and additionally takes a direct `handler::ha_external_lock()` instead of going through the lock set. Redeeming a parked blob chain puts records back on the shared free list, so it needs the same protection, and both redemption points assert it. `hp_test_unlock_check-t` builds the lock states directly, in the order `ha_heap::external_lock()` builds them, so nothing here is raced. Four MTR tests cover the shapes it cannot reach: blob updates and deletes on a user `TEMPORARY` MEMORY table (`heap.blob_tmp_table`), `INSERT DELAYED` (`heap.blob_delayed_insert`), the `ALTER` copy target (`heap.blob_online_alter`), and one share locked twice in a lock set (`heap.blob_lock_twice`). No existing test exercised any of them.