From f2c88afdbd8322654255b8ba281c91b967cc3a39 Mon Sep 17 00:00:00 2001 From: Arcadiy Ivanov Date: Sun, 2 Aug 2026 21:05:46 -0400 Subject: [PATCH] MDEV-40591 Unexpected ER_NOT_KEYFILE or MSAN error in heap_check_heap `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. 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 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. 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 the second path it is set while nothing is held. So HEAP now records the grant itself: - `hp_lock_granted()`, registered as the `THR_LOCK` `get_status` callback, sets `HP_INFO::lock_granted` when `thr_lock()` gives the lock to the handle; - `hp_lock_released()` clears it from `ha_heap::external_lock(F_UNLCK)`, which per the description in `sql/lock.cc` the SQL layer reaches while the THR_LOCK is still held, just before `thr_multi_unlock()`; - `hp_lock_is_held()` requires both the grant and a lock type that `thr_unlock()` has not reset, the latter covering the lock that `thr_multi_lock()` takes and then rolls back when a later table times out. The grant is set rather than 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. A count would keep the surplus for the life of the handle; setting is idempotent. Deriving this in the engine rather than repairing `lock_external()` also covers `ha_partition`, which reimplements the same unwind. `hp_may_check_heap_on_unlock()` gates the verification on that. Redeeming a parked blob chain puts records back on the shared free list, so it needs the same protection -- but the condition is that no other connection can reach the share, and holding the THR_LOCK is only one way to satisfy it. Chains are parked only by `heap_delete()` and `heap_update()`, and only for a table that is not internal, which is not the set the server locks: `share->internal` is `HA_OPEN_INTERNAL_TABLE`, the optimizer's own temporary table, whereas `get_lock_data()` leaves every non-transactional `TEMPORARY` table out of the lock set entirely. A user `TEMPORARY` MEMORY table therefore parks while holding no THR_LOCK, and owns its chains alone. `copy_data_between_tables()` reaches the same state from the other direction: it locks the `ALTER` copy target with a direct `handler::ha_external_lock()` instead of through the lock set, so `thr_lock()` never grants that handle anything even while an online `ALTER` replays concurrent deletes onto it. Both are private to one session, so `ha_heap::external_lock(F_UNLCK)` and `ha_heap::reset()` keep redeeming unconditionally, and each asserts the property that makes that safe: a parked chain is either lock-protected or on a table no other connection can reach. `hp_test_unlock_check-t` reproduces the lock states deterministically, by driving `thr_multi_lock()`/`thr_multi_unlock()` directly instead of racing. Four MTR tests cover the shapes it cannot reach: blob updates and deletes on a user `TEMPORARY` MEMORY table (`heap.blob_tmp_table`), the repeated `get_status` (`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`) -- the last being the only place a handle is locked, released and locked again, which is what makes the release edge observable. No existing test exercised any of them. --- include/heap.h | 1 + mysql-test/suite/heap/blob_delayed_insert.opt | 1 + .../suite/heap/blob_delayed_insert.result | 51 +++ .../suite/heap/blob_delayed_insert.test | 76 ++++ mysql-test/suite/heap/blob_lock_twice.result | 59 +++ mysql-test/suite/heap/blob_lock_twice.test | 69 ++++ .../suite/heap/blob_online_alter.result | 26 ++ mysql-test/suite/heap/blob_online_alter.test | 47 +++ mysql-test/suite/heap/blob_tmp_table.result | 95 +++++ mysql-test/suite/heap/blob_tmp_table.test | 78 ++++ storage/heap/CMakeLists.txt | 2 +- storage/heap/ha_heap.cc | 75 +++- storage/heap/heapdef.h | 74 ++++ storage/heap/hp_create.c | 26 ++ storage/heap/hp_open.c | 2 +- storage/heap/hp_test_unlock_check-t.c | 342 ++++++++++++++++++ 16 files changed, 1013 insertions(+), 11 deletions(-) create mode 100644 mysql-test/suite/heap/blob_delayed_insert.opt create mode 100644 mysql-test/suite/heap/blob_delayed_insert.result create mode 100644 mysql-test/suite/heap/blob_delayed_insert.test create mode 100644 mysql-test/suite/heap/blob_lock_twice.result create mode 100644 mysql-test/suite/heap/blob_lock_twice.test create mode 100644 mysql-test/suite/heap/blob_online_alter.result create mode 100644 mysql-test/suite/heap/blob_online_alter.test create mode 100644 mysql-test/suite/heap/blob_tmp_table.result create mode 100644 mysql-test/suite/heap/blob_tmp_table.test create mode 100644 storage/heap/hp_test_unlock_check-t.c diff --git a/include/heap.h b/include/heap.h index 7d326c4988762..d3eb4efa4e9c1 100644 --- a/include/heap.h +++ b/include/heap.h @@ -219,6 +219,7 @@ typedef struct st_heap_info my_bool implicit_emptied; my_bool has_zerocopy_blobs; /* Last hp_read_blobs produced zero-copy ptrs */ my_bool has_pending_blob_free; /* pending_blob_chains awaits freeing */ + my_bool lock_granted; /* thr_lock() gave `lock' to this handle */ THR_LOCK_DATA lock; LIST open_list; } HP_INFO; diff --git a/mysql-test/suite/heap/blob_delayed_insert.opt b/mysql-test/suite/heap/blob_delayed_insert.opt new file mode 100644 index 0000000000000..789275fa25e27 --- /dev/null +++ b/mysql-test/suite/heap/blob_delayed_insert.opt @@ -0,0 +1 @@ +--skip-log-bin diff --git a/mysql-test/suite/heap/blob_delayed_insert.result b/mysql-test/suite/heap/blob_delayed_insert.result new file mode 100644 index 0000000000000..b869012ce137d --- /dev/null +++ b/mysql-test/suite/heap/blob_delayed_insert.result @@ -0,0 +1,51 @@ +CREATE TABLE t1 (a INT, b BLOB) ENGINE=MEMORY; +INSERT DELAYED INTO t1 VALUES (1, REPEAT('x', 300)), (2, REPEAT('y', 300)); +SELECT VARIABLE_VALUE > 0 AS delayed_thread_ran +FROM information_schema.global_status +WHERE VARIABLE_NAME = 'DELAYED_WRITES'; +delayed_thread_ran +1 +INSERT DELAYED INTO t1 VALUES (3, REPEAT('z', 300)), (4, REPEAT('w', 300)); +connect reader,localhost,root,,test; +connection default; +INSERT DELAYED INTO t1 VALUES (5, REPEAT('v', 300)); +connection reader; +SELECT COUNT(*) >= 0 AS reader_ran FROM t1; +reader_ran +1 +connection default; +INSERT DELAYED INTO t1 VALUES (6, REPEAT('u', 300)); +disconnect reader; +SELECT a, LENGTH(b) FROM t1; +a LENGTH(b) +1 300 +2 300 +3 300 +4 300 +5 300 +6 300 +CHECK TABLE t1; +Table Op Msg_type Msg_text +test.t1 check status OK +INSERT INTO t1 VALUES (7, REPEAT('t', 300)); +UPDATE t1 SET b = REPEAT('s', 400) WHERE a = 7; +DELETE FROM t1 WHERE a = 7; +CHECK TABLE t1; +Table Op Msg_type Msg_text +test.t1 check status OK +FLUSH TABLES; +INSERT INTO t1 VALUES (8, REPEAT('r', 300)); +UPDATE t1 SET b = REPEAT('q', 400) WHERE a = 8; +DELETE FROM t1 WHERE a = 8; +CHECK TABLE t1; +Table Op Msg_type Msg_text +test.t1 check status OK +SELECT a, LENGTH(b) FROM t1; +a LENGTH(b) +1 300 +2 300 +3 300 +4 300 +5 300 +6 300 +DROP TABLE t1; diff --git a/mysql-test/suite/heap/blob_delayed_insert.test b/mysql-test/suite/heap/blob_delayed_insert.test new file mode 100644 index 0000000000000..eeb29b77c27e6 --- /dev/null +++ b/mysql-test/suite/heap/blob_delayed_insert.test @@ -0,0 +1,76 @@ +# +# The record of a THR_LOCK grant survives an INSERT DELAYED lock cycle. +# +# thr_lock() does not call the grant callback once per external_lock(): a +# delayed insert is granted TL_WRITE_DELAYED and then calls it again on the +# same request when thr_upgrade_write_delay_lock() turns that into a real +# write lock, with no external_lock() in between. A handle that counted +# grants would keep the surplus for its whole life and then report a lock it +# does not hold on every later statement. +# +# The binary log must be off: with statement binlogging INSERT DELAYED is +# downgraded to an ordinary write lock and the delayed thread never runs. +# + +CREATE TABLE t1 (a INT, b BLOB) ENGINE=MEMORY; + +INSERT DELAYED INTO t1 VALUES (1, REPEAT('x', 300)), (2, REPEAT('y', 300)); + +--let $wait_condition= SELECT COUNT(*) = 2 FROM t1 +--source include/wait_condition.inc + +# The delayed thread really did the writing, rather than the statement being +# silently downgraded to a plain INSERT. +SELECT VARIABLE_VALUE > 0 AS delayed_thread_ran + FROM information_schema.global_status + WHERE VARIABLE_NAME = 'DELAYED_WRITES'; + +# Second cycle on the still-live delayed thread: it locks the same handle +# again, which is where a leaked grant is noticed. +INSERT DELAYED INTO t1 VALUES (3, REPEAT('z', 300)), (4, REPEAT('w', 300)); + +--let $wait_condition= SELECT COUNT(*) = 4 FROM t1 +--source include/wait_condition.inc + +# A third cycle with a concurrent reader, so the delayed thread has to give +# the lock up and take it again mid-batch. +connect (reader,localhost,root,,test); + +connection default; +INSERT DELAYED INTO t1 VALUES (5, REPEAT('v', 300)); + +connection reader; +SELECT COUNT(*) >= 0 AS reader_ran FROM t1; + +connection default; +INSERT DELAYED INTO t1 VALUES (6, REPEAT('u', 300)); + +--let $wait_condition= SELECT COUNT(*) = 6 FROM t1 +--source include/wait_condition.inc + +disconnect reader; + +--sorted_result +SELECT a, LENGTH(b) FROM t1; +CHECK TABLE t1; + +# Ordinary lock cycles on the same table afterwards. These write blobs, so +# they park and redeem chains, which a handle wrongly believing it holds the +# lock would do outside it. +INSERT INTO t1 VALUES (7, REPEAT('t', 300)); +UPDATE t1 SET b = REPEAT('s', 400) WHERE a = 7; +DELETE FROM t1 WHERE a = 7; +CHECK TABLE t1; + +# And once more after the table is reopened, so the same share is reached +# through a fresh handle. +FLUSH TABLES; +INSERT INTO t1 VALUES (8, REPEAT('r', 300)); +UPDATE t1 SET b = REPEAT('q', 400) WHERE a = 8; +DELETE FROM t1 WHERE a = 8; +CHECK TABLE t1; + +--sorted_result +SELECT a, LENGTH(b) FROM t1; + +DROP TABLE t1; diff --git a/mysql-test/suite/heap/blob_lock_twice.result b/mysql-test/suite/heap/blob_lock_twice.result new file mode 100644 index 0000000000000..a51a55bbfaf28 --- /dev/null +++ b/mysql-test/suite/heap/blob_lock_twice.result @@ -0,0 +1,59 @@ +CREATE TABLE ht (a INT, b BLOB) ENGINE=MEMORY; +CREATE TABLE mi (a INT, b BLOB) ENGINE=MyISAM; +INSERT INTO ht VALUES (1,'foo'),(2,'bar'); +INSERT INTO mi VALUES (1,'foo'),(2,'bar'); +# ===== single aliased entry, referenced as "table AS alias" ===== +# -- MyISAM +LOCK TABLE mi AS m1 WRITE; +SELECT COUNT(*) FROM mi AS m1; +COUNT(*) +2 +UPDATE mi AS m1 SET b='x' WHERE a=1; +# the unaliased name is not locked +SELECT COUNT(*) FROM mi; +ERROR HY000: Table 'mi' was not locked with LOCK TABLES +UNLOCK TABLES; +# -- MEMORY +LOCK TABLE ht AS h1 WRITE; +SELECT COUNT(*) FROM ht AS h1; +COUNT(*) +2 +UPDATE ht AS h1 SET b='x' WHERE a=1; +# the unaliased name is not locked +SELECT COUNT(*) FROM ht; +ERROR HY000: Table 'ht' was not locked with LOCK TABLES +UNLOCK TABLES; +# ===== two aliased entries, one WRITE and one READ ===== +# -- MyISAM +LOCK TABLE mi AS m1 WRITE, mi AS m2 READ; +SELECT COUNT(*) FROM mi AS m2; +COUNT(*) +2 +UPDATE mi AS m1 SET b='y' WHERE a=1; +# INSERT takes no alias, so it cannot reach an aliased lock at all +INSERT INTO mi SELECT a+10, b FROM mi AS m2; +ERROR HY000: Table 'mi' was not locked with LOCK TABLES +UNLOCK TABLES; +# -- MEMORY +LOCK TABLE ht AS h1 WRITE, ht AS h2 READ; +SELECT COUNT(*) FROM ht AS h2; +COUNT(*) +2 +UPDATE ht AS h1 SET b='y' WHERE a=1; +# INSERT takes no alias, so it cannot reach an aliased lock at all +INSERT INTO ht SELECT a+10, b FROM ht AS h2; +ERROR HY000: Table 'ht' was not locked with LOCK TABLES +UNLOCK TABLES; +# ===== blob update and delete under the double lock (MEMORY) ===== +LOCK TABLE ht AS h1 WRITE, ht AS h2 READ; +UPDATE ht AS h1, ht AS h2 SET h1.b=REPEAT('z', 900) +WHERE h1.a=h2.a AND h1.a=1; +DELETE FROM ht AS h1 WHERE a=2; +UNLOCK TABLES; +CHECK TABLE ht; +Table Op Msg_type Msg_text +test.ht check status OK +SELECT a, LENGTH(b) FROM ht; +a LENGTH(b) +1 900 +DROP TABLE ht, mi; diff --git a/mysql-test/suite/heap/blob_lock_twice.test b/mysql-test/suite/heap/blob_lock_twice.test new file mode 100644 index 0000000000000..0dd62abe47419 --- /dev/null +++ b/mysql-test/suite/heap/blob_lock_twice.test @@ -0,0 +1,69 @@ +# +# A MEMORY table with a blob locked twice in the same lock set. +# +# Two aliased entries give one share two handles, and thr_lock() grants each +# of them separately, so the grant a handle records is its own. Every row +# operation below runs with both grants live, and the blob update parks a +# chain that has to be redeemed when the handles are unlocked. MyISAM runs +# the same shapes as a control. +# +# An aliased entry can only be referenced by its alias while LOCK TABLES is +# in effect; the unaliased name is not in the lock set at all. That is +# asserted rather than avoided, because it is what keeps the two handles +# distinct. +# +CREATE TABLE ht (a INT, b BLOB) ENGINE=MEMORY; +CREATE TABLE mi (a INT, b BLOB) ENGINE=MyISAM; +INSERT INTO ht VALUES (1,'foo'),(2,'bar'); +INSERT INTO mi VALUES (1,'foo'),(2,'bar'); + +--echo # ===== single aliased entry, referenced as "table AS alias" ===== +--echo # -- MyISAM +LOCK TABLE mi AS m1 WRITE; +SELECT COUNT(*) FROM mi AS m1; +UPDATE mi AS m1 SET b='x' WHERE a=1; +--echo # the unaliased name is not locked +--error ER_TABLE_NOT_LOCKED +SELECT COUNT(*) FROM mi; +UNLOCK TABLES; + +--echo # -- MEMORY +LOCK TABLE ht AS h1 WRITE; +SELECT COUNT(*) FROM ht AS h1; +UPDATE ht AS h1 SET b='x' WHERE a=1; +--echo # the unaliased name is not locked +--error ER_TABLE_NOT_LOCKED +SELECT COUNT(*) FROM ht; +UNLOCK TABLES; + +--echo # ===== two aliased entries, one WRITE and one READ ===== +--echo # -- MyISAM +LOCK TABLE mi AS m1 WRITE, mi AS m2 READ; +SELECT COUNT(*) FROM mi AS m2; +UPDATE mi AS m1 SET b='y' WHERE a=1; +--echo # INSERT takes no alias, so it cannot reach an aliased lock at all +--error ER_TABLE_NOT_LOCKED +INSERT INTO mi SELECT a+10, b FROM mi AS m2; +UNLOCK TABLES; + +--echo # -- MEMORY +LOCK TABLE ht AS h1 WRITE, ht AS h2 READ; +SELECT COUNT(*) FROM ht AS h2; +UPDATE ht AS h1 SET b='y' WHERE a=1; +--echo # INSERT takes no alias, so it cannot reach an aliased lock at all +--error ER_TABLE_NOT_LOCKED +INSERT INTO ht SELECT a+10, b FROM ht AS h2; +UNLOCK TABLES; + +--echo # ===== blob update and delete under the double lock (MEMORY) ===== +LOCK TABLE ht AS h1 WRITE, ht AS h2 READ; +UPDATE ht AS h1, ht AS h2 SET h1.b=REPEAT('z', 900) + WHERE h1.a=h2.a AND h1.a=1; +DELETE FROM ht AS h1 WHERE a=2; +UNLOCK TABLES; + +CHECK TABLE ht; +--sorted_result +SELECT a, LENGTH(b) FROM ht; + +DROP TABLE ht, mi; diff --git a/mysql-test/suite/heap/blob_online_alter.result b/mysql-test/suite/heap/blob_online_alter.result new file mode 100644 index 0000000000000..1583e3608b121 --- /dev/null +++ b/mysql-test/suite/heap/blob_online_alter.result @@ -0,0 +1,26 @@ +CREATE TABLE t1 (a INT PRIMARY KEY, b BLOB) ENGINE=MEMORY; +INSERT INTO t1 VALUES (1, REPEAT('x', 300)), (2, REPEAT('y', 300)), +(3, REPEAT('w', 300)), (4, REPEAT('v', 300)); +connect alterer,localhost,root,,test; +SET DEBUG_SYNC= 'alter_table_online_downgraded SIGNAL downgraded EXECUTE 1'; +SET DEBUG_SYNC= 'alter_table_online_progress WAIT_FOR dml_done EXECUTE 1'; +ALTER TABLE t1 ADD COLUMN c INT, ALGORITHM=COPY, LOCK=NONE; +connection default; +SET SESSION lock_wait_timeout= 20; +SET DEBUG_SYNC= 'now WAIT_FOR downgraded'; +DELETE FROM t1 WHERE a = 1; +UPDATE t1 SET b = REPEAT('z', 900) WHERE a = 2; +DELETE FROM t1 WHERE a = 3; +SET DEBUG_SYNC= 'now SIGNAL dml_done'; +connection alterer; +connection default; +disconnect alterer; +SET DEBUG_SYNC= 'RESET'; +SELECT a, LENGTH(b), c FROM t1; +a LENGTH(b) c +2 900 NULL +4 300 NULL +CHECK TABLE t1; +Table Op Msg_type Msg_text +test.t1 check status OK +DROP TABLE t1; diff --git a/mysql-test/suite/heap/blob_online_alter.test b/mysql-test/suite/heap/blob_online_alter.test new file mode 100644 index 0000000000000..c6891493212c4 --- /dev/null +++ b/mysql-test/suite/heap/blob_online_alter.test @@ -0,0 +1,47 @@ +# +# Blob chains parked on an online-ALTER copy target. +# +# The copy target is locked with a direct handler::ha_external_lock() rather +# than through the SQL layer's lock set, so thr_lock() never grants it +# anything and the handle holds a write lock that no grant records. Rows +# deleted or updated on the source while the copy runs are replayed onto that +# target, and a blob delete/update there parks a chain that has to be redeemed +# when the copy is unlocked. +# +# The ALTER is not paused while it holds the source lock -- doing that blocks +# the very statements this test needs to run. It is released at the +# post-downgrade point and only made to wait once it is replaying. +# +--source include/have_debug_sync.inc + +CREATE TABLE t1 (a INT PRIMARY KEY, b BLOB) ENGINE=MEMORY; +INSERT INTO t1 VALUES (1, REPEAT('x', 300)), (2, REPEAT('y', 300)), + (3, REPEAT('w', 300)), (4, REPEAT('v', 300)); + +connect (alterer,localhost,root,,test); +SET DEBUG_SYNC= 'alter_table_online_downgraded SIGNAL downgraded EXECUTE 1'; +SET DEBUG_SYNC= 'alter_table_online_progress WAIT_FOR dml_done EXECUTE 1'; +--send ALTER TABLE t1 ADD COLUMN c INT, ALGORITHM=COPY, LOCK=NONE + +connection default; +SET SESSION lock_wait_timeout= 20; +SET DEBUG_SYNC= 'now WAIT_FOR downgraded'; + +# Both shapes that park a chain: a delete, and an update that grows the blob +DELETE FROM t1 WHERE a = 1; +UPDATE t1 SET b = REPEAT('z', 900) WHERE a = 2; +DELETE FROM t1 WHERE a = 3; + +SET DEBUG_SYNC= 'now SIGNAL dml_done'; + +connection alterer; +--reap + +connection default; +disconnect alterer; +SET DEBUG_SYNC= 'RESET'; + +--sorted_result +SELECT a, LENGTH(b), c FROM t1; +CHECK TABLE t1; +DROP TABLE t1; diff --git a/mysql-test/suite/heap/blob_tmp_table.result b/mysql-test/suite/heap/blob_tmp_table.result new file mode 100644 index 0000000000000..77058a9ddeeea --- /dev/null +++ b/mysql-test/suite/heap/blob_tmp_table.result @@ -0,0 +1,95 @@ +# +# Blob updates and deletes on a user TEMPORARY MEMORY table. +# +# A non-transactional TEMPORARY table is left out of the lock set +# altogether, so its handle never holds a THR_LOCK. It is not an +# internal table, though, so it still defers its blob chain frees -- +# a combination no other kind of table has, and one a debug build used +# to assert on at the end of every such statement. +# +CREATE TEMPORARY TABLE t1 (a INT, b BLOB) ENGINE=MEMORY; +INSERT INTO t1 VALUES (1,'one'), (2,REPEAT('two',500)), (3,REPEAT('three',500)); +# UPDATE parks one chain per changed blob +UPDATE t1 SET b=REPEAT('x',4000) WHERE a=2; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +a LENGTH(b) LEFT(b,5) +1 3 one +2 4000 xxxxx +3 2500 three +CHECK TABLE t1; +Table Op Msg_type Msg_text +test.t1 check status OK +# DELETE parks the whole row's chain +DELETE FROM t1 WHERE a=3; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +a LENGTH(b) LEFT(b,5) +1 3 one +2 4000 xxxxx +CHECK TABLE t1; +Table Op Msg_type Msg_text +test.t1 check status OK +# Repeated cycles: every parked chain has to come back to the free list +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +a LENGTH(b) LEFT(b,5) +1 1500 oneon +2 8000 zzzzz +CHECK TABLE t1; +Table Op Msg_type Msg_text +test.t1 check status OK +# Multi-table UPDATE and DELETE across two TEMPORARY tables +CREATE TEMPORARY TABLE t2 (a INT, b BLOB) ENGINE=MEMORY; +INSERT INTO t2 VALUES (1,REPEAT('u',4000)), (2,REPEAT('v',4000)); +UPDATE t1, t2 SET t1.b=REPEAT('p',4000), t2.b=REPEAT('q',4000) +WHERE t1.a=t2.a; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +a LENGTH(b) LEFT(b,5) +1 4000 ppppp +2 4000 ppppp +SELECT a, LENGTH(b), LEFT(b,5) FROM t2 ORDER BY a; +a LENGTH(b) LEFT(b,5) +1 4000 qqqqq +2 4000 qqqqq +CHECK TABLE t1, t2; +Table Op Msg_type Msg_text +test.t1 check status OK +test.t2 check status OK +DELETE t1, t2 FROM t1, t2 WHERE t1.a=t2.a AND t1.a=1; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +a LENGTH(b) LEFT(b,5) +2 4000 ppppp +SELECT a, LENGTH(b), LEFT(b,5) FROM t2 ORDER BY a; +a LENGTH(b) LEFT(b,5) +2 4000 qqqqq +CHECK TABLE t1, t2; +Table Op Msg_type Msg_text +test.t1 check status OK +test.t2 check status OK +DROP TEMPORARY TABLE t1, t2; +# +# The same statements on a non-temporary MEMORY table, which does hold +# the lock while it parks and redeems at unlock time. +# +CREATE TABLE t1 (a INT, b BLOB) ENGINE=MEMORY; +INSERT INTO t1 VALUES (1,'one'), (2,REPEAT('two',500)), (3,REPEAT('three',500)); +UPDATE t1 SET b=REPEAT('x',4000) WHERE a=2; +DELETE FROM t1 WHERE a=3; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +a LENGTH(b) LEFT(b,5) +1 3 one +2 4000 xxxxx +CHECK TABLE t1; +Table Op Msg_type Msg_text +test.t1 check status OK +# Under LOCK TABLES the redemption happens in ha_heap::reset() instead, +# with the lock still held +LOCK TABLES t1 WRITE; +UPDATE t1 SET b=REPEAT('w',4000) WHERE a=2; +DELETE FROM t1 WHERE a=1; +UNLOCK TABLES; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +a LENGTH(b) LEFT(b,5) +2 4000 wwwww +CHECK TABLE t1; +Table Op Msg_type Msg_text +test.t1 check status OK +DROP TABLE t1; diff --git a/mysql-test/suite/heap/blob_tmp_table.test b/mysql-test/suite/heap/blob_tmp_table.test new file mode 100644 index 0000000000000..009b02e45183e --- /dev/null +++ b/mysql-test/suite/heap/blob_tmp_table.test @@ -0,0 +1,78 @@ +--source include/not_embedded.inc + +--echo # +--echo # Blob updates and deletes on a user TEMPORARY MEMORY table. +--echo # +--echo # A non-transactional TEMPORARY table is left out of the lock set +--echo # altogether, so its handle never holds a THR_LOCK. It is not an +--echo # internal table, though, so it still defers its blob chain frees -- +--echo # a combination no other kind of table has, and one a debug build used +--echo # to assert on at the end of every such statement. +--echo # + +CREATE TEMPORARY TABLE t1 (a INT, b BLOB) ENGINE=MEMORY; +INSERT INTO t1 VALUES (1,'one'), (2,REPEAT('two',500)), (3,REPEAT('three',500)); + +--echo # UPDATE parks one chain per changed blob +UPDATE t1 SET b=REPEAT('x',4000) WHERE a=2; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +CHECK TABLE t1; + +--echo # DELETE parks the whole row's chain +DELETE FROM t1 WHERE a=3; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +CHECK TABLE t1; + +--echo # Repeated cycles: every parked chain has to come back to the free list +--disable_query_log +let $i= 20; +while ($i) +{ + UPDATE t1 SET b=REPEAT('y',4000) WHERE a=2; + UPDATE t1 SET b=REPEAT('z',8000) WHERE a=2; + DELETE FROM t1 WHERE a=1; + INSERT INTO t1 VALUES (1,REPEAT('one',500)); + dec $i; +} +--enable_query_log +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +CHECK TABLE t1; + +--echo # Multi-table UPDATE and DELETE across two TEMPORARY tables +CREATE TEMPORARY TABLE t2 (a INT, b BLOB) ENGINE=MEMORY; +INSERT INTO t2 VALUES (1,REPEAT('u',4000)), (2,REPEAT('v',4000)); +UPDATE t1, t2 SET t1.b=REPEAT('p',4000), t2.b=REPEAT('q',4000) + WHERE t1.a=t2.a; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +SELECT a, LENGTH(b), LEFT(b,5) FROM t2 ORDER BY a; +CHECK TABLE t1, t2; + +DELETE t1, t2 FROM t1, t2 WHERE t1.a=t2.a AND t1.a=1; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +SELECT a, LENGTH(b), LEFT(b,5) FROM t2 ORDER BY a; +CHECK TABLE t1, t2; + +DROP TEMPORARY TABLE t1, t2; + +--echo # +--echo # The same statements on a non-temporary MEMORY table, which does hold +--echo # the lock while it parks and redeems at unlock time. +--echo # + +CREATE TABLE t1 (a INT, b BLOB) ENGINE=MEMORY; +INSERT INTO t1 VALUES (1,'one'), (2,REPEAT('two',500)), (3,REPEAT('three',500)); +UPDATE t1 SET b=REPEAT('x',4000) WHERE a=2; +DELETE FROM t1 WHERE a=3; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +CHECK TABLE t1; + +--echo # Under LOCK TABLES the redemption happens in ha_heap::reset() instead, +--echo # with the lock still held +LOCK TABLES t1 WRITE; +UPDATE t1 SET b=REPEAT('w',4000) WHERE a=2; +DELETE FROM t1 WHERE a=1; +UNLOCK TABLES; +SELECT a, LENGTH(b), LEFT(b,5) FROM t1 ORDER BY a; +CHECK TABLE t1; + +DROP TABLE t1; diff --git a/storage/heap/CMakeLists.txt b/storage/heap/CMakeLists.txt index d0dc06a6c5823..4e75796778d8e 100644 --- a/storage/heap/CMakeLists.txt +++ b/storage/heap/CMakeLists.txt @@ -34,7 +34,7 @@ IF(WITH_UNIT_TESTS) TARGET_LINK_LIBRARIES(hp_test2 heap mysys dbug strings) MY_ADD_TESTS(hp_test_hash hp_test_scan hp_test_freelist hp_test_concurrent hp_test_block_size hp_test_blob_alias hp_test_update - hp_test_write_dup + hp_test_write_dup hp_test_unlock_check LINK_LIBRARIES heap mysys dbug strings) INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/sql diff --git a/storage/heap/ha_heap.cc b/storage/heap/ha_heap.cc index f6694ba1ae3b8..6433d69883e72 100644 --- a/storage/heap/ha_heap.cc +++ b/storage/heap/ha_heap.cc @@ -499,6 +499,31 @@ int ha_heap::extra(enum ha_extra_function operation) int ha_heap::reset() { + /* + heap_reset() redeems the blob chains parked by a deferred free, which puts + records back on the shared free list. That is a write to HP_SHARE, so it + needs the THR_LOCK -- unless no other connection can reach the share at + all, which is the case for a TEMPORARY table. + + Only heap_delete() and heap_update() park, and only for a table that is + not internal, an internal one being never binlogged and so free to drop + its chains outright. `internal' is HA_OPEN_INTERNAL_TABLE, the + optimizer's own temporary table, which is not the set the server locks: + per sql/lock.cc, a non-transactional TEMPORARY table is never passed to + thr_multi_lock() and never reaches external_lock(F_UNLCK), so a user + TEMPORARY table parks its chains while holding nothing. It owns them + alone -- no other connection can reach the share -- and this call, reached + from mark_tmp_table_as_free_for_reuse() at the end of every statement, is + what redeems them. + + Every other table does hold the lock while it parks, and keeps holding it + until the chains are back: external_lock(F_UNLCK) redeems them for an + ordinary statement, and this call does for a statement under LOCK TABLES, + reached from mark_used_tables_as_free_for_reuse() with the lock still + held. By the time close_thread_table() gets here there is nothing left. + */ + DBUG_ASSERT(!file->has_pending_blob_free || hp_lock_is_held(file) || + table->s->tmp_table != NO_TMP_TABLE); return heap_reset(file); } @@ -528,22 +553,54 @@ int ha_heap::reset_auto_increment(ulonglong value) int ha_heap::external_lock(THD *thd, int lock_type) { #if !defined(DBUG_OFF) && defined(EXTRA_HEAP_DEBUG) - /* - A table already marked crashed is knowingly inconsistent; every data - access on it fails with HA_ERR_CRASHED, so re-detecting the damage - here would only raise a second error into a diagnostics area that - can already be OK (e.g. after UNLOCK TABLES) and fire the - Diagnostics_area assertion. - */ - if (lock_type == F_UNLCK && file->s->changed && - !heap_is_crashed(file->s) && heap_check_heap(file, 0)) + /* See hp_may_check_heap_on_unlock() for when this is safe to run at all */ + if (lock_type == F_UNLCK && hp_may_check_heap_on_unlock(file) && + heap_check_heap(file, 0)) return HA_ERR_CRASHED; #endif if (lock_type != F_UNLCK && heap_is_crashed(file->s)) return HA_ERR_CRASHED; if (lock_type == F_UNLCK) + { + /* + Redeeming the parked chains puts records back on the shared free list, + which needs the THR_LOCK -- see hp_test_concurrent-t.c, which reproduces + the del_link corruption that flushing without it causes -- unless no + other connection can reach the share at all, the same escape + ha_heap::reset() takes for a temporary table. + + A chain is parked only by heap_delete()/heap_update(), so only by a + handle that could write the share, and the description in sql/lock.cc + has the unlock order -- external_lock(F_UNLCK) first, thr_multi_unlock() + after -- so a handle that took the lock through the SQL layer still + holds it here. The two paths that reach F_UNLCK holding nothing -- + lock_external() unwinding what it had locked, and mysql_lock_tables() + balancing it after thr_multi_lock() failed -- never ran a row operation, + so they have nothing parked and this is a no-op for them. + + The temporary table is the ALTER TABLE copy target, which + copy_data_between_tables() locks with a direct ha_external_lock() + instead of through the lock set. thr_lock() never grants that handle + anything, so hp_lock_is_held() is false on it even while it writes; an + online ALTER replays concurrent deletes onto it, which park. Its share + is private to the ALTER, so redeeming without the lock is safe. + */ + DBUG_ASSERT(!file->has_pending_blob_free || hp_lock_is_held(file) || + table->s->tmp_table != NO_TMP_TABLE); hp_flush_pending_blob_free(file); + hp_lock_released(file); + } + else + { + /* + Every grant is released above before the next request is made, so a + request can never find one left over from the request before it. Were + that to happen, hp_lock_is_held() would report a lock this handle does + not hold, and the verification above would scan HP_SHARE unlocked. + */ + DBUG_ASSERT(!hp_lock_is_held(file)); + } return 0; // No external locking } diff --git a/storage/heap/heapdef.h b/storage/heap/heapdef.h index f157bcc77e9b9..e58e0cccbf2ef 100644 --- a/storage/heap/heapdef.h +++ b/storage/heap/heapdef.h @@ -391,6 +391,80 @@ static inline void hp_flush_pending_blob_free(HP_INFO *info) hp_flush_pending_blob_free_impl(info); } +/* + A grant recorded by hp_lock_granted() is over. + + Called from ha_heap::external_lock(F_UNLCK), which per the description in + sql/lock.cc the SQL layer reaches while the THR_LOCK is still held, just + before thr_multi_unlock(). + + F_UNLCK is also reached on paths where this handle was never granted + anything (see hp_lock_is_held()), where clearing an already clear record is + simply a no-op. +*/ + +static inline void hp_lock_released(HP_INFO *info) +{ + info->lock_granted= 0; +} + +/* + Does this handle hold its table's THR_LOCK? + + ha_heap::external_lock() is reached with F_UNLCK on three paths that look + identical to the handler. Only the first of them still holds the lock: + + - mysql_unlock_tables() calls unlock_external() before thr_multi_unlock(); + - mysql_lock_tables() calls unlock_external() to balance the external + locks it already took, because thr_multi_lock() failed; + - 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. + + The description in sql/lock.cc covers the first and the third; the second is + only in its code, where a non-zero thr_multi_lock() return sends + mysql_lock_tables() through unlock_external(). + + The requested lock type cannot tell them apart on its own, because + ha_heap::store_lock() records it at get_lock_data() time, before anything is + locked: on the last path it is set while nothing is held. Only an actual + grant can, which is what hp_lock_granted() records. The type is still + needed as well, for the lock thr_multi_lock() takes and then rolls back when + a later table times out: thr_unlock() resets the type but cannot reach the + record, which stands until ha_heap::external_lock(F_UNLCK) clears it. + + A HEAP table has no row-level concurrency control: everything shared through + HP_SHARE is protected by the THR_LOCK alone. Anything that reads or writes + the share therefore has to ask this first. +*/ + +static inline my_bool hp_lock_is_held(const HP_INFO *info) +{ + return info->lock_granted && info->lock.type != TL_UNLOCK; +} + +/* + May ha_heap::external_lock(F_UNLCK) verify the table with heap_check_heap()? + + A scan taken outside the THR_LOCK sees a writer's intermediate state: + hp_alloc_from_tail() publishes total_records at allocation time, before the + slot is written. The scan then either counts a slot the writer has not + filled in yet or races the counters it compares against, and reports damage + that is not there. Since heap_check_heap() marks the share crashed, that + false positive poisons a healthy table for every connection using it. + + A table already marked crashed is knowingly inconsistent; every data access + on it fails with HA_ERR_CRASHED, so re-detecting the damage here would only + raise a second error into a diagnostics area that can already be OK (e.g. + after UNLOCK TABLES) and fire the Diagnostics_area assertion. +*/ + +static inline my_bool hp_may_check_heap_on_unlock(const HP_INFO *info) +{ + return (hp_lock_is_held(info) && info->s->changed && + !heap_is_crashed(info->s)); +} + /* Does a record's blob data live in `chain`? diff --git a/storage/heap/hp_create.c b/storage/heap/hp_create.c index 8bdc08bb36908..bb3871089f6b8 100644 --- a/storage/heap/hp_create.c +++ b/storage/heap/hp_create.c @@ -22,6 +22,31 @@ static void init_block(HP_BLOCK *block, size_t reclength, ulong min_records, ulong max_records); +/* + THR_LOCK grant callback. + + The only thing HEAP wants from it is the fact that it was called: a handle + may not touch HP_SHARE until thr_lock() has actually given it the lock, and + the requested lock type says nothing about that (see hp_lock_is_held()). + Never fails, so a grant is never turned into THR_LOCK_ABORTED. + + Set rather than counted, because thr_lock() does not call this once per + ha_heap::external_lock(): a delayed insert is granted TL_WRITE_DELAYED and + then calls here a second time on the same THR_LOCK_DATA when + thr_upgrade_write_delay_lock() turns it into a real write lock, with no + external_lock() in between. A count would keep the surplus for the life of + the handle; setting is idempotent, and hp_lock_released() clears it from + ha_heap::external_lock(F_UNLCK). +*/ + +static my_bool hp_lock_granted(void *param, + my_bool concurrent_insert __attribute__((unused))) +{ + ((HP_INFO*) param)->lock_granted= 1; + return 0; +} + + /* In how many parts are we going to do allocations of memory and indexes If we assign 1M to the heap table memory, we will allocate roughly @@ -305,6 +330,7 @@ int heap_create(const char *name, HP_CREATE_INFO *create_info, if (!create_info->internal_table) { thr_lock_init(&share->lock); + share->lock.get_status= hp_lock_granted; share->open_list.data= (void*) share; heap_share_list= list_add(heap_share_list,&share->open_list); } diff --git a/storage/heap/hp_open.c b/storage/heap/hp_open.c index c512b88efd369..fe06d463af1ef 100644 --- a/storage/heap/hp_open.c +++ b/storage/heap/hp_open.c @@ -39,7 +39,7 @@ HP_INFO *heap_open_from_share(HP_SHARE *share, int mode) DBUG_RETURN(0); } share->open_count++; - thr_lock_data_init(&share->lock,&info->lock,NULL); + thr_lock_data_init(&share->lock,&info->lock,info); info->s= share; info->lastkey= (uchar*) (info + 1); info->recbuf= (uchar*) (info->lastkey + share->max_key_length); diff --git a/storage/heap/hp_test_unlock_check-t.c b/storage/heap/hp_test_unlock_check-t.c new file mode 100644 index 0000000000000..0b1d683e100df --- /dev/null +++ b/storage/heap/hp_test_unlock_check-t.c @@ -0,0 +1,342 @@ +/* + Unit test: the unlock-time table verification must not run when the + THR_LOCK is not held. + + ha_heap::external_lock(F_UNLCK) verifies the table with heap_check_heap(). + That is safe on the ordinary unlock path, which sql/lock.cc describes as + external_lock(F_UNLCK) followed by thr_multi_unlock(), so the THR_LOCK is + still held. It is not safe on either path that unlocks after a failed lock + attempt: when mysql_lock_tables() balances the external locks it took + because thr_multi_lock() timed out, and when lock_external() itself unwinds + the tables it already locked because a later table refused -- sql/lock.cc + spells out the second of those, the first is only in its code. In both the + caller holds nothing, while another connection is writing. + + The test drives the real lock protocol (thr_multi_lock/thr_multi_unlock, + exactly what sql/lock.cc calls) rather than reproducing a race by repetition. + A holder thread takes TL_WRITE and keeps it until told to let go, so the + contending request is guaranteed to time out; the holder also parks the share + in the state a writer passes through mid-row, so the verification has + something to (wrongly) find. Every outcome here is forced, not raced. +*/ + +#include "hp_test_helpers.h" +#include + +/* Holder-thread handshake. Both flags are set under state_mutex. */ +static pthread_mutex_t state_mutex; +static pthread_cond_t state_cond; +static int holder_ready; /* lock taken, share parked */ +static int holder_release; /* main is done, let go */ + +static HP_INFO *holder_info; + +/* + The slot the holder allocated but has not written yet. Handed back to main + so it can undo the parked state after the holder is gone. +*/ +static uchar *parked_slot; + + +static void set_flag(int *flag) +{ + pthread_mutex_lock(&state_mutex); + *flag= 1; + pthread_cond_broadcast(&state_cond); + pthread_mutex_unlock(&state_mutex); +} + + +static void wait_flag(int *flag) +{ + pthread_mutex_lock(&state_mutex); + while (!*flag) + pthread_cond_wait(&state_cond, &state_mutex); + pthread_mutex_unlock(&state_mutex); +} + + +/* + Put a handle in the state the SQL layer leaves it in just before the + THR_LOCK is attempted: ha_heap::store_lock() has recorded the requested type + (at get_lock_data() time, well before any locking), and lock_external() has + reached ha_heap::external_lock() with that type. Nothing is held yet. +*/ + +static void request_lock(HP_INFO *info, enum thr_lock_type type) +{ + info->lock.type= type; /* ha_heap::store_lock() */ +} + + +/* + Release the handle in the order sql/lock.cc documents: mysql_unlock_tables() + reaches ha_heap::external_lock(F_UNLCK) while the THR_LOCK is still held, and + only then calls thr_multi_unlock(). Call this before thr_multi_unlock(), + never after, or the test stops mirroring the protocol it is here to police. +*/ + +static void release_lock(HP_INFO *info) +{ + hp_lock_released(info); /* ha_heap::external_lock() */ +} + + +/* + Reproduce the state heap_write() is in between allocating a slot and marking + it visible: next_free_record_pos() has already published the slot in + total_records, but the record has not been stored yet. + + The slot is zeroed rather than left as it comes from my_malloc() so that the + test asserts on a defined outcome. A zero flags byte is what a scan of a + half-written row legitimately sees; leaving the malloc garbage in place is + what makes the same access an uninitialised read under MSAN. +*/ + +static uchar *park_mid_write(HP_SHARE *share) +{ + uchar *pos= next_free_record_pos(share); + if (pos) + memset(pos, 0, share->block.recbuffer); + return pos; +} + + +static void unpark_mid_write(HP_SHARE *share, uchar *pos) +{ + hp_push_free_record(share, pos); + hp_shrink_tail(share); +} + + +static void *holder_thread(void *arg __attribute__((unused))) +{ + THR_LOCK_INFO owner; + THR_LOCK_DATA *lock_data[1]; + + my_thread_init(); + thr_lock_info_init(&owner, my_thread_var); + + request_lock(holder_info, TL_WRITE); + lock_data[0]= &holder_info->lock; + + if (thr_multi_lock(lock_data, 1, &owner, 0) != THR_LOCK_SUCCESS) + { + /* Nothing else holds the lock yet, so this cannot fail */ + set_flag(&holder_ready); + my_thread_end(); + return NULL; + } + + parked_slot= park_mid_write(holder_info->s); + + set_flag(&holder_ready); + wait_flag(&holder_release); + + release_lock(holder_info); + thr_multi_unlock(lock_data, 1, 0); + my_thread_end(); + return NULL; +} + + +int main(int argc __attribute__((unused)), + char **argv __attribute__((unused))) +{ + HP_SHARE *share, *share2; + HP_INFO *info1, *info2, *other; + const char *contended; + THR_LOCK_INFO waiter; + THR_LOCK_DATA *waiter_lock_data[2]; + enum enum_thr_lock_result lock_result; + pthread_t holder; + uchar rec[REC_LENGTH]; + uchar blob_data[200]; + int i; + + plan(17); + MY_INIT("hp_test_unlock_check-t"); + pthread_mutex_init(&state_mutex, NULL); + pthread_cond_init(&state_cond, NULL); + + if (create_and_open("test_unlock_check", &share, &info1) || + create_and_open("test_unlock_check2", &share2, &other)) + { + ok(0, "setup failed"); + return exit_status(); + } + + /* + thr_multi_lock() sorts the request array before locking anything, and + LOCK_CMP compares the THR_LOCK address first. Both requests in the + two-table scenario below are TL_WRITE on different shares, so nothing + breaks that tie and the order is decided by where my_malloc() put the two + shares -- which arm of the scenario runs would otherwise vary per run. + + Contend the share that sorts LAST. Then the other table is always + reached and granted before the contended one times out, which is the + granted-then-rolled-back arm. It is the only thing anywhere that + exercises the lock.type half of hp_lock_is_held(): a handle whose grant + is recorded while it holds nothing. The arm this avoids leaves both + halves false and so passes whichever half is deleted. + */ + if (&share->lock > &share2->lock) + contended= "test_unlock_check"; + else + { + HP_SHARE *swap_share= share; + HP_INFO *swap_info= info1; + share= share2; + info1= other; + share2= swap_share; + other= swap_info; + contended= "test_unlock_check2"; + } + + info2= heap_open(contended, 2); + if (!info2) + { + ok(0, "second open failed"); + heap_close(info1); + return exit_status(); + } + heap_extra(info2, HA_EXTRA_NO_READCHECK); + + /* Populate, so that share->changed is set and the scan has work to do */ + for (i= 0; i < 5; i++) + { + memset(blob_data, 'a' + i, sizeof(blob_data)); + build_record(rec, 100 + i, blob_data, (uint16) sizeof(blob_data)); + if (heap_write(info1, rec)) + { + ok(0, "populate failed"); + heap_close(info2); + heap_drop_table(other); + heap_drop_table(info1); + return exit_status(); + } + } + + ok(heap_check_heap(info1, 0) == 0, "table is consistent before the test"); + ok(share->changed != 0, "share is marked changed by the writes"); + + holder_info= info1; + pthread_create(&holder, NULL, holder_thread, NULL); + wait_flag(&holder_ready); + + ok(parked_slot != NULL, "holder parked the share mid-write under TL_WRITE"); + + /* + What sql/lock.cc does: thr_multi_lock() with a timeout, which cannot + succeed because the holder keeps TL_WRITE until we say so. + */ + thr_lock_info_init(&waiter, my_thread_var); + request_lock(info2, TL_WRITE); + waiter_lock_data[0]= &info2->lock; + lock_result= thr_multi_lock(waiter_lock_data, 1, &waiter, 1); + + ok(lock_result == THR_LOCK_WAIT_TIMEOUT, "contending lock request times out"); + + /* + mysql_lock_tables() now calls unlock_external() to balance the external + locks it took, reaching ha_heap::external_lock(F_UNLCK) with no THR_LOCK. + thr_multi_lock() left the request marked TL_UNLOCK, which is how the + handler tells this path from an ordinary unlock. + */ + ok(info2->lock.type == TL_UNLOCK, + "failed lock request is left marked TL_UNLOCK"); + ok(!hp_lock_is_held(info2), "the failed requester knows it holds no lock"); + ok(hp_lock_is_held(info1), "the holder knows it does hold the lock"); + ok(!hp_may_check_heap_on_unlock(info2), + "unlock-time verification is skipped when the lock is not held"); + + /* + Show what running it there would have cost. The holder is still parked + mid-write, so the verification finds the half-written slot and marks the + share crashed -- which is what makes a later INSERT fail with + ER_NOT_KEYFILE even though nothing is wrong with the table. + */ + ok(heap_check_heap(info2, 0) != 0 && heap_is_crashed(share), + "running it anyway reports damage and marks the share crashed"); + + heap_clear_state(share); + + /* Finish the unlock_external() this path performs */ + release_lock(info2); + + /* + A real statement locks more than one table, and a failed request leaves + its tables in two different states. The setup above pinned which is + which: thr_multi_lock() reaches the uncontended table first and grants + it, then times out on the contended one and rolls that grant back. + + The rollback is the shape the predicate exists for. thr_unlock() resets + the request's type but cannot reach the handler's grant record, so the + record stands on a handle that holds nothing, and only the type half of + hp_lock_is_held() answers no. The contended table is the other shape -- + never attempted, and normalized to TL_UNLOCK by the loop in + thr_multi_lock() -- which leaves both halves false. + */ + request_lock(info2, TL_WRITE); + request_lock(other, TL_WRITE); + waiter_lock_data[0]= &info2->lock; + waiter_lock_data[1]= &other->lock; + lock_result= thr_multi_lock(waiter_lock_data, 2, &waiter, 1); + + ok(lock_result == THR_LOCK_WAIT_TIMEOUT, + "multi-table request times out on the contended table"); + ok(other->lock_granted && other->lock.type == TL_UNLOCK, + "the uncontended table was granted and then rolled back"); + ok(!info2->lock_granted, + "the contended table timed out without ever being granted"); + ok(!hp_lock_is_held(info2) && !hp_lock_is_held(other), + "neither table of a failed multi-table request claims the lock"); + + /* + unlock_external() balances every table of the failed request, including + the one thr_multi_lock() granted and then rolled back: thr_unlock() reset + that request's type but left its grant recorded. + */ + release_lock(info2); + release_lock(other); + + /* An uncontended grant, to show the predicate does say yes when it should */ + request_lock(other, TL_WRITE); + waiter_lock_data[0]= &other->lock; + lock_result= thr_multi_lock(waiter_lock_data, 1, &waiter, 1); + + ok(lock_result == THR_LOCK_SUCCESS && hp_lock_is_held(other), + "an uncontended grant is reported as held"); + + release_lock(other); + thr_multi_unlock(waiter_lock_data, 1, 0); + ok(!hp_lock_is_held(other), "the lock is not reported as held once released"); + + /* + The third F_UNLCK path: lock_external() locked this table, a later table + refused, and lock_external() unwinds what it had already locked -- + entirely before thr_multi_lock() runs. The requested type is set and + nothing normalized it, so the type alone cannot tell this apart from an + ordinary unlock. Only the fact that no grant ever arrived can. + */ + request_lock(other, TL_WRITE); + ok(!hp_lock_is_held(other), + "a request unwound before the lock is attempted claims nothing"); + + /* Let the holder finish its row and release the lock */ + set_flag(&holder_release); + pthread_join(holder, NULL); + + unpark_mid_write(share, parked_slot); + + ok(heap_check_heap(info1, 0) == 0, + "the table was consistent all along: the report was a false positive"); + + heap_close(info2); + heap_drop_table(other); + heap_drop_table(info1); + pthread_cond_destroy(&state_cond); + pthread_mutex_destroy(&state_mutex); + my_end(0); + return exit_status(); +}