MDEV-41050 versioned DELETE via row_end index leaves a row undeleted

1. Aria/MyISAM engines

A system-versioned DELETE on a MyISAM or Aria table indexed by row_end
could leave the last current row undeleted. The delete scans the current
rows via an equality search on row_end = MAX, which is driven by
mi_rnext_same/maria_rnext_same. That function keeps the search's
reference key in lastkey2 and uses the HA_STATE_RNEXT_SAME flag to
remember it has already stored it. This reference-key mechanism is
exactly what lets the scan modify rows it is walking without skipping
them.

Deleting a versioned row is an in-place update of row_end, and the update
reuses lastkey2 as scratch space for the changed key, so it clears
HA_STATE_RNEXT_SAME to request that rnext_same re-store its reference on
the next call. However, TABLE::delete_row wraps the update in
HA_EXTRA_REMEMBER_POS/HA_EXTRA_RESTORE_POS, and RESTORE_POS restored the
whole saved info->update word, resurrecting the HA_STATE_RNEXT_SAME bit
that the update had just cleared.

As a result rnext_same skipped rebuilding its reference key and compared
subsequent keys against the now-overwritten lastkey2, hitting a spurious
end-of-file and terminating the scan one row early, defeating the engine's
own protection against a Halloween-style skip.

Fixed by preserving the current HA_STATE_RNEXT_SAME bit across
RESTORE_POS instead of restoring the stale saved value. See also the HEAP
fix below: same root cause, different per-engine mechanism.

2. HEAP engine

The row loss also reproduces on the MEMORY (HEAP) engine. A system-
versioned DELETE scans the current rows on the row_end index and turns
each delete into an in-place update of row_end, so it modifies the very
index it is walking.

When the changed key is the scanned one (info->lastinx), hp_delete_key()
repositions the cursor but heap_update() leaves info->update untouched, so
HA_STATE_NEXT_FOUND from the preceding heap_rnext() stays set. The next
heap_rnext() then sees current_ptr == 0 with that bit and takes the
"!current_ptr && HA_STATE_NEXT_FOUND" guard as a false end-of-file,
stopping one row early.

Fixed by clearing HA_STATE_NEXT_FOUND when the scanned index key changed.
HA_STATE_AKTIV is kept (unlike heap_delete): the row is updated, not
removed, so a following op must not fail test_active(). The bit is only
set after a heap_rnext(), so a plain single-row UPDATE never reaches this.
See also the Aria/MyISAM fix above: same root cause, different per-engine
mechanism.

3. Why the fix differs per engine, and InnoDB

Aria and HEAP both trace their handler design back to MyISAM, hence the
same root cause (stale scan bookkeeping after an in-place key change) in
all three, fixed at each engine's own bookkeeping spot. InnoDB needs no
fix: its persistent cursor survives concurrent index modification by
design, already covered by this same test under the timestamp
combination (default-storage-engine=innodb), which passes unmodified.

Author: midenok

mysql-test/suite/versioning/r/delete.result

@@ -168,3 +168,25 @@ f1	f3	check_row(row_start, row_end)
 2	2	HISTORICAL ROW
 # Cleanup
 drop tables t1, t2;
+#
+# MDEV-41050 versioned DELETE via row_end index leaves a row undeleted
+#
+create table t (
+id int primary key,
+row_start timestamp(6) generated always as row start,
+row_end   timestamp(6) generated always as row end,
+period for system_time(row_start, row_end),
+key row_end_idx (row_end)
+) engine=ENGINE with system versioning;
+insert into t (id) values (1), (2), (3);
+delete from t force index (row_end_idx);
+# Expected: no current rows remain, all history rows sane.
+select id from t;
+id
+select id, check_row_ts(row_start, row_end) from t for system_time all order by id;
+id	check_row_ts(row_start, row_end)
+1	HISTORICAL ROW
+2	HISTORICAL ROW
+3	HISTORICAL ROW
+drop table t;
+# End of 11.8 tests

mysql-test/suite/versioning/t/delete.test

@@ -121,4 +121,32 @@ select f1, f3, check_row(row_start, row_end) from t1 for system_time all order b
 --echo # Cleanup
 drop tables t1, t2;
 
+--echo #
+--echo # MDEV-41050 versioned DELETE via row_end index leaves a row undeleted
+--echo #
+--let $engine=`select @@default_storage_engine`
+# No dedicated Aria combination exists, so borrow trx_id's run for Aria coverage.
+if ($MTR_COMBINATION_TRX_ID)
+{
+  --let $engine= Aria
+}
+
+--replace_result $engine ENGINE
+eval create table t (
+  id int primary key,
+  row_start timestamp(6) generated always as row start,
+  row_end   timestamp(6) generated always as row end,
+  period for system_time(row_start, row_end),
+  key row_end_idx (row_end)
+) engine=$engine with system versioning;
+
+insert into t (id) values (1), (2), (3);
+delete from t force index (row_end_idx);
+--echo # Expected: no current rows remain, all history rows sane.
+select id from t;
+select id, check_row_ts(row_start, row_end) from t for system_time all order by id;
+drop table t;
+
+--echo # End of 11.8 tests
+
 --source suite/versioning/common_finish.inc

storage/heap/hp_update.c

@@ -23,7 +23,7 @@ int heap_update(HP_INFO *info, const uchar *old, const uchar *heap_new)
   HP_KEYDEF *keydef, *end, *p_lastinx;
   uchar *pos, *recovery_ptr;
   struct st_hp_hash_info *recovery_hash_ptr;
-  my_bool auto_key_changed= 0, key_changed= 0;
+  my_bool auto_key_changed= 0, key_changed= 0, lastinx_changed= 0;
   HP_SHARE *share= info->s;
   DBUG_ENTER("heap_update");
 
@@ -40,14 +40,24 @@ int heap_update(HP_INFO *info, const uchar *old, const uchar *heap_new)
   recovery_hash_ptr= info->current_hash_ptr;
 
   p_lastinx= share->keydef + info->lastinx;
+  /* Re-index the record on every key whose value differs between old and new */
   for (keydef= share->keydef, end= keydef + share->keys; keydef < end; keydef++)
   {
+    /* Skip keys that are unchanged by this update */
     if (hp_rec_key_cmp(keydef, old, heap_new))
     {
+      /* Remove the old key entry, then insert the new one */
       if ((*keydef->delete_key)(info, keydef, old, pos, keydef == p_lastinx) ||
           (*keydef->write_key)(info, keydef, heap_new, pos))
         goto err;
       key_changed= 1;
+      /*
+        p_lastinx is the index the caller is currently scanning (info->lastinx),
+        the one holding the live cursor. Detect when its key moved, so we can fix
+        up the scan state below.
+      */
+      if (keydef == p_lastinx)
+        lastinx_changed= 1;
       if (share->auto_key == (uint) (keydef - share->keydef + 1))
         auto_key_changed= 1;
     }
@@ -63,6 +73,26 @@ int heap_update(HP_INFO *info, const uchar *old, const uchar *heap_new)
     heap_update_auto_increment(info, heap_new);
   if (key_changed)
     share->key_version++;
+  if (lastinx_changed && (info->update & HA_STATE_NEXT_FOUND))
+  {
+    /*
+      The scanned-index key of the current record changed in place (e.g. a
+      versioned DELETE bumping row_end while scanning row_end), so the record
+      left its position mid-scan. Clear HA_STATE_NEXT_FOUND, otherwise the next
+      heap_rnext() hits the "!current_ptr && HA_STATE_NEXT_FOUND" guard in its
+      hash (non-BTREE) branch, reports a false end-of-file and leaves a row
+      behind. This is the same scan-state contract heap_delete() already
+      maintains for heap_rnext() (it sets info->update there too); an
+      in-place update that moves the scanned key must keep it consistent
+      for the same reason. We only get here on an index scan; rnd scans
+      (heap_scan()/heap_rrnd()) leave lastinx == -1, so despite also setting
+      HA_STATE_NEXT_FOUND they never reach this. A plain single-row UPDATE has
+      no preceding heap_rnext(), so the bit is unset and this is a no-op there.
+
+      See also: mi_extra.c/ma_extra.c HA_EXTRA_RESTORE_POS (MDEV-41050).
+    */
+    info->update&= ~HA_STATE_NEXT_FOUND;
+  }
   DBUG_RETURN(0);
 
  err:

storage/maria/ma_extra.c

@@ -42,6 +42,7 @@ int maria_extra(MARIA_HA *info, enum ha_extra_function function,
   ulong cache_size;
   MARIA_SHARE *share= info->s;
   my_bool block_records= share->data_file_type == BLOCK_RECORD;
+  uint save_update;
   DBUG_ENTER("maria_extra");
   DBUG_PRINT("enter",("function: %d",(int) function));
 
@@ -209,7 +210,25 @@ int maria_extra(MARIA_HA *info, enum ha_extra_function function,
       bmove(info->last_key.data,
 	    info->last_key.data + share->base.max_key_length*2,
 	    info->save_lastkey_data_length + info->save_lastkey_ref_length);
-      info->update=	info->save_update | HA_STATE_WRITTEN;
+      /*
+        Preserve current HA_STATE_RNEXT_SAME state: a wrapped ha_update_row may
+        have reused lastkey_buff2 and cleared the bit. Restoring the saved value
+        would resurrect it, so maria_rnext_same would compare against a stale
+        reference key and end the scan early, leading to Halloween-like skip.
+        Only for RESTORE_POS: NO_KEYREAD ends a key-read scan and must restore
+        save_update as is, otherwise the temporary scan's bit/reference leaks in.
+        This targets only the known REMEMBER_POS/RESTORE_POS pair around a
+        wrapped ha_update_row (TABLE::delete_row); it does not make the
+        lastkey_buff2 reuse itself reentrant for other callers.
+
+        See also: hp_update.c HA_STATE_NEXT_FOUND, mi_extra.c (MDEV-41050).
+      */
+      if (function == HA_EXTRA_RESTORE_POS)
+        save_update= (info->save_update & ~HA_STATE_RNEXT_SAME) |
+                     (info->update & HA_STATE_RNEXT_SAME);
+      else
+        save_update= info->save_update;
+      info->update=	save_update | HA_STATE_WRITTEN;
       if (info->lastinx != info->save_lastinx)             /* Index changed */
       {
         info->lastinx = info->save_lastinx;

storage/myisam/mi_extra.c

@@ -40,6 +40,7 @@ int mi_extra(MI_INFO *info, enum ha_extra_function function, void *extra_arg)
   int error=0;
   ulong cache_size;
   MYISAM_SHARE *share=info->s;
+  uint save_update;
   DBUG_ENTER("mi_extra");
   DBUG_PRINT("enter",("function: %d",(int) function));
 
@@ -200,7 +201,22 @@ int mi_extra(MI_INFO *info, enum ha_extra_function function, void *extra_arg)
       bmove((uchar*) info->lastkey,
 	    (uchar*) info->lastkey+share->base.max_key_length*2,
 	    info->save_lastkey_length);
-      info->update=	info->save_update | HA_STATE_WRITTEN;
+      /*
+        Preserve current HA_STATE_RNEXT_SAME state: a wrapped ha_update_row may
+        have reused lastkey2 and cleared the bit. Restoring the saved value would
+        resurrect it, so mi_rnext_same would compare against a stale reference key
+        and end the scan early, leading to Halloween-like skip. Only for
+        RESTORE_POS: NO_KEYREAD ends a key-read scan and must restore save_update
+        as is, otherwise the temporary scan's bit/reference would leak in.
+
+        See also: hp_update.c HA_STATE_NEXT_FOUND, ma_extra.c (MDEV-41050).
+      */
+      if (function == HA_EXTRA_RESTORE_POS)
+        save_update= (info->save_update & ~HA_STATE_RNEXT_SAME) |
+                     (info->update & HA_STATE_RNEXT_SAME);
+      else
+        save_update= info->save_update;
+      info->update=	save_update | HA_STATE_WRITTEN;
       info->lastinx=	info->save_lastinx;
       info->lastpos=	info->save_lastpos;
       info->lastkey_length=info->save_lastkey_length;