MDEV-40431 heap: classify rb-tree insert failures, mark the table crashed when key recovery fails

`hp_rb_write_key()` reported **every** rejected `tree_insert()` as
`HA_ERR_FOUND_DUPP_KEY`, although `tree_insert()` also returns NULL when
the allocation of the tree node fails. An out of memory during a
BTREE-indexed UPDATE was therefore reported as a duplicate: the user got
`ER_DUP_ENTRY` with a fabricated value, possibly naming a key that is
not unique at all, `handler::is_fatal_error()` treated the allocation
failure as not fatal for callers asking for `HA_CHECK_DUP_KEY`, and the
`HA_ERR_OUT_OF_MEM` / `ENOMEM` arms of the `heap_update()` recovery list
were unreachable on the rb-tree path.

`tree_insert()` now records why it returned NULL in the new
`TREE::error` (`TREE_ERROR_OOM` or `TREE_ERROR_DUP_KEY`), and
`hp_rb_write_key()` maps that to `HA_ERR_OUT_OF_MEM` or
`HA_ERR_FOUND_DUPP_KEY` (`HA_ERR_INTERNAL_ERROR` defensively, should a
new NULL source appear). With the classification corrected, the
allocation failure lands in the `HA_ERR_OUT_OF_MEM` arm of the
`heap_update()` recovery, which moves the already changed keys back, so
correcting the error report does not trade the wrong message for a
silently corrupt index.

The recovery at `err:` keeps its explicit list of error codes: those are
the errors we know how to recover from, and recovering from errors whose
meaning we do not know is worse than stopping. What changes around it:

1. `info->errkey` is only set for `HA_ERR_FOUND_DUPP_KEY`, the single
   error it describes; every other failure leaves the `-1` that `err:`
   starts with.
2. When the recovery itself cannot restore a key -- the re-insert of an
   old key value fails -- or the failure is outside the list, the index
   no longer describes the data and the table is marked **crashed** in
   the new `HP_SHARE::state_changed` (bits and macros modeled on the
   `state.changed` of Maria, see `storage/maria/maria_def.h`). A crashed
   table refuses every read and write with `HA_ERR_CRASHED` ("Index for
   table is corrupt"), `heap_check_heap()` reports it as damaged, and
   `hp_clear()` -- reached through TRUNCATE or DELETE without WHERE --
   clears the state along with the data, because it rebuilds the indexes
   from nothing. Refusing a table known to be corrupt beats continuing
   and delivering wrong results.
3. `delete_key()` is assumed to succeed: it allocates no memory, so it
   cannot fail unless the table is already inconsistent, and building
   recovery logic and injected failures for that case would complicate
   the code for a scenario that cannot happen on a healthy table. If it
   ever does fail, the error falls outside the recovery list and the
   table is marked crashed, preserving the damaged state for analysis.

The debug-build consistency check in `ha_heap::external_lock(F_UNLCK)`
skips tables already marked crashed: their inconsistency is known and
deliberate, and re-detecting it would raise a second error into a
diagnostics area that can already hold OK, firing the
`Diagnostics_area` assertion the check exists to prevent.

The allocation failure is injected inside `tree_insert()` itself:
`simulate_tree_insert_oom` fails every node allocation until the caller
disarms it, `once_simulate_tree_insert_oom` fails only the next one and
disarms itself. The two names must not be a prefix of one another,
because `DBUG_SET()` matches an existing keyword by prefix and would
silently merge instead of adding. Callers arm the keywords themselves
and keep an inert guard keyword in the list, because a keyword list
that becomes empty while debugging is on matches every keyword.

The unit test `hp_test_update` drives the failures through the engine
API: the classification and its duplicate-key counterpart, an already
moved key being moved back, and the crashed lifecycle -- marking,
refusal of reads and writes, and the reset on emptying. With the
defects reintroduced, 8 of its 24 assertions fail.
`heap.update_key_rollback` covers the same from SQL; without the fix its
first UPDATE reports `ER_DUP_ENTRY 'Duplicate entry 101 for key k2'` on
a key that is not unique.

Author: arcivanov

include/heap.h

@@ -143,6 +143,17 @@ typedef struct st_hp_blob_desc
   uint packlength;   /* 1, 2, 3, or 4: length prefix size */
 } HP_BLOB_DESC;
 
+/*
+  Bits for HP_SHARE::state_changed, modeled on the state.changed bitmaps
+  of Maria and MyISAM (see storage/maria/maria_def.h).  A table marked
+  crashed refuses every read and write with HA_ERR_CRASHED until it is
+  re-created or emptied (hp_clear()).
+*/
+#define HEAP_STATE_CRASHED 1U
+
+#define heap_mark_crashed(share) ((share)->state_changed|= HEAP_STATE_CRASHED)
+#define heap_is_crashed(share)   ((share)->state_changed & HEAP_STATE_CRASHED)
+
 typedef struct st_heap_share
 {
   HP_BLOCK block;
@@ -160,6 +171,7 @@ typedef struct st_heap_share
   uint reclength;			/* Length of one record */
   uint visible;     /* Offset to the flags byte (active/deleted/continuation) */
   uint changed;
+  uint state_changed;                   /* Bitmap of HEAP_STATE_* flags */
   uint keys,max_key_length;
   uint currently_disabled_keys;    /* saved value from "keys" when disabled */
   uint open_count;

include/my_tree.h

@@ -48,6 +48,8 @@ typedef uint32 element_count;
 typedef int (*tree_walk_action)(void *,element_count,void *);
 
 typedef enum { free_init, free_free, free_end } TREE_FREE;
+typedef enum { TREE_ERROR_NONE, TREE_ERROR_OOM, TREE_ERROR_DUP_KEY }
+TREE_ERROR;
 typedef int (*tree_element_free)(void*, TREE_FREE, void *);
 
 typedef struct st_tree_element {
@@ -70,6 +72,7 @@ typedef struct st_tree {
   tree_element_free free;
   myf my_flags;
   uint flag;
+  TREE_ERROR error;
 } TREE;
 
 	/* Functions on whole tree */

mysql-test/suite/heap/update_key_rollback.result

@@ -0,0 +1,105 @@
+#
+# Index consistency after a failed UPDATE on a HEAP table
+#
+# heap_update() moves the changed key entries to their new key values
+# before it updates the record.  A failure raised after that has to
+# move them back.  An rb-tree node allocation failure used to be
+# reported as a duplicate key, and when the recovery itself cannot
+# restore a key the table is now marked crashed, so that no statement
+# can use an index that no longer describes the data.
+#
+# The keyword lists armed below always carry the inert
+# "heap_test_guard" keyword: the one-shot injection removes its own
+# keyword when it fires, and a keyword list that becomes empty while
+# debugging is on matches every keyword.
+#
+CALL mtr.add_suppression("Index for table .t1. is corrupt");
+#
+# The hash key k1 is moved to the new value, then the rb-tree node
+# allocation for k2 fails once: the failure is reported as out of
+# memory, not as a duplicate, and both keys are moved back
+#
+CREATE TABLE t1 (
+id INT,
+a INT,
+b INT,
+KEY k1 (a),
+KEY k2 (b) USING BTREE
+) ENGINE=HEAP;
+INSERT INTO t1 VALUES (1, 10, 100), (2, 20, 200);
+SET SESSION debug_dbug="+d,once_simulate_tree_insert_oom,heap_test_guard";
+UPDATE t1 SET a= 11, b= 101 WHERE id = 1;
+ERROR HY000: Out of memory.
+SET SESSION debug_dbug="";
+CHECK TABLE t1;
+Table	Op	Msg_type	Msg_text
+test.t1	check	status	OK
+# Both indexes must still find the row under its unchanged values
+SELECT id, a, b FROM t1 FORCE INDEX(k1) WHERE a = 10;
+id	a	b
+1	10	100
+SELECT id, a, b FROM t1 FORCE INDEX(k2) WHERE b = 100;
+id	a	b
+1	10	100
+SELECT COUNT(*) FROM t1 FORCE INDEX(k1) WHERE a = 11;
+COUNT(*)
+0
+SELECT COUNT(*) FROM t1 FORCE INDEX(k2) WHERE b = 101;
+COUNT(*)
+0
+SELECT id, a, b FROM t1 ORDER BY id;
+id	a	b
+1	10	100
+2	20	200
+#
+# A duplicate key is still reported as a duplicate
+#
+CREATE TABLE t2 (
+id INT,
+a INT,
+UNIQUE KEY k1 (a) USING BTREE
+) ENGINE=HEAP;
+INSERT INTO t2 VALUES (1, 10), (2, 20);
+UPDATE t2 SET a= 20 WHERE id = 1;
+ERROR 23000: Duplicate entry '20' for key 'k1'
+CHECK TABLE t2;
+Table	Op	Msg_type	Msg_text
+test.t2	check	status	OK
+SELECT id, a FROM t2 ORDER BY id;
+id	a
+1	10
+2	20
+DROP TABLE t2;
+#
+# The allocation failure persists, so restoring the old k2 entry
+# fails as well: the already moved k1 no longer describes the
+# record, the table is marked crashed, and every read and write on
+# it is refused until it is emptied
+#
+SET SESSION debug_dbug="+d,simulate_tree_insert_oom,heap_test_guard";
+UPDATE t1 SET a= 11, b= 101 WHERE id = 1;
+ERROR HY000: Out of memory.
+SET SESSION debug_dbug="";
+CHECK TABLE t1;
+Table	Op	Msg_type	Msg_text
+test.t1	check	error	Corrupt
+SELECT id, a, b FROM t1 ORDER BY id;
+ERROR HY000: Index for table 't1' is corrupt; try to repair it
+SELECT id, a, b FROM t1 FORCE INDEX(k1) WHERE a = 10;
+ERROR HY000: Index for table 't1' is corrupt; try to repair it
+INSERT INTO t1 VALUES (3, 30, 300);
+ERROR HY000: Index for table 't1' is corrupt; try to repair it
+UPDATE t1 SET a= 12 WHERE id = 2;
+ERROR HY000: Index for table 't1' is corrupt; try to repair it
+DELETE FROM t1 WHERE id = 2;
+ERROR HY000: Index for table 't1' is corrupt; try to repair it
+# Emptying the table rebuilds the indexes from nothing
+TRUNCATE TABLE t1;
+CHECK TABLE t1;
+Table	Op	Msg_type	Msg_text
+test.t1	check	status	OK
+INSERT INTO t1 VALUES (5, 50, 500);
+SELECT id, a, b FROM t1 FORCE INDEX(k2) WHERE b = 500;
+id	a	b
+5	50	500
+DROP TABLE t1;

mysql-test/suite/heap/update_key_rollback.test

@@ -0,0 +1,99 @@
+--source include/have_debug.inc
+
+--echo #
+--echo # Index consistency after a failed UPDATE on a HEAP table
+--echo #
+--echo # heap_update() moves the changed key entries to their new key values
+--echo # before it updates the record.  A failure raised after that has to
+--echo # move them back.  An rb-tree node allocation failure used to be
+--echo # reported as a duplicate key, and when the recovery itself cannot
+--echo # restore a key the table is now marked crashed, so that no statement
+--echo # can use an index that no longer describes the data.
+--echo #
+--echo # The keyword lists armed below always carry the inert
+--echo # "heap_test_guard" keyword: the one-shot injection removes its own
+--echo # keyword when it fires, and a keyword list that becomes empty while
+--echo # debugging is on matches every keyword.
+--echo #
+
+CALL mtr.add_suppression("Index for table .t1. is corrupt");
+
+--echo #
+--echo # The hash key k1 is moved to the new value, then the rb-tree node
+--echo # allocation for k2 fails once: the failure is reported as out of
+--echo # memory, not as a duplicate, and both keys are moved back
+--echo #
+
+CREATE TABLE t1 (
+  id INT,
+  a INT,
+  b INT,
+  KEY k1 (a),
+  KEY k2 (b) USING BTREE
+) ENGINE=HEAP;
+
+INSERT INTO t1 VALUES (1, 10, 100), (2, 20, 200);
+
+SET SESSION debug_dbug="+d,once_simulate_tree_insert_oom,heap_test_guard";
+--error ER_OUT_OF_RESOURCES
+UPDATE t1 SET a= 11, b= 101 WHERE id = 1;
+SET SESSION debug_dbug="";
+
+CHECK TABLE t1;
+--echo # Both indexes must still find the row under its unchanged values
+SELECT id, a, b FROM t1 FORCE INDEX(k1) WHERE a = 10;
+SELECT id, a, b FROM t1 FORCE INDEX(k2) WHERE b = 100;
+SELECT COUNT(*) FROM t1 FORCE INDEX(k1) WHERE a = 11;
+SELECT COUNT(*) FROM t1 FORCE INDEX(k2) WHERE b = 101;
+SELECT id, a, b FROM t1 ORDER BY id;
+
+--echo #
+--echo # A duplicate key is still reported as a duplicate
+--echo #
+
+CREATE TABLE t2 (
+  id INT,
+  a INT,
+  UNIQUE KEY k1 (a) USING BTREE
+) ENGINE=HEAP;
+
+INSERT INTO t2 VALUES (1, 10), (2, 20);
+
+--error ER_DUP_ENTRY
+UPDATE t2 SET a= 20 WHERE id = 1;
+
+CHECK TABLE t2;
+SELECT id, a FROM t2 ORDER BY id;
+DROP TABLE t2;
+
+--echo #
+--echo # The allocation failure persists, so restoring the old k2 entry
+--echo # fails as well: the already moved k1 no longer describes the
+--echo # record, the table is marked crashed, and every read and write on
+--echo # it is refused until it is emptied
+--echo #
+
+SET SESSION debug_dbug="+d,simulate_tree_insert_oom,heap_test_guard";
+--error ER_OUT_OF_RESOURCES
+UPDATE t1 SET a= 11, b= 101 WHERE id = 1;
+SET SESSION debug_dbug="";
+
+CHECK TABLE t1;
+--error ER_NOT_KEYFILE
+SELECT id, a, b FROM t1 ORDER BY id;
+--error ER_NOT_KEYFILE
+SELECT id, a, b FROM t1 FORCE INDEX(k1) WHERE a = 10;
+--error ER_NOT_KEYFILE
+INSERT INTO t1 VALUES (3, 30, 300);
+--error ER_NOT_KEYFILE
+UPDATE t1 SET a= 12 WHERE id = 2;
+--error ER_NOT_KEYFILE
+DELETE FROM t1 WHERE id = 2;
+
+--echo # Emptying the table rebuilds the indexes from nothing
+TRUNCATE TABLE t1;
+CHECK TABLE t1;
+INSERT INTO t1 VALUES (5, 50, 500);
+SELECT id, a, b FROM t1 FORCE INDEX(k2) WHERE b = 500;
+
+DROP TABLE t1;

mysys/tree.c

@@ -106,6 +106,7 @@ void init_tree(TREE *tree, size_t default_alloc_size, size_t memory_limit,
   tree->custom_arg = custom_arg;
   tree->my_flags= my_flags;
   tree->flag= 0;
+  tree->error= TREE_ERROR_NONE;
   if (!free_element && size >= 0 &&
       ((uint) size <= sizeof(void*) || ((uint) size & (sizeof(void*)-1))))
   {
@@ -274,13 +275,35 @@ TREE_ELEMENT *tree_insert(TREE *tree, void *key, uint key_size,
     }
 
     key_size+=tree->size_of_element;
+    /*
+      Debug hooks that make this node allocation fail as if the allocator
+      had returned NULL.  "simulate_tree_insert_oom" fails every insert
+      until the caller disarms it; "once_simulate_tree_insert_oom" fails
+      only the next one and disarms itself.  Neither name may be a prefix
+      of the other: DBUG_SET() matches an existing list entry by prefix
+      and would merge into it instead of adding the keyword.
+    */
+    DBUG_EXECUTE_IF("once_simulate_tree_insert_oom",
+                    {
+                      DBUG_SET("-d,once_simulate_tree_insert_oom");
+                      tree->error= TREE_ERROR_OOM;
+                      return(NULL);
+                    });
+    DBUG_EXECUTE_IF("simulate_tree_insert_oom",
+                    {
+                      tree->error= TREE_ERROR_OOM;
+                      return(NULL);
+                    });
     if (tree->with_delete)
       element=(TREE_ELEMENT *) my_malloc(key_memory_TREE, alloc_size,
                                          MYF(tree->my_flags | MY_WME));
     else
       element=(TREE_ELEMENT *) alloc_root(&tree->mem_root,alloc_size);
     if (!element)
+    {
+      tree->error= TREE_ERROR_OOM;
       return(NULL);
+    }
     **parent=element;
     element->left=element->right= &null_element;
     if (!tree->offset_to_key)
@@ -303,7 +326,10 @@ TREE_ELEMENT *tree_insert(TREE *tree, void *key, uint key_size,
   else
   {
     if (tree->flag & TREE_NO_DUPS)
+    {
+      tree->error= TREE_ERROR_DUP_KEY;
       return(NULL);
+    }
     element->count++;
     /* Avoid a wrap over of the count. */
     if (! element->count)

storage/heap/CMakeLists.txt

@@ -32,7 +32,8 @@ IF(WITH_UNIT_TESTS)
   TARGET_LINK_LIBRARIES(hp_test1 heap mysys dbug strings)
   ADD_EXECUTABLE(hp_test2 hp_test2.c)
   TARGET_LINK_LIBRARIES(hp_test2 heap mysys dbug strings)
-  MY_ADD_TESTS(hp_test_hash hp_test_scan hp_test_freelist hp_test_concurrent LINK_LIBRARIES heap mysys dbug strings)
+  MY_ADD_TESTS(hp_test_hash hp_test_scan hp_test_freelist hp_test_concurrent
+               hp_test_update LINK_LIBRARIES heap mysys dbug strings)
 
   INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/sql
                       ${CMAKE_SOURCE_DIR}/include)

storage/heap/_check.c

@@ -51,6 +51,9 @@ int heap_check_heap(const HP_INFO *info, my_bool print_status)
   uchar *current_ptr= info->current_ptr;
   DBUG_ENTER("heap_check_heap");
 
+  if (heap_is_crashed(share))
+    DBUG_RETURN(1);                     /* Already known to be damaged */
+
   for (error=key= 0 ; key < share->keys ; key++)
   {
     if (share->keydef[key].algorithm == HA_KEY_ALG_BTREE)

storage/heap/ha_heap.cc

@@ -528,7 +528,15 @@ 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)
-  if (lock_type == F_UNLCK && file->s->changed && heap_check_heap(file, 0))
+  /*
+    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))
     return HA_ERR_CRASHED;
 #endif
   if (lock_type == F_UNLCK)

storage/heap/hp_clear.c

@@ -43,6 +43,7 @@ void hp_clear(HP_SHARE *info)
   info->data_length= 0;
   info->blength=1;
   info->changed=0;
+  info->state_changed=0;
   info->del_link=0;
   info->key_version++;
   info->file_version++;

storage/heap/hp_delete.c

@@ -146,6 +146,8 @@ int heap_delete(HP_INFO *info, const uchar *record)
   DBUG_ENTER("heap_delete");
   DBUG_PRINT("enter",("info: %p  record: %p", info, record));
 
+  if (heap_is_crashed(share))
+    DBUG_RETURN(my_errno= HA_ERR_CRASHED);
   test_active(info);
   hp_flush_pending_blob_free(info);