MDEV-25644 UPDATE not working properly on transaction precise system versioned table

First UPDATE under START TRANSACTION does nothing (nstate= nstate),
but anyway generates history. Since update vector is empty we get into
(!uvect->n_fields) branch which only adds history row, but does not do
update. After that we get current row with wrong (old) row_start value
and because of that second UPDATE tries to insert history row again
because it sees trx->id != row_start which is the guard to avoid
inserting multiple trx_id-based history rows under same transaction
(because we have same trx_id and we get duplicate error and this bug
demostrates that). But this try anyway fails because PK is based on
row_end which is constant under same transaction, so PK didn't change.

The fix moves vers_make_update() to an earlier stage of
calc_row_difference(). Therefore it prepares update vector before
(!uvect->n_fields) check and never gets into that branch, hence no
need to handle versioning inside that condition anymore.

Now trx->id and row_start are equal after first UPDATE and we don't
try to insert second history row.

== Cleanups and improvements ==

ha_innobase::update_row():

vers_set_fields and vers_ins_row are cleaned up into direct condition
check. SQLCOM_ALTER_TABLE check now is not used as this is dead code,
assertion is done instead.

upd_node->is_delete is set in calc_row_difference() just to keep
versioning code as much in one place as possible. vers_make_delete()
is still located in row_update_for_mysql() as this is required for
ha_innodbase::delete_row() as well.

row_ins_duplicate_error_in_clust():

Restrict DB_FOREIGN_DUPLICATE_KEY to the better conditions.
VERSIONED_DELETE is used specifically to help lower stack to
understand what caused current insert. Related to MDEV-29813.

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

@@ -133,18 +133,38 @@ drop table t1;
 #
 # MDEV-21138 Assertion `col->ord_part' or `f.col->ord_part' failed in row_build_index_entry_low
 #
+# Check DELETE and multi-DELETE with foreign key
 create table t1 (
 f1 int, f2 text, f3 int, fulltext (f2), key(f1), key(f3),
-foreign key r (f3) references t1 (f1) on delete set null)
+foreign key r (f3) references t1 (f1) on delete set null,
+row_start SYS_TYPE as row start invisible,
+row_end SYS_TYPE as row end invisible,
+period for system_time (row_start, row_end))
 with system versioning engine innodb;
 insert into t1 values (1, repeat('a', 8193), 1), (1, repeat('b', 8193), 1);
-select f1, f3, check_row_ts(row_start, row_end) from t1;
+insert into t1 select 2, f2, 2 from t1;
-f1	f3	check_row_ts(row_start, row_end)
+select f1, f3, check_row(row_start, row_end) from t1;
+f1	f3	check_row(row_start, row_end)
 1	1	CURRENT ROW
 1	1	CURRENT ROW
-delete from t1;
+2	2	CURRENT ROW
-select f1, f3, check_row_ts(row_start, row_end) from t1 for system_time all;
+2	2	CURRENT ROW
-f1	f3	check_row_ts(row_start, row_end)
+delete from t1 where f1 = 1;
+select f1, f3, check_row(row_start, row_end) from t1 for system_time all order by f1, row_end;
+f1	f3	check_row(row_start, row_end)
 1	1	HISTORICAL ROW
 1	1	HISTORICAL ROW
-drop table t1;
+2	2	CURRENT ROW
+2	2	CURRENT ROW
+create table t2 (f1 int);
+insert into t2 values (2);
+# Multi-delelte
+delete t1, t2 from t1 join t2 where t1.f1 = t2.f1;
+select f1, f3, check_row(row_start, row_end) from t1 for system_time all order by f1, row_end;
+f1	f3	check_row(row_start, row_end)
+1	1	HISTORICAL ROW
+1	1	HISTORICAL ROW
+2	2	HISTORICAL ROW
+2	2	HISTORICAL ROW
+# Cleanup
+drop tables t1, t2;

mysql-test/suite/versioning/r/update,trx_id.rdiff

@@ -1,6 +1,6 @@
---- update.result	2018-12-19 13:55:35.873917389 +0300
+--- update.result
-+++ update,trx_id.reject	2018-12-19 13:55:35.533917399 +0300
++++ update.reject
-@@ -81,12 +81,10 @@
+@@ -84,12 +84,10 @@
  commit;
  select x, y, sys_trx_end = MAXVAL as current from t1 for system_time all order by sys_trx_end, x, y;
  x	y	current
@@ -14,3 +14,11 @@
  1	1	1
  2	2	1
  3	3	1
+@@ -464,7 +462,6 @@
+ select nid, nstate, check_row(row_start, row_end) from t1 for system_time all order by row_start, row_end;
+ nid	nstate	check_row(row_start, row_end)
+ 1	1	HISTORICAL ROW
+-1	1	HISTORICAL ROW
+ 1	3	CURRENT ROW
+ commit;
+ drop tables t1;

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

@@ -51,19 +51,22 @@ sys_trx_start SYS_DATATYPE as row start invisible,
 sys_trx_end SYS_DATATYPE as row end invisible,
 period for system_time (sys_trx_start, sys_trx_end))
 with system versioning;
+set timestamp= unix_timestamp('2000-01-01 00:00:00');
 insert into t1 values(1, 1, 1);
-set @ins_t= now(6);
 select sys_trx_start into @tmp1 from t1;
+set timestamp= unix_timestamp('2000-01-01 01:00:00');
 update t1 set x= 11, y= 11 where id = 1;
 select @tmp1 < sys_trx_start as A1, x, y from t1;
 A1	x	y
 1	11	11
 select sys_trx_start into @tmp1 from t1;
+set timestamp= unix_timestamp('2000-01-01 02:00:00');
 update t1 set y= 1 where id = 1;
 select @tmp1 = sys_trx_start as A2, x from t1;
 A2	x
 1	11
 drop table t1;
+set timestamp= default;
 create table t1 (
 x int,
 y int,
@@ -451,4 +454,32 @@ select x, y, check_row_ts(row_start, row_end) from t1 for system_time all order
 x	y	check_row_ts(row_start, row_end)
 1	3	CURRENT ROW
 drop table t1;
+#
+# MDEV-25644 UPDATE not working properly on transaction precise system versioned table
+#
+create or replace table t1 (nid int primary key, nstate int, ntype int) engine innodb;
+alter table t1 add
+row_start SYS_DATATYPE generated always as row start invisible,
+add row_end SYS_DATATYPE generated always as row end invisible,
+add period for system_time(row_start, row_end),
+add system versioning;
+insert into t1 values (1, 1, 1);
+select nid, nstate, check_row(row_start, row_end) from t1 for system_time all order by row_start, row_end;
+nid	nstate	check_row(row_start, row_end)
+1	1	CURRENT ROW
+start transaction;
+update t1 set nstate= nstate where nid = 1;
+select nid, nstate, check_row(row_start, row_end) from t1 for system_time all order by row_start, row_end;
+nid	nstate	check_row(row_start, row_end)
+1	1	HISTORICAL ROW
+1	1	CURRENT ROW
+# Bug: ERROR 1761 (23000): Foreign key constraint for table 'xxx', record '1-18446744073709551615' would lead to a duplicate entry in table 'xxx', key 'PRIMARY'
+update t1 set nstate= 3 where nid= 1;
+select nid, nstate, check_row(row_start, row_end) from t1 for system_time all order by row_start, row_end;
+nid	nstate	check_row(row_start, row_end)
+1	1	HISTORICAL ROW
+1	1	HISTORICAL ROW
+1	3	CURRENT ROW
+commit;
+drop tables t1;
 # End of 10.4 tests

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

@@ -97,16 +97,26 @@ drop table t1;
 --echo #
 --echo # MDEV-21138 Assertion `col->ord_part' or `f.col->ord_part' failed in row_build_index_entry_low
 --echo #
-create table t1 (
+--echo # Check DELETE and multi-DELETE with foreign key
+replace_result $sys_datatype_expl SYS_TYPE;
+eval create table t1 (
   f1 int, f2 text, f3 int, fulltext (f2), key(f1), key(f3),
-  foreign key r (f3) references t1 (f1) on delete set null)
+  foreign key r (f3) references t1 (f1) on delete set null,
+  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 engine innodb;
 insert into t1 values (1, repeat('a', 8193), 1), (1, repeat('b', 8193), 1);
-select f1, f3, check_row_ts(row_start, row_end) from t1;
+insert into t1 select 2, f2, 2 from t1;
-delete from t1;
+select f1, f3, check_row(row_start, row_end) from t1;
-select f1, f3, check_row_ts(row_start, row_end) from t1 for system_time all;
+delete from t1 where f1 = 1;
-
+select f1, f3, check_row(row_start, row_end) from t1 for system_time all order by f1, row_end;
-# cleanup
+create table t2 (f1 int);
-drop table t1;
+insert into t2 values (2);
+--echo # Multi-delelte
+delete t1, t2 from t1 join t2 where t1.f1 = t2.f1;
+select f1, f3, check_row(row_start, row_end) from t1 for system_time all order by f1, row_end;
+--echo # Cleanup
+drop tables t1, t2;
 
 --source suite/versioning/common_finish.inc

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

@@ -26,15 +26,18 @@ eval create table t1 (
   sys_trx_end $sys_datatype_expl as row end invisible,
   period for system_time (sys_trx_start, sys_trx_end))
 with system versioning;
+set timestamp= unix_timestamp('2000-01-01 00:00:00');
 insert into t1 values(1, 1, 1);
-set @ins_t= now(6);
 select sys_trx_start into @tmp1 from t1;
+set timestamp= unix_timestamp('2000-01-01 01:00:00');
 update t1 set x= 11, y= 11 where id = 1;
 select @tmp1 < sys_trx_start as A1, x, y from t1;
 select sys_trx_start into @tmp1 from t1;
+set timestamp= unix_timestamp('2000-01-01 02:00:00');
 update t1 set y= 1 where id = 1;
 select @tmp1 = sys_trx_start as A2, x from t1;
 drop table t1;
+set timestamp= default;
 
 replace_result $sys_datatype_expl SYS_DATATYPE;
 eval create table t1 (
@@ -389,6 +392,28 @@ select x, y, check_row_ts(row_start, row_end) from t1 for system_time all order
 
 drop table t1;
 
+--echo #
+--echo # MDEV-25644 UPDATE not working properly on transaction precise system versioned table
+--echo #
+create or replace table t1 (nid int primary key, nstate int, ntype int) engine innodb;
+--replace_result $sys_datatype_expl SYS_DATATYPE
+eval alter table t1 add
+  row_start $sys_datatype_expl generated always as row start invisible,
+  add row_end $sys_datatype_expl generated always as row end invisible,
+  add period for system_time(row_start, row_end),
+  add system versioning;
+insert into t1 values (1, 1, 1);
+select nid, nstate, check_row(row_start, row_end) from t1 for system_time all order by row_start, row_end;
+start transaction;
+update t1 set nstate= nstate where nid = 1;
+select nid, nstate, check_row(row_start, row_end) from t1 for system_time all order by row_start, row_end;
+--echo # Bug: ERROR 1761 (23000): Foreign key constraint for table 'xxx', record '1-18446744073709551615' would lead to a duplicate entry in table 'xxx', key 'PRIMARY'
+update t1 set nstate= 3 where nid= 1;
+# Under one transaction trx_id generates only one history row, that differs from timestamp
+select nid, nstate, check_row(row_start, row_end) from t1 for system_time all order by row_start, row_end;
+commit;
+drop tables t1;
+
 --echo # End of 10.4 tests
 
 source suite/versioning/common_finish.inc;

sql/sql_update.cc

@@ -232,6 +232,11 @@ bool TABLE::vers_check_update(List<Item> &items)
       }
     }
   }
+  /*
+    Tell TRX_ID-versioning that it does not insert history row
+    (see calc_row_difference()).
+  */
+  vers_write= false;
   return false;
 }
 

storage/innobase/handler/ha_innodb.cc

@@ -8410,6 +8410,10 @@ calc_row_difference(
 	trx_t* const	trx = prebuilt->trx;
 	doc_id_t	doc_id = FTS_NULL_DOC_ID;
 	ulint		num_v = 0;
+#ifndef DBUG_OFF
+	uint		vers_fields = 0;
+#endif
+	prebuilt->versioned_write = table->versioned_write(VERS_TRX_ID);
 	const bool skip_virtual = ha_innobase::omits_virtual_cols(*table->s);
 
 	ut_ad(!srv_read_only_mode);
@@ -8422,6 +8426,14 @@ calc_row_difference(
 
 	for (uint i = 0; i < table->s->fields; i++) {
 		field = table->field[i];
+
+#ifndef DBUG_OFF
+		if (!field->vers_sys_field()
+		    && !field->vers_update_unversioned()) {
+			++vers_fields;
+		}
+#endif
+
 		const bool is_virtual = !field->stored_in_db();
 		if (is_virtual && skip_virtual) {
 			num_v++;
@@ -8759,6 +8771,21 @@ calc_row_difference(
 
 	ut_a(buf <= (byte*) original_upd_buff + buff_len);
 
+	const TABLE_LIST *tl= table->pos_in_table_list;
+	const uint8 op_map= tl->trg_event_map | tl->slave_fk_event_map;
+	/* Used to avoid reading history in FK check on DELETE (see MDEV-16210). */
+	prebuilt->upd_node->is_delete =
+		(op_map & trg2bit(TRG_EVENT_DELETE)
+		 && table->versioned(VERS_TIMESTAMP))
+		? VERSIONED_DELETE : NO_DELETE;
+
+	if (prebuilt->versioned_write) {
+		/* Guaranteed by CREATE TABLE, but anyway we make sure we
+		generate history only when there are versioned fields. */
+		DBUG_ASSERT(vers_fields);
+		prebuilt->upd_node->vers_make_update(trx);
+	}
+
 	ut_ad(uvect->validate());
 	return(DB_SUCCESS);
 }
@@ -8913,48 +8940,24 @@ ha_innobase::update_row(
 		MySQL that the row is not really updated and it
 		should not increase the count of updated rows.
 		This is fix for http://bugs.mysql.com/29157 */
-		if (m_prebuilt->versioned_write
-		    && thd_sql_command(m_user_thd) != SQLCOM_ALTER_TABLE
-		    /* Multiple UPDATE of same rows in single transaction create
-		       historical rows only once. */
-		    && trx->id != table->vers_start_id()) {
-			error = row_insert_for_mysql((byte*) old_row,
-						     m_prebuilt,
-						     ROW_INS_HISTORICAL);
-			if (error != DB_SUCCESS) {
-				goto func_exit;
-			}
-			innobase_srv_conc_exit_innodb(m_prebuilt);
-			innobase_active_small();
-		}
 		DBUG_RETURN(HA_ERR_RECORD_IS_THE_SAME);
 	} else {
-		const bool vers_set_fields = m_prebuilt->versioned_write
-			&& m_prebuilt->upd_node->update->affects_versioned();
-		const bool vers_ins_row = vers_set_fields
-			&& thd_sql_command(m_user_thd) != SQLCOM_ALTER_TABLE;
-
-                TABLE_LIST *tl= table->pos_in_table_list;
-                uint8 op_map= tl->trg_event_map | tl->slave_fk_event_map;
-		/* This is not a delete */
-		m_prebuilt->upd_node->is_delete =
-			(vers_set_fields && !vers_ins_row) ||
-			(op_map & trg2bit(TRG_EVENT_DELETE) &&
-				table->versioned(VERS_TIMESTAMP))
-			? VERSIONED_DELETE
-			: NO_DELETE;
-
 		innobase_srv_conc_enter_innodb(m_prebuilt);
 
 		if (m_prebuilt->upd_node->is_delete) {
 			trx->fts_next_doc_id = 0;
 		}
+		/* row_start was updated by vers_make_update()
+		in calc_row_difference() */
 		error = row_update_for_mysql(m_prebuilt);
 
-		if (error == DB_SUCCESS && vers_ins_row
+		if (error == DB_SUCCESS && m_prebuilt->versioned_write
 		    /* Multiple UPDATE of same rows in single transaction create
 		       historical rows only once. */
 		    && trx->id != table->vers_start_id()) {
+			/* UPDATE is not used by ALTER TABLE. Just precaution
+			as we don't need history generation for ALTER TABLE. */
+			ut_ad(thd_sql_command(m_user_thd) != SQLCOM_ALTER_TABLE);
 			error = row_insert_for_mysql((byte*) old_row,
 						     m_prebuilt,
 						     ROW_INS_HISTORICAL);

storage/innobase/row/row0ins.cc

@@ -2439,7 +2439,10 @@ row_ins_duplicate_error_in_clust(
 duplicate:
 				trx->error_info = cursor->index;
 				err = DB_DUPLICATE_KEY;
-				if (cursor->index->table->versioned()
+				if (thr->prebuilt
+				    && thr->prebuilt->upd_node
+				    && thr->prebuilt->upd_node->is_delete
+					== VERSIONED_DELETE
 				    && entry->vers_history_row())
 				{
 					ulint trx_id_len;

storage/innobase/row/row0mysql.cc

@@ -1790,12 +1790,8 @@ row_update_for_mysql(row_prebuilt_t* prebuilt)
 
 	ut_ad(!prebuilt->versioned_write || node->table->versioned());
 
-	if (prebuilt->versioned_write) {
+	if (prebuilt->versioned_write && node->is_delete == VERSIONED_DELETE) {
-		if (node->is_delete == VERSIONED_DELETE) {
+		node->vers_make_delete(trx);
-                  node->vers_make_delete(trx);
-                } else if (node->update->affects_versioned()) {
-                  node->vers_make_update(trx);
-                }
 	}
 
 	for (;;) {

storage/innobase/row/row0upd.cc

@@ -3505,9 +3505,8 @@ error_handling:
 void thd_get_query_start_data(THD *thd, char *buf);
 
 /** Appends row_start or row_end field to update vector and sets a
-CURRENT_TIMESTAMP/trx->id value to it.
+CURRENT_TIMESTAMP/trx->id value to it. Called by vers_make_update() and
-Supposed to be called only by make_versioned_update() and
+vers_make_delete().
-make_versioned_delete().
 @param[in]	trx	transaction
 @param[in]	vers_sys_idx	table->row_start or table->row_end */
 void upd_node_t::vers_update_fields(const trx_t *trx, ulint idx)