From 14cc7e7d6eed0cb3552f8bb28dc8a7a3edf2d89e Mon Sep 17 00:00:00 2001 From: Aleksey Midenkov Date: Thu, 20 Jul 2023 14:14:00 +0300 Subject: [PATCH] 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 | 34 +++++++--- .../suite/versioning/r/update,trx_id.rdiff | 14 ++++- mysql-test/suite/versioning/r/update.result | 33 +++++++++- mysql-test/suite/versioning/t/delete.test | 26 +++++--- mysql-test/suite/versioning/t/update.test | 27 +++++++- sql/sql_update.cc | 5 ++ storage/innobase/handler/ha_innodb.cc | 63 ++++++++++--------- storage/innobase/row/row0ins.cc | 5 +- storage/innobase/row/row0mysql.cc | 8 +-- storage/innobase/row/row0upd.cc | 5 +- 10 files changed, 160 insertions(+), 60 deletions(-) diff --git a/mysql-test/suite/versioning/r/delete.result b/mysql-test/suite/versioning/r/delete.result index 6f8c8921790..89c1ef516eb 100644 --- a/mysql-test/suite/versioning/r/delete.result +++ b/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; -f1 f3 check_row_ts(row_start, row_end) +insert into t1 select 2, f2, 2 from t1; +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; -select f1, f3, check_row_ts(row_start, row_end) from t1 for system_time all; -f1 f3 check_row_ts(row_start, row_end) +2 2 CURRENT ROW +2 2 CURRENT ROW +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; diff --git a/mysql-test/suite/versioning/r/update,trx_id.rdiff b/mysql-test/suite/versioning/r/update,trx_id.rdiff index 7ce75714235..18395507b71 100644 --- a/mysql-test/suite/versioning/r/update,trx_id.rdiff +++ b/mysql-test/suite/versioning/r/update,trx_id.rdiff @@ -1,6 +1,6 @@ ---- update.result 2018-12-19 13:55:35.873917389 +0300 -+++ update,trx_id.reject 2018-12-19 13:55:35.533917399 +0300 -@@ -81,12 +81,10 @@ +--- update.result ++++ update.reject +@@ -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; diff --git a/mysql-test/suite/versioning/r/update.result b/mysql-test/suite/versioning/r/update.result index f0a5052acf9..df463de0f91 100644 --- a/mysql-test/suite/versioning/r/update.result +++ b/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 diff --git a/mysql-test/suite/versioning/t/delete.test b/mysql-test/suite/versioning/t/delete.test index a5a0497fef2..9debdcfec8c 100644 --- a/mysql-test/suite/versioning/t/delete.test +++ b/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; -delete from t1; -select f1, f3, check_row_ts(row_start, row_end) from t1 for system_time all; - -# cleanup -drop table t1; +insert into t1 select 2, f2, 2 from t1; +select f1, f3, check_row(row_start, row_end) from t1; +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; +create table t2 (f1 int); +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 diff --git a/mysql-test/suite/versioning/t/update.test b/mysql-test/suite/versioning/t/update.test index b86296616fd..046a4ac149d 100644 --- a/mysql-test/suite/versioning/t/update.test +++ b/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; diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 834fa6111e5..233ad49576e 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -232,6 +232,11 @@ bool TABLE::vers_check_update(List &items) } } } + /* + Tell TRX_ID-versioning that it does not insert history row + (see calc_row_difference()). + */ + vers_write= false; return false; } diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 3a787da3fa8..3a8d562f705 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/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); diff --git a/storage/innobase/row/row0ins.cc b/storage/innobase/row/row0ins.cc index 81f662e7a15..6765ba546d5 100644 --- a/storage/innobase/row/row0ins.cc +++ b/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; diff --git a/storage/innobase/row/row0mysql.cc b/storage/innobase/row/row0mysql.cc index bb994a926eb..dc3d0ca2ade 100644 --- a/storage/innobase/row/row0mysql.cc +++ b/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 (node->is_delete == VERSIONED_DELETE) { - node->vers_make_delete(trx); - } else if (node->update->affects_versioned()) { - node->vers_make_update(trx); - } + if (prebuilt->versioned_write && node->is_delete == VERSIONED_DELETE) { + node->vers_make_delete(trx); } for (;;) { diff --git a/storage/innobase/row/row0upd.cc b/storage/innobase/row/row0upd.cc index 0ea7aea7fb1..0700792e441 100644 --- a/storage/innobase/row/row0upd.cc +++ b/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. -Supposed to be called only by make_versioned_update() and -make_versioned_delete(). +CURRENT_TIMESTAMP/trx->id value to it. Called by vers_make_update() and +vers_make_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)