Bug#33856374: Query returns incorrect results in 8.0.22+

This problem requires a rather complex set of conditions to trigger:

- We have a CTE that is materialized and used in multiple query blocks.
- For the first query block that uses the CTE, there are multiple
  possible key definitions, which causes JOIN::finalize_derived_keys()
  to move the used key definition into position zero.
- The second query block that uses the CTE manipulates an index that
  uses the same position as the original position for the key chosen
  for the first query block.

Now, the function TABLE::copy_tmp_key() is supposed to move the first
key into a position that does not overlap with the subsequent keys.
And this is done for the KEY entry, however the associated entries
(KEY_PART_INFO, key names and rec per key information) are kept as is,
meaning that a subsequent key analysis for another query block may
overwrite the information in these data structures, possibly causing
invalid results to be returned.

The fix is to also move these extra data structures, so that subsequent
query block analyses use pristine data structures.

The function TABLE::copy_tmp_key() is renamed to TABLE::move_tmp_key(),
as this is really a move operation. In addition, old entries are
zeroed out so that a later invalid access will likely fail.

A synthetic test case is added, as the original case is too complex and
contains too much data to be included.

Change-Id: I908dca74560d20fecdd886e55cabfcaa348aba4d

Author: roylyseng

mysql-test/r/derived.result

@@ -4609,3 +4609,33 @@ GROUP BY b;
 1
 1
 DROP TABLE t1, t2;
+# Bug#33856374: Query returns incorrect results in 8.0.22+
+CREATE TABLE t1
+(a INTEGER,
+b INTEGER,
+c INTEGER
+);
+INSERT INTO t1 VALUES
+(1, 1, 10), (1, 2, 20), (1, 3, 30), (2, 1, 40), (2, 2, 50), (2, 3, 60);
+CREATE TABLE t2
+(a INTEGER,
+d INTEGER,
+e INTEGER
+);
+INSERT INTO t2 VALUES
+(1, 6, 60), (2, 6, 60), (3, 6, 60);
+WITH
+cte AS
+(SELECT SUM(c) AS c, SUM(b) AS b, a
+FROM t1
+GROUP BY a)
+SELECT t2.a, (SELECT MIN(c) FROM cte AS cte2 WHERE t2.d = cte2.b)
+FROM t2 LEFT JOIN cte AS cte1 ON t2.a = cte1.a
+LEFT JOIN t2 AS tx ON tx.e = cte1.c;
+a	(SELECT MIN(c) FROM cte AS cte2 WHERE t2.d = cte2.b)
+1	60
+1	60
+1	60
+2	60
+3	60
+DROP TABLE t1, t2;

mysql-test/t/derived.test

@@ -3307,3 +3307,34 @@ SELECT 1 FROM t2, (
 GROUP BY b;
 
 DROP TABLE t1, t2;
+
+--echo # Bug#33856374: Query returns incorrect results in 8.0.22+
+
+CREATE TABLE t1
+ (a INTEGER,
+  b INTEGER,
+  c INTEGER
+ );
+
+INSERT INTO t1 VALUES
+ (1, 1, 10), (1, 2, 20), (1, 3, 30), (2, 1, 40), (2, 2, 50), (2, 3, 60);
+
+CREATE TABLE t2
+ (a INTEGER,
+  d INTEGER,
+  e INTEGER
+ );
+
+INSERT INTO t2 VALUES
+ (1, 6, 60), (2, 6, 60), (3, 6, 60);
+
+WITH
+cte AS
+ (SELECT SUM(c) AS c, SUM(b) AS b, a
+  FROM t1
+  GROUP BY a)
+SELECT t2.a, (SELECT MIN(c) FROM cte AS cte2 WHERE t2.d = cte2.b)
+FROM t2 LEFT JOIN cte AS cte1 ON t2.a = cte1.a
+        LEFT JOIN t2 AS tx ON tx.e = cte1.c;
+
+DROP TABLE t1, t2;

sql/key.h

@@ -1,4 +1,4 @@
-/* Copyright (c) 2006, 2021, Oracle and/or its affiliates.
+/* Copyright (c) 2006, 2022, Oracle and/or its affiliates.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -299,6 +299,28 @@ class KEY {
     rec_per_key_float = rec_per_key_float_arg;
   }
 
+  /**
+    Move rec_per_key arrays from old to new position.
+
+    Ignore if arrays have not been set up yet.
+
+    @param rec_per_key_arg       pointer to adjusted rec per key array
+    @param rec_per_key_float_arg pointer to adjusted rec per key array (float)
+  */
+
+  void move_rec_per_key(ulong *rec_per_key_arg,
+                        rec_per_key_t *rec_per_key_float_arg) {
+    if (rec_per_key_float == nullptr || rec_per_key == nullptr) return;
+
+    rec_per_key_t *old_rec_per_key_float = rec_per_key_float;
+    ulong *old_rec_per_key = rec_per_key;
+    set_rec_per_key_array(rec_per_key_arg, rec_per_key_float_arg);
+    for (uint i = 0; i < actual_key_parts; i++) {
+      rec_per_key[i] = old_rec_per_key[i];
+      rec_per_key_float[i] = old_rec_per_key_float[i];
+    }
+  }
+
   /**
     Retrieve the estimate for how much of the index data that is available
     in a memory buffer.

sql/sql_optimizer.cc

@@ -9094,7 +9094,7 @@ void JOIN::finalize_derived_keys() {
             blocks must be considered here, as they need a key_info array
             consistent with the to-be-changed table->s->keys.
           */
-          t->copy_tmp_key(old_idx, it.is_first());
+          t->move_tmp_key(old_idx, it.is_first());
         }
       } else
         new_idx = old_idx;  // Index stays at same slot

sql/table.cc

@@ -38,6 +38,7 @@
 #include "ft_global.h"
 #include "m_string.h"
 #include "map_helpers.h"
+#include "memory_debugging.h"
 #include "my_alloc.h"
 #include "my_byteorder.h"
 #include "my_dbug.h"
@@ -5784,14 +5785,15 @@ bool TABLE::alloc_tmp_keys(uint new_key_count, uint new_key_part_count,
     s->key_names = static_cast<Key_name *>(
         s->mem_root.Alloc(sizeof(Key_name) * new_key_count));
     if (s->key_names == nullptr) return true; /* purecov: inspected */
-    memset(s->key_names, 0, sizeof(Key_name) * new_key_count);
+    TRASH(s->key_names, sizeof(Key_name) * new_key_count);
     /*
       A derived table may have a unique index with name stored in
       s->key_info->name. Check for this special case, and copy the name into
       the first location of key_names array.
     */
-    if (old_key_count > 0 && old_key_names == nullptr)
-      memcpy(&s->key_names->name, s->key_info->name, strlen(s->key_info->name));
+    if (old_key_count > 0 && old_key_names == nullptr) {
+      strcpy(pointer_cast<char *>(&s->key_names->name), s->key_info->name);
+    }
 
     s->key_info = s->mem_root.ArrayAlloc<KEY>(new_key_count);
     if (s->key_info == nullptr) return true; /* purecov: inspected */
@@ -6004,19 +6006,53 @@ uint TABLE_SHARE::find_first_unused_tmp_key(const Key_map &k) {
 }
 
 /**
-  For a materialized derived table: copies a KEY definition from a position to
+  For a materialized derived table: moves a KEY definition from a position to
   the first not-yet-used position (which is lower).
 
+  @note memset operations are used to invalidate old entries, in order to
+        trap invalid accesses after the move. memset is considered cheap
+        in this context.
+
+  The function needs to move the following entries:
+  - The KEY (both for TABLE and TABLE_SHARE)
+  - The KEY_PART_INFO objects (TABLE only, TABLE_SHARE shares with first TABLE)
+  - The key names (TABLE_SHARE only)
+  - rec per key information (TABLE_SHARE only)
+
   @param old_idx        source position
   @param modify_share   Do modifications to TABLE_SHARE. @see alloc_tmp_keys
 */
-void TABLE::copy_tmp_key(int old_idx, bool modify_share) {
-  if (modify_share)
-    s->key_info[s->first_unused_tmp_key++] = s->key_info[old_idx];
+void TABLE::move_tmp_key(int old_idx, bool modify_share) {
+  if (modify_share) {
+    const int new_idx = s->first_unused_tmp_key++;
+    s->key_info[new_idx] = s->key_info[old_idx];
+    TRASH(pointer_cast<void *>(s->key_info + old_idx), sizeof(KEY));
+    s->key_names[new_idx] = s->key_names[old_idx];
+    TRASH(pointer_cast<void *>(s->key_names + old_idx), sizeof(Key_name));
+    s->key_info[new_idx].name = s->key_names[new_idx].name;
+  }
   const int new_idx = s->first_unused_tmp_key - 1;
   assert(!created && new_idx < old_idx && old_idx < (int)s->keys);
+  uint key_partno = 0;
+  for (int i = 0; i < new_idx; i++) {
+    key_partno += s->key_info[i].user_defined_key_parts;
+  }
   key_info[new_idx] = key_info[old_idx];
+  KEY_PART_INFO *old_key_part = key_info[old_idx].key_part;
+  TRASH(pointer_cast<void *>(key_info + old_idx), sizeof(KEY));
+  key_info[new_idx].key_part = base_key_parts + key_partno;
+  key_info[new_idx].name = s->key_names[new_idx].name;
 
+  for (uint i = 0; i < s->key_info[new_idx].user_defined_key_parts; i++) {
+    base_key_parts[key_partno + i] = old_key_part[i];
+    TRASH(pointer_cast<void *>(old_key_part + i), sizeof(KEY_PART_INFO));
+  }
+  if (modify_share) {
+    s->key_info[new_idx].key_part = base_key_parts + key_partno;
+    s->key_info[new_idx].move_rec_per_key(
+        s->base_rec_per_key + key_partno,
+        s->base_rec_per_key_float + key_partno);
+  }
   for (auto reg_field = field; *reg_field; reg_field++) {
     auto f = *reg_field;
     f->key_start.clear_bit(new_idx);
@@ -6037,7 +6073,7 @@ void TABLE::copy_tmp_key(int old_idx, bool modify_share) {
 }
 
 /**
-  For a materialized derived table: after copy_tmp_key() has copied all
+  For a materialized derived table: after move_tmp_key() has moved all
   definitions of used KEYs, in TABLE::key_info we have a head of used keys
   followed by a tail of unused keys; this function chops the tail.
 

sql/table.h

@@ -1,7 +1,7 @@
 #ifndef TABLE_INCLUDED
 #define TABLE_INCLUDED
 
-/* Copyright (c) 2000, 2021, Oracle and/or its affiliates.
+/* Copyright (c) 2000, 2022, Oracle and/or its affiliates.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -1843,7 +1843,7 @@ struct TABLE {
   bool alloc_tmp_keys(uint new_key_count, uint new_key_part_count,
                       bool modify_share);
   bool add_tmp_key(Field_map *key_parts, bool invisible, bool modify_share);
-  void copy_tmp_key(int old_idx, bool modify_share);
+  void move_tmp_key(int old_idx, bool modify_share);
   void drop_unused_tmp_keys(bool modify_share);
 
   void set_keyread(bool flag);