MDEV-40634 Const MEMORY table's BLOB outlives the lock protecting it

mysql-test/suite/heap/blob_const_unlock.inc

@@ -0,0 +1,58 @@
+#
+# One round of the const-table blob race.
+#
+# Expects t1 (the const table), t2 (the table it is joined against), a
+# procedure churn_blob_memory() that rewrites t1's blob memory, and
+# $reader_id / $writer_id.
+#
+
+--connection default
+# --ps-protocol executes every complete SELECT twice and compares the two
+# result sets.  GET_LOCK() is recursive, so a doubly executed acquisition
+# takes the gate twice while the single RELEASE_LOCK() below drops only one
+# reference.  The gate would then stay shut and the reader would sit there
+# until its own timeout expired.
+--disable_ps2_protocol # because SELECT with side effects
+SELECT GET_LOCK('const_row_gate', 60);
+--enable_ps2_protocol
+
+--connection reader
+# t1 holds a single row, so it is read during optimization and its row is
+# kept in record[0] for the rest of the statement.  GET_LOCK() is evaluated
+# per row of t2 during execution, i.e. after the const row is in hand and
+# after the const tables have been unlocked, and parks the statement there.
+--send SELECT LEFT(t1.b, 16) AS head, LENGTH(t1.b) AS len FROM t1, t2 WHERE t2.a = GET_LOCK('const_row_gate', 60)
+
+--connection default
+let $wait_condition=
+  SELECT COUNT(*) FROM information_schema.PROCESSLIST
+   WHERE ID = $reader_id AND STATE = 'User lock';
+--source include/wait_condition.inc
+
+--connection writer
+--send CALL churn_blob_memory()
+
+--connection default
+# The procedure signals before its first statement, so once the signal
+# arrives the writer is executing.  From there it either blocks on t1's
+# surviving read lock or runs to completion; both mean it has had its turn
+# and the reader can be let go.
+SET DEBUG_SYNC= 'now WAIT_FOR writer_started';
+let $wait_condition=
+  SELECT COUNT(*) FROM information_schema.PROCESSLIST
+   WHERE ID = $writer_id
+     AND (STATE = 'Waiting for table level lock' OR COMMAND = 'Sleep');
+--source include/wait_condition.inc
+
+DO RELEASE_LOCK('const_row_gate');
+
+--connection reader
+--reap
+# The reader took the gate over when it was released.
+DO RELEASE_ALL_LOCKS();
+
+--connection writer
+--reap
+
+--connection default
+SET DEBUG_SYNC= 'RESET';

mysql-test/suite/heap/blob_const_unlock.result

@@ -0,0 +1,78 @@
+CREATE TABLE t1 (a INT, b BLOB) ENGINE=MEMORY;
+INSERT INTO t1 VALUES (1, REPEAT('x', 4000));
+CREATE TABLE t2 (a INT) ENGINE=MEMORY;
+INSERT INTO t2 VALUES (1),(2),(3),(4);
+connect  reader,localhost,root,,test;
+connect  writer,localhost,root,,test;
+connection reader;
+connection writer;
+SET SESSION lock_wait_timeout= 60;
+#
+# The freed blob blocks are handed to another row
+#
+connection default;
+CREATE PROCEDURE churn_blob_memory()
+BEGIN
+SET DEBUG_SYNC= 'now SIGNAL writer_started';
+UPDATE t1 SET b= REPEAT('y', 4000) WHERE a = 1;
+INSERT INTO t1 VALUES (2, REPEAT('z', 4000)), (3, REPEAT('w', 4000));
+END|
+connection default;
+SELECT GET_LOCK('const_row_gate', 60);
+GET_LOCK('const_row_gate', 60)
+1
+connection reader;
+SELECT LEFT(t1.b, 16) AS head, LENGTH(t1.b) AS len FROM t1, t2 WHERE t2.a = GET_LOCK('const_row_gate', 60);
+connection default;
+connection writer;
+CALL churn_blob_memory();
+connection default;
+SET DEBUG_SYNC= 'now WAIT_FOR writer_started';
+DO RELEASE_LOCK('const_row_gate');
+connection reader;
+head	len
+xxxxxxxxxxxxxxxx	4000
+DO RELEASE_ALL_LOCKS();
+connection writer;
+connection default;
+SET DEBUG_SYNC= 'RESET';
+connection default;
+DROP PROCEDURE churn_blob_memory;
+TRUNCATE TABLE t1;
+INSERT INTO t1 VALUES (1, REPEAT('x', 4000));
+#
+# The blob blocks are released outright
+#
+CREATE PROCEDURE churn_blob_memory()
+BEGIN
+SET DEBUG_SYNC= 'now SIGNAL writer_started';
+DELETE FROM t1;
+INSERT INTO t1 VALUES (2, REPEAT('z', 9000));
+END|
+connection default;
+SELECT GET_LOCK('const_row_gate', 60);
+GET_LOCK('const_row_gate', 60)
+1
+connection reader;
+SELECT LEFT(t1.b, 16) AS head, LENGTH(t1.b) AS len FROM t1, t2 WHERE t2.a = GET_LOCK('const_row_gate', 60);
+connection default;
+connection writer;
+CALL churn_blob_memory();
+connection default;
+SET DEBUG_SYNC= 'now WAIT_FOR writer_started';
+DO RELEASE_LOCK('const_row_gate');
+connection reader;
+head	len
+xxxxxxxxxxxxxxxx	4000
+DO RELEASE_ALL_LOCKS();
+connection writer;
+connection default;
+SET DEBUG_SYNC= 'RESET';
+connection default;
+CHECK TABLE t1;
+Table	Op	Msg_type	Msg_text
+test.t1	check	status	OK
+disconnect reader;
+disconnect writer;
+DROP PROCEDURE churn_blob_memory;
+DROP TABLE t1, t2;

mysql-test/suite/heap/blob_const_unlock.test

@@ -0,0 +1,86 @@
+#
+# A const table's blob must not outlive the lock that protected it.
+#
+# A single-row table is read during optimization and its row is kept in
+# record[0] for the rest of the statement.  The optimizer then releases the
+# lock on every const table, on the premise -- stated in its own comment --
+# that all it did was read, so the row is already in hand.
+#
+# For a MEMORY table with a blob the row is not in hand.  hp_read_blobs()
+# aims record[0]'s blob pointer straight into the shared record memory
+# instead of copying the value, so once the lock is gone another connection
+# is free to overwrite, free or recycle the bytes the const row still points
+# at, and the statement goes on reading them.
+#
+# The two rounds below cover the two ways that memory changes hands: reuse
+# (UPDATE frees the chain, INSERT hands the same blocks to another row) and
+# outright release (DELETE with no WHERE goes through delete_all_rows()).
+#
+# The reader is parked with GET_LOCK() rather than with a stored function.
+# A stored function would put the statement into prelocked mode, and the
+# const-table unlock is skipped altogether in that mode, so the code path
+# under test would never run.
+#
+--source include/have_debug_sync.inc
+
+CREATE TABLE t1 (a INT, b BLOB) ENGINE=MEMORY;
+INSERT INTO t1 VALUES (1, REPEAT('x', 4000));
+
+# Several rows, so this one cannot become a const table itself and the join
+# still has an execution phase for the reader to park in.
+CREATE TABLE t2 (a INT) ENGINE=MEMORY;
+INSERT INTO t2 VALUES (1),(2),(3),(4);
+
+connect (reader,localhost,root,,test);
+connect (writer,localhost,root,,test);
+
+--connection reader
+let $reader_id= `SELECT CONNECTION_ID()`;
+
+--connection writer
+let $writer_id= `SELECT CONNECTION_ID()`;
+# A bad interleaving should fail fast rather than hang for a day.
+SET SESSION lock_wait_timeout= 60;
+
+--echo #
+--echo # The freed blob blocks are handed to another row
+--echo #
+--connection default
+DELIMITER |;
+CREATE PROCEDURE churn_blob_memory()
+BEGIN
+  SET DEBUG_SYNC= 'now SIGNAL writer_started';
+  UPDATE t1 SET b= REPEAT('y', 4000) WHERE a = 1;
+  INSERT INTO t1 VALUES (2, REPEAT('z', 4000)), (3, REPEAT('w', 4000));
+END|
+DELIMITER ;|
+
+--source blob_const_unlock.inc
+
+--connection default
+DROP PROCEDURE churn_blob_memory;
+TRUNCATE TABLE t1;
+INSERT INTO t1 VALUES (1, REPEAT('x', 4000));
+
+--echo #
+--echo # The blob blocks are released outright
+--echo #
+DELIMITER |;
+CREATE PROCEDURE churn_blob_memory()
+BEGIN
+  SET DEBUG_SYNC= 'now SIGNAL writer_started';
+  DELETE FROM t1;
+  INSERT INTO t1 VALUES (2, REPEAT('z', 9000));
+END|
+DELIMITER ;|
+
+--source blob_const_unlock.inc
+
+--connection default
+CHECK TABLE t1;
+
+disconnect reader;
+disconnect writer;
+
+DROP PROCEDURE churn_blob_memory;
+DROP TABLE t1, t2;

sql/lock.cc

@@ -800,6 +800,26 @@ static int unlock_external(THD *thd, TABLE **table,uint count)
 }
 
 
+/*
+  Does this table answer a read with pointers into memory it shares with
+  other connections, rather than with a copy of the row?
+
+  MEMORY does, for a blob: hp_read_blobs() points record[0] at the blob data
+  inside HP_SHARE instead of copying it, so the row is only valid while the
+  lock is held -- once it is gone another connection may free or reuse that
+  memory.
+
+  A caller unlocking for good does not care.  One that unlocks early while
+  still holding a row does, and passes GET_LOCK_SKIP_ZERO_COPY_ROWS to leave
+  such a table locked for as long as it means to keep reading the row.
+*/
+
+static inline bool table_has_zero_copy_rows(const TABLE *t)
+{
+  return t->s->db_type() == heap_hton && t->s->blob_fields != 0;
+}
+
+
 /**
   Get lock structures from table structs and initialize locks.
 
@@ -809,6 +829,8 @@ static int unlock_external(THD *thd, TABLE **table,uint count)
            - GET_LOCK_UNLOCK      : If we should send TL_IGNORE to store lock
            - GET_LOCK_STORE_LOCKS : Store lock info in TABLE
            - GET_LOCK_SKIP_SEQUENCES : Ignore sequences (for temporary unlock)
+           - GET_LOCK_SKIP_ZERO_COPY_ROWS : Ignore tables whose already-read
+             row would not survive the unlock (for temporary unlock)
            - GET_LOCK_ON_THD      : Store lock in thd->mem_root
 
   Temporary tables are not locked (as these are single user), except for
@@ -831,7 +853,9 @@ MYSQL_LOCK *get_lock_data(THD *thd, TABLE **table_ptr, uint count, uint flags)
 
     if ((likely(!t->s->tmp_table) ||
          (t->s->tmp_table == TRANSACTIONAL_TMP_TABLE)) &&
-        (!(flags & GET_LOCK_SKIP_SEQUENCES) || t->s->sequence == 0))
+        (!(flags & GET_LOCK_SKIP_SEQUENCES) || t->s->sequence == 0) &&
+        (!(flags & GET_LOCK_SKIP_ZERO_COPY_ROWS) ||
+         !table_has_zero_copy_rows(t)))
     {
       lock_count+= t->file->lock_count();
       table_count++;
@@ -864,7 +888,9 @@ MYSQL_LOCK *get_lock_data(THD *thd, TABLE **table_ptr, uint count, uint flags)
     THR_LOCK_DATA **locks_start;
 
     if ((table->s->tmp_table && table->s->tmp_table != TRANSACTIONAL_TMP_TABLE)
-       || (flags & GET_LOCK_SKIP_SEQUENCES && table->s->sequence != NULL))
+       || (flags & GET_LOCK_SKIP_SEQUENCES && table->s->sequence != NULL)
+       || (flags & GET_LOCK_SKIP_ZERO_COPY_ROWS &&
+           table_has_zero_copy_rows(table)))
       continue;
     lock_type= table->reginfo.lock_type;
     DBUG_ASSERT(lock_type != TL_WRITE_DEFAULT && lock_type != TL_READ_DEFAULT);

sql/lock.h

@@ -47,6 +47,7 @@ bool lock_object_name(THD *thd, MDL_key::enum_mdl_namespace mdl_type,
 #define GET_LOCK_ACTION_MASK    1
 #define GET_LOCK_ON_THD         (1 << 1)
 #define GET_LOCK_SKIP_SEQUENCES (1 << 2)
+#define GET_LOCK_SKIP_ZERO_COPY_ROWS (1 << 3)
 
 MYSQL_LOCK *get_lock_data(THD *thd, TABLE **table_ptr, uint count, uint flags);
 void reset_lock_data(MYSQL_LOCK *sql_lock, bool unlock);

sql/sql_select.cc

@@ -2881,9 +2881,15 @@ int JOIN::optimize_stage2()
       Unlock all tables, except sequences, as accessing these may still
       require table updates. It's safe to ignore result code as all
       tables where opened for read only.
+
+      A const table's row stays in record[0] and is read from there for the
+      rest of the statement, so a table that answered the read with pointers
+      into memory it shares with other connections has to stay locked too --
+      see table_has_zero_copy_rows().
     */
     (void) mysql_unlock_some_tables(thd, table, const_tables,
-                                    GET_LOCK_SKIP_SEQUENCES);
+                                    GET_LOCK_SKIP_SEQUENCES |
+                                    GET_LOCK_SKIP_ZERO_COPY_ROWS);
   }
   if (!conds && outer_join)
   {