MDEV-15990 handle timestamp-based collisions as well

FooBarrior · midenok · commit 8001679af648 · 2025-09-17T18:29:47.000+03:00
Timestamp-versioned row deletion was exposed to a collisional problem: if
current timestamp wasn't changed, then a sequence of row delete+insert could
get a duplication error. A row delete would find another conflicting history row
and return an error.

This is true both for REPLACE and DELETE statements, however in REPLACE, the
"optimized" path is usually taken, especially in the tests. There, delete+insert
is substituted for a single versioned row update. In the end, both paths end up
as ha_update_row + ha_write_row.

The solution is to handle a history collision somehow.

From the design perspective, the user shouldn't experience history rows loss,
unless there's a technical limitation.

To the contrary, trxid-based changes should never generate history for the same
transaction, see MDEV-15427.

If two operations on the same row happened too quickly, so that they happen at
the same timestamp, the history row shouldn't be lost. We can still write a
history row, though it'll have row_start == row_end.

We cannot store more than one such historical row, as this will violate the
unique constraint on row_end. So we will have to phisically delete the row if
the history row is already available.

In this commit:
1. Improve TABLE::delete_row to handle the history collision: if an update
   results with a duplicate error, delete a row for real.
2. use TABLE::delete_row in a non-optimistic path of REPLACE, where the
   system-versioned case now belongs entirely.
diff --git a/mysql-test/suite/versioning/common.inc b/mysql-test/suite/versioning/common.inc
@@ -66,6 +66,11 @@ if ($MTR_COMBINATION_TRX_ID)
   let $sys_datatype_expl= bigint(20) unsigned;
   let $sys_datatype_expl_uc= BIGINT(20) UNSIGNED;
   let $sys_datatype_max= 18446744073709551615;
+  let $current_row= current_row;
+}
+if ($MTR_COMBINATION_TIMESTAMP)
+{
+  let $current_row= current_row_ts;
 }
 
 eval create or replace function current_row(sys_trx_end $sys_datatype_expl)
@@ -76,7 +81,7 @@ deterministic
 eval create or replace function current_row_ts(sys_trx_end timestamp(6))
 returns int
 deterministic
-  return convert_tz(sys_trx_end, '+00:00', @@time_zone) = $sys_time_max;
+  return convert_tz(sys_trx_end, @@time_zone, '+00:00') = $sys_time_max;
 
 delimiter ~~;
 eval create or replace function check_row(row_start $sys_datatype_expl, row_end $sys_datatype_expl)
@@ -87,7 +92,7 @@ begin
     return "ERROR: row_end < row_start";
   elseif row_end = row_start then
     return "ERROR: row_end == row_start";
-  elseif current_row(row_end) then
+  elseif $current_row(row_end) then
     return "CURRENT ROW";
   end if;
   return "HISTORICAL ROW";
@@ -99,7 +104,7 @@ eval create or replace function check_row_slave(row_start $sys_datatype_expl, ro
 returns varchar(255)
 deterministic
 begin
-  if current_row(row_end) then
+  if $current_row(row_end) then
     return "CURRENT ROW";
   end if;
   return "HISTORICAL ROW";
diff --git a/mysql-test/suite/versioning/r/replace,trx_id.rdiff b/mysql-test/suite/versioning/r/replace,trx_id.rdiff
@@ -0,0 +1,26 @@
+--- replace.result
++++ replace.reject
+@@ -75,7 +75,6 @@
+ replace into t1 (pk,i) values (1,10),(1,100),(1,1000);
+ select pk, i, check_row(row_start, row_end) from t1 for system_time all;
+ pk	i	check_row(row_start, row_end)
+-1	10	ERROR: row_end == row_start
+ 1	1000	CURRENT ROW
+ drop table t1;
+ create or replace table t1 (
+@@ -94,7 +93,6 @@
+ commit;
+ select pk, i, check_row(row_start, row_end) from t1 for system_time all;
+ pk	i	check_row(row_start, row_end)
+-1	3	ERROR: row_end == row_start
+ 1	300	CURRENT ROW
+ drop table t1;
+ # In this case there'll be no unique constraint on a historical row.
+@@ -114,8 +112,6 @@
+ commit;
+ select pk, i, check_row(row_start, row_end) from t1 for system_time all;
+ pk	i	check_row(row_start, row_end)
+-1	3	ERROR: row_end == row_start
+-1	30	ERROR: row_end == row_start
+ 1	300	CURRENT ROW
+ drop table t1;
diff --git a/mysql-test/suite/versioning/r/replace.result b/mysql-test/suite/versioning/r/replace.result
@@ -63,6 +63,7 @@ connection default;
 replace into t1 values (1),(2);
 connection con1;
 replace into t1 values (1),(2);
+ERROR 23000: Duplicate entry '1' for key 'PRIMARY'
 drop table t1;
 #
 # MDEV-14794 Limitations which the row end as a part of PK imposes due to
@@ -95,13 +96,56 @@ drop table t1;
 # error (ER_DUP_ENTRY)
 #
 create or replace table t1 (
-pk int primary key,  i int,
-row_start bigint unsigned as row start,
-row_end bigint unsigned as row end,
+pk int KEY_TYPE, i int,
+row_start SYS_DATATYPE as row start invisible,
+row_end SYS_DATATYPE as row end invisible,
 period for system_time(row_start, row_end)
-) engine=InnoDB with system versioning;
+) with system versioning;
 replace into t1 (pk,i) values (1,10),(1,100),(1,1000);
-select pk, i, row_end from t1 for system_time all;
-pk	i	row_end
-1	1000	18446744073709551615
+select pk, i, check_row(row_start, row_end) from t1 for system_time all;
+pk	i	check_row(row_start, row_end)
+1	1000	CURRENT ROW
+drop table t1;
+create or replace table t1 (
+pk int KEY_TYPE, i int,
+row_start SYS_DATATYPE as row start invisible,
+row_end SYS_DATATYPE as row end invisible,
+period for system_time(row_start, row_end)
+) with system versioning;
+set timestamp= (select unix_timestamp());
+begin;
+insert t1(pk, i) values(1,3);
+delete from t1;
+insert t1(pk, i) values(1,30);
+delete from t1;
+insert t1(pk, i) values(1,300);
+commit;
+select pk, i, check_row(row_start, row_end) from t1 for system_time all;
+pk	i	check_row(row_start, row_end)
+1	3	ERROR: row_end == row_start
+1	300	CURRENT ROW
 drop table t1;
+# In this case there'll be no unique constraint on a historical row.
+create or replace table t1 (
+pk int, i int,
+row_start SYS_DATATYPE as row start invisible,
+row_end SYS_DATATYPE as row end invisible,
+period for system_time(row_start, row_end)
+) with system versioning;
+set timestamp= (select unix_timestamp());
+begin;
+insert t1(pk, i) values(1,3);
+delete from t1;
+insert t1(pk, i) values(1,30);
+delete from t1;
+insert t1(pk, i) values(1,300);
+commit;
+select pk, i, check_row(row_start, row_end) from t1 for system_time all;
+pk	i	check_row(row_start, row_end)
+1	3	ERROR: row_end == row_start
+1	30	ERROR: row_end == row_start
+1	300	CURRENT ROW
+drop table t1;
+#
+# End of 10.5 tests
+#
diff --git a/mysql-test/suite/versioning/t/insert2.test b/mysql-test/suite/versioning/t/insert2.test
@@ -91,4 +91,4 @@ select pk,i,row_end > '2038-01-01' from t1 for system_time all;
 drop table t1;
 
 # Cleanup
-set @@time_zone=default;
+set @@time_zone=default;
diff --git a/mysql-test/suite/versioning/t/replace.test b/mysql-test/suite/versioning/t/replace.test
@@ -78,9 +78,11 @@ replace into t1 values (1),(2);
 --connection con1
 --error ER_DUP_ENTRY
 replace into t1 values (1),(2);
-
 drop table t1;
 
+--echo #
+--echo # MDEV-14794 Limitations which the row end as a part of PK imposes due to
+--echo # CURRENT_TIMESTAMP behavior and otherwise
 --echo #
 --echo # MDEV-15330 Server crash or assertion `table->insert_values' failure in write_record upon LOAD DATA
 --echo #
@@ -121,15 +123,56 @@ drop table t1;
 --echo # error (ER_DUP_ENTRY)
 --echo #
 
-create or replace table t1 (
-    pk int primary key,  i int,
-    row_start bigint unsigned as row start,
-    row_end bigint unsigned as row end,
+--replace_result $sys_datatype_expl SYS_DATATYPE "$KEY_TYPE" KEY_TYPE
+eval create or replace table t1 (
+    pk int $KEY_TYPE, i int,
+    row_start $sys_datatype_expl as row start invisible,
+    row_end $sys_datatype_expl as row end invisible,
     period for system_time(row_start, row_end)
-) engine=InnoDB with system versioning;
+) with system versioning;
 replace into t1 (pk,i) values (1,10),(1,100),(1,1000);
-select pk, i, row_end from t1 for system_time all;
+select pk, i, check_row(row_start, row_end) from t1 for system_time all;
+drop table t1;
+
+--replace_result $sys_datatype_expl SYS_DATATYPE "$KEY_TYPE" KEY_TYPE
+eval create or replace table t1 (
+    pk int $KEY_TYPE, i int,
+    row_start $sys_datatype_expl as row start invisible,
+    row_end $sys_datatype_expl as row end invisible,
+    period for system_time(row_start, row_end)
+) with system versioning;
+set timestamp= (select unix_timestamp());
+begin;
+  insert t1(pk, i) values(1,3);
+  delete from t1;
+  insert t1(pk, i) values(1,30);
+  delete from t1;
+  insert t1(pk, i) values(1,300);
+commit;
+select pk, i, check_row(row_start, row_end) from t1 for system_time all;
+drop table t1;
+
+--echo # In this case there'll be no unique constraint on a historical row.
+--replace_result $sys_datatype_expl SYS_DATATYPE
+eval create or replace table t1 (
+    pk int, i int,
+    row_start $sys_datatype_expl as row start invisible,
+    row_end $sys_datatype_expl as row end invisible,
+    period for system_time(row_start, row_end)
+) with system versioning;
+set timestamp= (select unix_timestamp());
+begin;
+  insert t1(pk, i) values(1,3);
+  delete from t1;
+  insert t1(pk, i) values(1,30);
+  delete from t1;
+  insert t1(pk, i) values(1,300);
+commit;
+select pk, i, check_row(row_start, row_end) from t1 for system_time all;
 drop table t1;
 
+--echo #
+--echo # End of 10.5 tests
+--echo #
 
 --source suite/versioning/common_finish.inc
diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
@@ -289,33 +289,86 @@ int update_portion_of_time(THD *thd, TABLE *table,
   return res;
 }
 
-inline
-int TABLE::delete_row()
+/**
+  Delete a record stored in:
+  replace= true: record[0]
+  replace= false: record[1]
+
+  with regard to the treat_versioned flag, which can be false for a versioned
+  table in case of versioned->versioned replication.
+
+ For a versioned case, we detect a few conditions, under which we should delete
+ a row instead of updating it to a history row.
+ This includes:
+ * History deletion by user;
+ * History collision, in case of REPLACE or very fast sequence of dmls
+   so that timestamp doesn't change;
+ * History collision in the parent table
+
+ A normal delete is processed here as well.
+*/
+template <bool replace>
+int TABLE::delete_row(bool treat_versioned)
 {
-  if (!versioned(VERS_TIMESTAMP) ||
-      !vers_end_field()->is_max())
-    return file->ha_delete_row(record[0]);
+  int err= 0;
+  uchar *del_buf= record[replace ? 1 : 0];
+  bool delete_row= !treat_versioned
+                   || in_use->lex->vers_conditions.delete_history
+                   || versioned(VERS_TRX_ID)
+                   || !vers_end_field()->is_max(
+                           vers_end_field()->ptr_in_record(del_buf));
 
-  store_record(this, record[1]);
-  vers_update_end();
-  int err;
   if ((err= file->extra(HA_EXTRA_REMEMBER_POS)))
     return err;
-  if ((err= file->ha_update_row(record[1], record[0])))
+
+  if (!delete_row)
   {
-    /*
-      MDEV-23644: we get HA_ERR_FOREIGN_DUPLICATE_KEY iff we already got
-      history row with same trx_id which is the result of foreign key action,
-      so we don't need one more history row.
-    */
-    if (err == HA_ERR_FOREIGN_DUPLICATE_KEY)
-      file->ha_delete_row(record[0]);
+    if (replace)
+    {
+      store_record(this, record[2]);
+      restore_record(this, record[1]);
+    }
     else
-      return err;
+    {
+      store_record(this, record[1]);
+    }
+    vers_update_end();
+    err= file->ha_update_row(record[1], record[0]);
+    if (unlikely(err))
+    {
+      /*
+        MDEV-23644: we get HA_ERR_FOREIGN_DUPLICATE_KEY iff we already got
+        history row with same trx_id which is the result of foreign key
+        action, so we don't need one more history row.
+
+        Additionally, delete the row if versioned record already exists.
+        This happens on replace, a very fast sequence of inserts and deletes,
+        or if timestamp is frozen.
+      */
+      delete_row= err == HA_ERR_FOUND_DUPP_KEY
+                  || err == HA_ERR_FOUND_DUPP_UNIQUE
+                  || err == HA_ERR_FOREIGN_DUPLICATE_KEY;
+      if (!delete_row)
+        return err;
+
+      if (!replace)
+        del_buf= record[1];
+    }
+
+    if (replace)
+      restore_record(this, record[2]);
   }
-  return file->extra(HA_EXTRA_RESTORE_POS);
+
+  if (delete_row)
+    err= file->ha_delete_row(del_buf);
+
+  (void) file->extra(HA_EXTRA_RESTORE_POS);
+
+  return err;
 }
 
+template int TABLE::delete_row<true>(bool treat_versioned);
+template int TABLE::delete_row<false>(bool treat_versioned);
 
 /**
   @brief Special handling of single-table deletes after prepare phase
diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
@@ -2144,7 +2144,21 @@ int Write_record::replace_row(ha_rows *inserted, ha_rows *deleted)
       error= table->file->ha_update_row(table->record[1], table->record[0]);
 
       if (likely(!error))
+      {
         ++*deleted;
+        if (table->versioned() && table->vers_write)
+        {
+          if (versioned)
+          {
+            store_record(table, record[2]);
+            error= vers_insert_history_row(table);
+            restore_record(table, record[2]);
+            if (unlikely(error))
+              return on_ha_error(error);
+          }
+          ++*inserted;
+        }
+      }
       else if (error != HA_ERR_RECORD_IS_THE_SAME)
         return on_ha_error(error);
       break;
@@ -2160,16 +2174,8 @@ int Write_record::replace_row(ha_rows *inserted, ha_rows *deleted)
                                                 TRG_ACTION_BEFORE, true,
                                                 &trg_skip_row))
         return restore_on_error();
-      if (!versioned)
-        error = table->file->ha_delete_row(table->record[1]);
-      else
-      {
-        store_record(table, record[2]);
-        restore_record(table, record[1]);
-        table->vers_update_end();
-        error = table->file->ha_update_row(table->record[1], table->record[0]);
-        restore_record(table, record[2]);
-      }
+
+      error= table->delete_row<true>(versioned);
 
       if (unlikely(error))
         return on_ha_error(error);
diff --git a/sql/sql_insert.h b/sql/sql_insert.h
@@ -121,11 +121,11 @@ class Write_record
   {
     if (info->handle_duplicates == DUP_REPLACE)
     {
-      bool has_delete_triggers= use_triggers &&
+      const bool has_delete_triggers= use_triggers &&
                                 table->triggers->has_delete_triggers();
-      bool referenced_by_fk= table->file->referenced_by_foreign_key();
-      can_optimize= !referenced_by_fk && !has_delete_triggers
-                    && !versioned && !table->versioned();
+      const bool referenced_by_fk= table->file->referenced_by_foreign_key();
+      can_optimize= !referenced_by_fk && !has_delete_triggers &&
+                    !table->versioned(VERS_TRX_ID);
     }
   }
   Write_record(THD *thd, TABLE *table, COPY_INFO *info,
diff --git a/sql/table.h b/sql/table.h
@@ -1986,7 +1986,9 @@ struct TABLE
                              ha_rows *rows_inserted);
   bool vers_check_update(List<Item> &items);
   static bool check_period_overlaps(const KEY &key, const uchar *lhs, const uchar *rhs);
-  int delete_row();
+  inline int delete_row(){ return delete_row<false>(versioned(VERS_TIMESTAMP)); }
+  template <bool replace>
+  int delete_row(bool versioned);
   /* Used in majority of DML (called from fill_record()) */
   bool vers_update_fields();
   /* Used in DELETE, DUP REPLACE and insert history row */

Original file line number	Diff line number	Diff line change
`@@ -121,11 +121,11 @@ class Write_record`
`121`	`121`	`{`
`122`	`122`	`if (info->handle_duplicates == DUP_REPLACE)`
`123`	`123`	`{`
`124`		`- bool has_delete_triggers= use_triggers &&`
	`124`	`+ const bool has_delete_triggers= use_triggers &&`
`125`	`125`	`table->triggers->has_delete_triggers();`
`126`		`- bool referenced_by_fk= table->file->referenced_by_foreign_key();`
`127`		`- can_optimize= !referenced_by_fk && !has_delete_triggers`
`128`		`- && !versioned && !table->versioned();`
	`126`	`+ const bool referenced_by_fk= table->file->referenced_by_foreign_key();`
	`127`	`+ can_optimize= !referenced_by_fk && !has_delete_triggers &&`
	`128`	`+ !table->versioned(VERS_TRX_ID);`
`129`	`129`	`}`
`130`	`130`	`}`
`131`	`131`	`Write_record(THD thd, TABLE table, COPY_INFO *info,`