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/handler.h

@@ -397,6 +397,18 @@ enum chf_create_flags {
 
 #define HA_LAST_TABLE_FLAG HA_NO_ONLINE_ALTER
 
+/*
+  All table flags are used, the following are a new set of flags
+  stored in table_flags_part2
+*/
+
+/*
+  Set if one cannot access rowdata, like blobs, after unlock, either because
+  the engine frees row data or the row data may be overwritten by other
+  connections. This happens with the zero-copy blobs in the heap engine.
+*/
+#define HA2_CANNOT_ACCESS_ROWDATA_AFTER_UNLOCK (1ULL << 0)
+
 /* bits in index_flags(index_number) for what you can do with index */
 #define HA_READ_NEXT            1       /* TODO really use this flag */
 #define HA_READ_PREV            2       /* supports ::index_prev */
@@ -3762,6 +3774,10 @@ class handler :public Sql_alloc
     DBUG_ASSERT((cached_table_flags >> 1) < HA_LAST_TABLE_FLAG);
     return cached_table_flags;
   }
+  Table_flags ha_table_flags2() const
+  {
+    return table_flags2();
+  }
   /**
     These functions represent the public interface to *users* of the
     handler class, hence they are *not* virtual. For the inheritance
@@ -5491,6 +5507,7 @@ class handler :public Sql_alloc
   */
   virtual int reset() { return 0; }
   virtual Table_flags table_flags(void) const= 0;
+  virtual Table_flags table_flags2() const { return 0; }
   /**
     Is not invoked for non-transactional temporary tables.
 

sql/lock.cc

@@ -809,6 +809,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 +833,10 @@ 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) ||
+         !(t->file->ha_table_flags2() &
+           HA2_CANNOT_ACCESS_ROWDATA_AFTER_UNLOCK)))
     {
       lock_count+= t->file->lock_count();
       table_count++;
@@ -864,7 +869,10 @@ 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->file->ha_table_flags2() &
+            HA2_CANNOT_ACCESS_ROWDATA_AFTER_UNLOCK)))
       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 HA2_CANNOT_ACCESS_ROWDATA_AFTER_UNLOCK.
     */
     (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)
   {

storage/heap/ha_heap.cc

@@ -91,8 +91,8 @@ static handler *heap_create_handler(handlerton *hton,
 *****************************************************************************/
 
 ha_heap::ha_heap(handlerton *hton, TABLE_SHARE *table_arg)
-  :handler(hton, table_arg), file(0), records_changed(0), key_stat_version(0), 
-  internal_table(0)
+  :handler(hton, table_arg), file(0), int_table_flags2(0),
+   records_changed(0), key_stat_version(0), internal_table(0)
 {
 }
 
@@ -151,6 +151,12 @@ int ha_heap::open(const char *name, int mode, uint test_if_locked)
     used.
     */
   key_stat_version= file->s->key_stat_version-1;
+  if (file->s->blob_count)
+  {
+    /* Mark that table may have zerocopy blobs and unlock is not safe */
+    int_table_flags2|= HA2_CANNOT_ACCESS_ROWDATA_AFTER_UNLOCK;
+  }
+
 end:
   return (file ? 0 : 1);
 }

storage/heap/ha_heap.h

@@ -25,6 +25,7 @@ class ha_heap final : public handler
   HP_INFO *file;
   HP_SHARE *internal_share;
   key_map btree_keys;
+  ulonglong int_table_flags2;
   /* number of records changed since last statistics update */
   ulong   records_changed;
   ulong   saved_current_record;  /* for remember_rnd_pos() / restart_rnd_next() */
@@ -45,6 +46,10 @@ class ha_heap final : public handler
             HA_HAS_RECORDS | HA_STATS_RECORDS_IS_EXACT | HA_CAN_HASH_KEYS |
             HA_CAN_GEOMETRY | HA_CAN_BIT_FIELD);
   }
+  ulonglong table_flags2() const override
+  {
+    return int_table_flags2;
+  }
   ulong index_flags(uint inx, uint part, bool all_parts) const override
   {
     return ((table_share->key_info[inx].algorithm == HA_KEY_ALG_BTREE) ?