From 17ab02f4b0537de321a281f47dec825a6368d483 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Mon, 2 Sep 2019 10:53:46 +0200 Subject: [PATCH 01/14] cleanup: on update default now * remove one level of virtual functions * remove redundant checks * remove an if() as the value is always known at compilation time don't pretend that "DEFAULT expr" and "ON UPDATE DEFAULT NOW" are "basically the same thing" --- sql/field.h | 22 ---------------------- sql/sql_base.cc | 4 ++-- sql/sql_insert.cc | 7 ++----- sql/sql_table.cc | 4 ++-- sql/sql_update.cc | 16 ++++++---------- sql/table.cc | 28 +++++++++++++++++++--------- sql/table.h | 3 ++- 7 files changed, 33 insertions(+), 51 deletions(-) diff --git a/sql/field.h b/sql/field.h index c82febd956e..73c36affce0 100644 --- a/sql/field.h +++ b/sql/field.h @@ -992,14 +992,6 @@ public: } bool set_explicit_default(Item *value); - /** - Evaluates the @c UPDATE default function, if one exists, and stores the - result in the record buffer. If no such function exists for the column, - or the function is not valid for the column's data type, invoking this - function has no effect. - */ - virtual int evaluate_update_default_function() { return 0; } - virtual bool binary() const { return 1; } virtual bool zero_pack() const { return 1; } virtual enum ha_base_keytype key_type() const { return HA_KEYTYPE_BINARY; } @@ -2519,13 +2511,6 @@ public: void sql_type(String &str) const; bool zero_pack() const { return 0; } int set_time(); - int evaluate_update_default_function() - { - int res= 0; - if (has_update_default_function()) - res= set_time(); - return res; - } /* Get TIMESTAMP field value as seconds since begging of Unix Epoch */ virtual my_time_t get_timestamp(const uchar *pos, ulong *sec_part) const; my_time_t get_timestamp(ulong *sec_part) const @@ -2954,13 +2939,6 @@ public: bool get_date(MYSQL_TIME *ltime, ulonglong fuzzydate) { return Field_datetime::get_TIME(ltime, ptr, fuzzydate); } int set_time(); - int evaluate_update_default_function() - { - int res= 0; - if (has_update_default_function()) - res= set_time(); - return res; - } uchar *pack(uchar* to, const uchar *from, uint max_length __attribute__((unused))) { diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 662ba3f6b30..14740274b87 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8072,7 +8072,7 @@ fill_record(THD *thd, TABLE *table_arg, List &fields, List &values, } if (!update && table_arg->default_field && - table_arg->update_default_fields(0, ignore_errors)) + table_arg->update_default_fields(ignore_errors)) goto err; /* Update virtual fields */ if (table_arg->vfield && @@ -8317,7 +8317,7 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List &values, all_fields_have_values &= field->set_explicit_default(value); } if (!all_fields_have_values && table->default_field && - table->update_default_fields(0, ignore_errors)) + table->update_default_fields(ignore_errors)) goto err; /* Update virtual fields */ thd->abort_on_warning= FALSE; diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 78564f28c31..550851bd215 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -1796,10 +1796,7 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) be updated as if this is an UPDATE. */ if (different_records && table->default_field) - { - if (table->update_default_fields(1, info->ignore)) - goto err; - } + table->evaluate_update_default_function(); /* CHECK OPTION for VIEW ... ON DUPLICATE KEY UPDATE ... */ res= info->table_list->view_check_option(table->in_use, info->ignore); @@ -3762,7 +3759,7 @@ int select_insert::send_data(List &values) thd->count_cuted_fields= CHECK_FIELD_WARN; // Calculate cuted fields store_values(values); - if (table->default_field && table->update_default_fields(0, info.ignore)) + if (table->default_field && table->update_default_fields(info.ignore)) DBUG_RETURN(1); thd->count_cuted_fields= CHECK_FIELD_ERROR_FOR_NULL; if (thd->is_error()) diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 33589b3044d..761e14e31d0 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -9455,7 +9455,7 @@ do_continue:; /* Check that we can call default functions with default field values */ altered_table->reset_default_fields(); if (altered_table->default_field && - altered_table->update_default_fields(0, 1)) + altered_table->update_default_fields(true)) goto err_new_table_cleanup; // Ask storage engine whether to use copy or in-place @@ -10138,7 +10138,7 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *to, } prev_insert_id= to->file->next_insert_id; if (to->default_field) - to->update_default_fields(0, ignore); + to->update_default_fields(ignore); if (to->vfield) to->update_virtual_fields(to->file, VCOL_UPDATE_FOR_WRITE); diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 1d1c38f59d6..4f129eb995a 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -761,11 +761,8 @@ int mysql_update(THD *thd, if (!can_compare_record || compare_record(table)) { - if (table->default_field && table->update_default_fields(1, ignore)) - { - error= 1; - break; - } + if (table->default_field) + table->evaluate_update_default_function(); if ((res= table_list->view_check_option(thd, ignore)) != VIEW_CHECK_OK) { @@ -2156,8 +2153,8 @@ int multi_update::send_data(List ¬_used_values) { int error; - if (table->default_field && table->update_default_fields(1, ignore)) - DBUG_RETURN(1); + if (table->default_field) + table->evaluate_update_default_function(); if ((error= cur_table->view_check_option(thd, ignore)) != VIEW_CHECK_OK) @@ -2482,9 +2479,8 @@ int multi_update::do_updates() if (!can_compare_record || compare_record(table)) { int error; - if (table->default_field && - (error= table->update_default_fields(1, ignore))) - goto err2; + if (table->default_field) + table->evaluate_update_default_function(); if (table->vfield && table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_WRITE)) goto err2; diff --git a/sql/table.cc b/sql/table.cc index 07858ab270a..e39b73b2f4b 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -7736,7 +7736,7 @@ int TABLE::update_virtual_field(Field *vf) ignore_errors == 0. If set then an error was generated. */ -int TABLE::update_default_fields(bool update_command, bool ignore_errors) +int TABLE::update_default_fields(bool ignore_errors) { Query_arena backup_arena; Field **field_ptr; @@ -7756,14 +7756,9 @@ int TABLE::update_default_fields(bool update_command, bool ignore_errors) */ if (!field->has_explicit_value()) { - if (!update_command) - { - if (field->default_value && - (field->default_value->flags || field->flags & BLOB_FLAG)) - res|= (field->default_value->expr->save_in_field(field, 0) < 0); - } - else - res|= field->evaluate_update_default_function(); + if (field->default_value && + (field->default_value->flags || field->flags & BLOB_FLAG)) + res|= (field->default_value->expr->save_in_field(field, 0) < 0); if (!ignore_errors && res) { my_error(ER_CALCULATING_DEFAULT_VALUE, MYF(0), field->field_name); @@ -7776,6 +7771,21 @@ int TABLE::update_default_fields(bool update_command, bool ignore_errors) DBUG_RETURN(res); } +void TABLE::evaluate_update_default_function() +{ + DBUG_ENTER("TABLE::evaluate_update_default_function"); + + if (s->has_update_default_function) + for (Field **field_ptr= default_field; *field_ptr ; field_ptr++) + { + Field *field= (*field_ptr); + if (!field->has_explicit_value() && field->has_update_default_function()) + field->set_time(); + } + DBUG_VOID_RETURN; +} + + /** Reset markers that fields are being updated */ diff --git a/sql/table.h b/sql/table.h index 8f6a9e7aa86..f10e00562dc 100644 --- a/sql/table.h +++ b/sql/table.h @@ -1453,7 +1453,8 @@ public: ulong actual_key_flags(KEY *keyinfo); int update_virtual_field(Field *vf); int update_virtual_fields(handler *h, enum_vcol_update_mode update_mode); - int update_default_fields(bool update, bool ignore_errors); + int update_default_fields(bool ignore_errors); + void evaluate_update_default_function(); void reset_default_fields(); inline ha_rows stat_records() { return used_stat_records; } From 3789692d17625780b546bf2ec4a33acf5badae2c Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Mon, 2 Sep 2019 14:14:57 +0200 Subject: [PATCH 02/14] don't compare unassigned columns on UPDATE, compare_record() was comparing all columns that are marked for writing. But generated columns that are written to the table are always deterministic and cannot change unless normal non-generated columns were changed. So it's enough to compare only non-generated columns that were explicitly assigned values in the SET clause. --- sql/sql_update.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 4f129eb995a..0d55fa58bde 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -76,7 +76,7 @@ bool compare_record(const TABLE *table) for (Field **ptr= table->field ; *ptr != NULL; ptr++) { Field *field= *ptr; - if (bitmap_is_set(table->write_set, field->field_index)) + if (field->has_explicit_value() && !field->vcol_info) { if (field->real_maybe_null()) { @@ -108,8 +108,9 @@ bool compare_record(const TABLE *table) /* Compare updated fields */ for (Field **ptr= table->field ; *ptr ; ptr++) { - if (bitmap_is_set(table->write_set, (*ptr)->field_index) && - (*ptr)->cmp_binary_offset(table->s->rec_buff_length)) + Field *field= *ptr; + if (field->has_explicit_value() && !field->vcol_info && + field->cmp_binary_offset(table->s->rec_buff_length)) return TRUE; } return FALSE; From c7c481f4d918aaf42cef083b77ab551d69cdae58 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Mon, 2 Sep 2019 14:10:20 +0200 Subject: [PATCH 03/14] MDEV-20403 Assertion `0' or Assertion `btr_validate_index(index, 0)' failed in row_upd_sec_index_entry or error code 126: Index is corrupted upon UPDATE with TIMESTAMP..ON UPDATE Three issues here: * ON UPDATE DEFAULT NOW columns were updated after generated columns were computed - this broke indexed virtual columns * ON UPDATE DEFAULT NOW columns were updated after BEFORE triggers, so triggers didn't see the correct NEW value * in case of a multi-update generated columns were also updated after BEFORE triggers --- mysql-test/r/function_defaults.result | 23 +++++++++++++++++++++++ mysql-test/t/function_defaults.test | 27 +++++++++++++++++++++++++++ sql/sql_base.cc | 10 +++++++--- sql/sql_update.cc | 19 ++++++++----------- 4 files changed, 65 insertions(+), 14 deletions(-) diff --git a/mysql-test/r/function_defaults.result b/mysql-test/r/function_defaults.result index 62422752e17..1c63ae17a5d 100644 --- a/mysql-test/r/function_defaults.result +++ b/mysql-test/r/function_defaults.result @@ -3093,3 +3093,26 @@ a b 1999-12-01 11:22:33.000000 1999-12-01 11:22:33.000000 2001-09-09 04:46:40.000000 2001-09-09 04:46:40.000000 DROP TABLE t1; +create table t1 (t timestamp, i int, v timestamp as (t) virtual, key(v)); +insert t1 (t,i) values ('2006-03-01 23:59:59',1); +update t1 set i = 2; +check table t1; +Table Op Msg_type Msg_text +test.t1 check status OK +drop table t1; +create table t1 (t timestamp, i int); +create trigger tr1 before update on t1 for each row set @new:=new.t; +insert t1 (t,i) values ('2006-03-01 23:59:59', 1); +update t1 set i = 2; +select if(@new = t, 'correct', 'wrong') from t1; +if(@new = t, 'correct', 'wrong') +correct +drop table t1; +create table t1 (i int, j int as (i)); +create trigger tr1 before update on t1 for each row set @new:=new.j; +insert t1 (i) values (1); +update t1, t1 as t2 set t1.i = 2; +select if(@new = j, 'correct', 'wrong') from t1; +if(@new = j, 'correct', 'wrong') +correct +drop table t1; diff --git a/mysql-test/t/function_defaults.test b/mysql-test/t/function_defaults.test index f8b23d0eda8..1e3e86599e3 100644 --- a/mysql-test/t/function_defaults.test +++ b/mysql-test/t/function_defaults.test @@ -19,3 +19,30 @@ let $now=NOW(6); let $timestamp=TIMESTAMP(6); let $datetime=DATETIME(6); source 'include/function_defaults.inc'; + +# +# MDEV-20403 Assertion `0' or Assertion `btr_validate_index(index, 0)' failed in row_upd_sec_index_entry or error code 126: Index is corrupted upon UPDATE with TIMESTAMP..ON UPDATE +# + +# ON UPDATE DEFAULT NOW and indexed virtual columns +create table t1 (t timestamp, i int, v timestamp as (t) virtual, key(v)); +insert t1 (t,i) values ('2006-03-01 23:59:59',1); +update t1 set i = 2; +check table t1; +drop table t1; + +# ON UPDATE DEFAULT NOW and triggers +create table t1 (t timestamp, i int); +create trigger tr1 before update on t1 for each row set @new:=new.t; +insert t1 (t,i) values ('2006-03-01 23:59:59', 1); +update t1 set i = 2; +select if(@new = t, 'correct', 'wrong') from t1; +drop table t1; + +# triggers, virtual columns, multi-update +create table t1 (i int, j int as (i)); +create trigger tr1 before update on t1 for each row set @new:=new.j; +insert t1 (i) values (1); +update t1, t1 as t2 set t1.i = 2; +select if(@new = j, 'correct', 'wrong') from t1; +drop table t1; diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 14740274b87..f7f7d50bc85 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8071,9 +8071,13 @@ fill_record(THD *thd, TABLE *table_arg, List &fields, List &values, rfield->set_explicit_default(value); } - if (!update && table_arg->default_field && - table_arg->update_default_fields(ignore_errors)) - goto err; + if (update) + table_arg->evaluate_update_default_function(); + else + if (table_arg->default_field && + table_arg->update_default_fields(ignore_errors)) + goto err; + /* Update virtual fields */ if (table_arg->vfield && table_arg->update_virtual_fields(table_arg->file, VCOL_UPDATE_FOR_WRITE)) diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 0d55fa58bde..65a828147ae 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -66,12 +66,15 @@ bool compare_record(const TABLE *table) { DBUG_ASSERT(records_are_comparable(table)); - if ((table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ) != 0) + if (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ || + table->s->has_update_default_function) { /* Storage engine may not have read all columns of the record. Fields (including NULL bits) not in the write_set may not have been read and can therefore not be compared. + Or ON UPDATE DEFAULT NOW() could've changed field values, including + NULL bits. */ for (Field **ptr= table->field ; *ptr != NULL; ptr++) { @@ -762,8 +765,6 @@ int mysql_update(THD *thd, if (!can_compare_record || compare_record(table)) { - if (table->default_field) - table->evaluate_update_default_function(); if ((res= table_list->view_check_option(thd, ignore)) != VIEW_CHECK_OK) { @@ -2154,9 +2155,6 @@ int multi_update::send_data(List ¬_used_values) { int error; - if (table->default_field) - table->evaluate_update_default_function(); - if ((error= cur_table->view_check_option(thd, ignore)) != VIEW_CHECK_OK) { @@ -2472,6 +2470,10 @@ int multi_update::do_updates() copy_field_ptr->to_field->set_has_explicit_value(); } + table->evaluate_update_default_function(); + if (table->vfield && + table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_WRITE)) + goto err2; if (table->triggers && table->triggers->process_triggers(thd, TRG_EVENT_UPDATE, TRG_ACTION_BEFORE, TRUE)) @@ -2480,11 +2482,6 @@ int multi_update::do_updates() if (!can_compare_record || compare_record(table)) { int error; - if (table->default_field) - table->evaluate_update_default_function(); - if (table->vfield && - table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_WRITE)) - goto err2; if ((error= cur_table->view_check_option(thd, ignore)) != VIEW_CHECK_OK) { From 6ee7a9a43876e6ffd2cad8e12bdd455f252a976a Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Tue, 3 Sep 2019 11:41:35 +0200 Subject: [PATCH 04/14] C/C --- libmariadb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libmariadb b/libmariadb index 7de639518ff..544b6f1d12f 160000 --- a/libmariadb +++ b/libmariadb @@ -1 +1 @@ -Subproject commit 7de639518ffe56a99ac805654381d17f42796be2 +Subproject commit 544b6f1d12f0e5b2a141129075ff2d64feb0e4c9 From cbb85f0d214a47a5e56c963a15cf70455425f39f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Lindstr=C3=B6m?= Date: Wed, 4 Sep 2019 08:36:52 +0300 Subject: [PATCH 05/14] Disable galera.galera_var_node_address test case. --- mysql-test/suite/galera/disabled.def | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mysql-test/suite/galera/disabled.def b/mysql-test/suite/galera/disabled.def index 017d8ec1380..c0dd58f85b9 100644 --- a/mysql-test/suite/galera/disabled.def +++ b/mysql-test/suite/galera/disabled.def @@ -28,6 +28,7 @@ galera_ist_progress : MDEV-15236 fails when trying to read transfer status galera_migrate : MariaDB does not support START SLAVE USER galera_ssl_upgrade : MDEV-19950 Galera test failure on galera_ssl_upgrade galera_sst_mariabackup_encrypt_with_key : MDEV-19926 Galera SST tests fail -galera_wan : MDEV-17259: Test failure on galera.galera_wan +galera_var_node_address : MDEV-20485 Galera test failure on galera.galera_var_node_address +galera_wan : MDEV-17259 Test failure on galera.galera_wan partition : MDEV-19958 Galera test failure on galera.partition query_cache: MDEV-15805 Test failure on galera.query_cache \ No newline at end of file From b2775ae85578c0724256dd7cc6794d8b1fc36944 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 2 Sep 2019 12:31:29 +0300 Subject: [PATCH 06/14] MVCC::view_close(): Correct comments --- storage/innobase/include/read0read.h | 7 ++++--- storage/innobase/read/read0read.cc | 8 +++----- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/storage/innobase/include/read0read.h b/storage/innobase/include/read0read.h index 4fe1341c36c..359db1d8c39 100644 --- a/storage/innobase/include/read0read.h +++ b/storage/innobase/include/read0read.h @@ -1,6 +1,7 @@ /***************************************************************************** Copyright (c) 1997, 2013, Oracle and/or its affiliates. All Rights Reserved. +Copyright (c) 2019, MariaDB Corporation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software @@ -49,9 +50,9 @@ public: void view_open(ReadView*& view, trx_t* trx); /** - Close a view created by the above function. - @para view view allocated by trx_open. - @param own_mutex true if caller owns trx_sys_t::mutex */ + Close a view created by view_open(). + @param view view allocated by view_open() + @param own_mutex whether the caller owns trx_sys_t::mutex */ void view_close(ReadView*& view, bool own_mutex); /** diff --git a/storage/innobase/read/read0read.cc b/storage/innobase/read/read0read.cc index 298d2fe4c43..3fd52d5d6dd 100644 --- a/storage/innobase/read/read0read.cc +++ b/storage/innobase/read/read0read.cc @@ -740,11 +740,9 @@ MVCC::size() const /** Close a view created by the above function. -@para view view allocated by trx_open. -@param own_mutex true if caller owns trx_sys_t::mutex */ - -void -MVCC::view_close(ReadView*& view, bool own_mutex) +@param view view allocated by view_open() +@param own_mutex whether the caller owns trx_sys_t::mutex */ +void MVCC::view_close(ReadView*& view, bool own_mutex) { uintptr_t p = reinterpret_cast(view); From 154bd0950f6691dff22a5e01d4aa68e281ea6092 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Tue, 3 Sep 2019 11:23:57 +0300 Subject: [PATCH 07/14] MDEV-15326 preparation: Test slow shutdown after XA PREPARE We were missing a test that would exercise trx_free_prepared() with innodb_fast_shutdown=0. Add a test. Note: if shutdown hangs due to the XA PREPARE transactions, in MariaDB 10.2 the test would unfortunately pass, but take 2*60 seconds longer, because of two shutdown_server statements timing out after 60 seconds. Starting with MariaDB 10.3, the hung server would be killed with SIGABRT, and the test could fail thanks to a backtrace message. --- .../suite/innodb/r/recovery_shutdown.result | 9 ++++++++- .../suite/innodb/t/recovery_shutdown.test | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/mysql-test/suite/innodb/r/recovery_shutdown.result b/mysql-test/suite/innodb/r/recovery_shutdown.result index 694fd4d9194..d2fd130add8 100644 --- a/mysql-test/suite/innodb/r/recovery_shutdown.result +++ b/mysql-test/suite/innodb/r/recovery_shutdown.result @@ -1,3 +1,5 @@ +FLUSH TABLES; +call mtr.add_suppression("Found 1 prepared XA transactions"); # # MDEV-13797 InnoDB may hang if shutdown is initiated soon after startup # while rolling back recovered incomplete transactions @@ -7,10 +9,12 @@ BEGIN; COMMIT; connect con$c,localhost,root,,; CREATE TABLE t8 (a SERIAL, b INT UNIQUE, c INT UNIQUE) ENGINE=InnoDB; -BEGIN; +XA START 'x'; INSERT INTO t8 (a) SELECT NULL FROM t; UPDATE t8 SET a=a+100, b=a; DELETE FROM t8; +XA END 'x'; +XA PREPARE 'x'; connect con$c,localhost,root,,; CREATE TABLE t7 (a SERIAL, b INT UNIQUE, c INT UNIQUE) ENGINE=InnoDB; BEGIN; @@ -62,4 +66,7 @@ connection default; SET GLOBAL innodb_flush_log_at_trx_commit=1; CREATE TABLE u(a SERIAL) ENGINE=INNODB; FLUSH TABLES; +XA RECOVER; +formatID gtrid_length bqual_length data +1 1 0 x DROP TABLE t,u; diff --git a/mysql-test/suite/innodb/t/recovery_shutdown.test b/mysql-test/suite/innodb/t/recovery_shutdown.test index 50fe4e87c9d..d72e785f21d 100644 --- a/mysql-test/suite/innodb/t/recovery_shutdown.test +++ b/mysql-test/suite/innodb/t/recovery_shutdown.test @@ -1,6 +1,10 @@ --source include/have_innodb.inc --source include/not_embedded.inc +# Flush any open myisam tables from previous tests +FLUSH TABLES; +call mtr.add_suppression("Found 1 prepared XA transactions"); + --echo # --echo # MDEV-13797 InnoDB may hang if shutdown is initiated soon after startup --echo # while rolling back recovered incomplete transactions @@ -20,6 +24,15 @@ dec $c; COMMIT; let $c = $trx; +connect (con$c,localhost,root,,); +eval CREATE TABLE t$c (a SERIAL, b INT UNIQUE, c INT UNIQUE) ENGINE=InnoDB; +XA START 'x'; +eval INSERT INTO t$c (a) SELECT NULL FROM t; +eval UPDATE t$c SET a=a+$size, b=a; +eval DELETE FROM t$c; +XA END 'x'; +XA PREPARE 'x'; +dec $c; while ($c) { connect (con$c,localhost,root,,); @@ -50,12 +63,17 @@ FLUSH TABLES; # Perform a slow shutdown in order to roll back all recovered transactions # and to avoid locking conflicts with the DROP TABLE below. +XA RECOVER; --disable_query_log SET GLOBAL innodb_fast_shutdown=0; --source include/restart_mysqld.inc --disable_query_log let $c = $trx; +disconnect con$c; +XA ROLLBACK 'x'; +eval DROP TABLE t$c; +dec $c; while ($c) { disconnect con$c; From 7c79c127842133d1f85ac2273a229d84007075c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 2 Sep 2019 14:00:53 +0300 Subject: [PATCH 08/14] MDEV-15326 preparation: Remove trx_sys_t::n_prepared_trx This is a backport of 900b07908bf9dbd2c79c3a66fc471e6be4cf0d13 from MariaDB Server 10.3. --- storage/innobase/include/trx0sys.h | 12 --------- storage/innobase/lock/lock0lock.cc | 26 +++++-------------- storage/innobase/trx/trx0roll.cc | 2 -- storage/innobase/trx/trx0sys.cc | 40 ++++++++++++++++++++++-------- storage/innobase/trx/trx0trx.cc | 26 +++++++++---------- 5 files changed, 47 insertions(+), 59 deletions(-) diff --git a/storage/innobase/include/trx0sys.h b/storage/innobase/include/trx0sys.h index 1f24dabe3ac..37ac8975783 100644 --- a/storage/innobase/include/trx0sys.h +++ b/storage/innobase/include/trx0sys.h @@ -635,18 +635,6 @@ struct trx_sys_t { TrxIdSet rw_trx_set; /*!< Mapping from transaction id to transaction instance */ - - ulint n_prepared_trx; /*!< Number of transactions currently - in the XA PREPARED state */ - - ulint n_prepared_recovered_trx; /*!< Number of transactions - currently in XA PREPARED state that are - also recovered. Such transactions cannot - be added during runtime. They can only - occur after recovery if mysqld crashed - while there were XA PREPARED - transactions. We disable query cache - if such transactions exist. */ }; /** When a trx id which is zero modulo this number (which must be a power of diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index 8cb9f19d93d..8634f7e73c5 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -6552,26 +6552,12 @@ lock_trx_release_locks( { check_trx_state(trx); - if (trx_state_eq(trx, TRX_STATE_PREPARED) - || trx_state_eq(trx, TRX_STATE_PREPARED_RECOVERED)) { - - mutex_enter(&trx_sys->mutex); - - ut_a(trx_sys->n_prepared_trx > 0); - --trx_sys->n_prepared_trx; - - if (trx->is_recovered) { - ut_a(trx_sys->n_prepared_recovered_trx > 0); - trx_sys->n_prepared_recovered_trx--; - } - - mutex_exit(&trx_sys->mutex); - } else { - ut_ad(trx_state_eq(trx, TRX_STATE_ACTIVE) - || (trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY) - && trx->is_recovered - && !UT_LIST_GET_LEN(trx->lock.trx_locks))); - } + ut_ad(trx_state_eq(trx, TRX_STATE_ACTIVE) + || trx_state_eq(trx, TRX_STATE_PREPARED) + || trx_state_eq(trx, TRX_STATE_PREPARED_RECOVERED) + || (trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY) + && trx->is_recovered + && !UT_LIST_GET_LEN(trx->lock.trx_locks))); bool release_lock; diff --git a/storage/innobase/trx/trx0roll.cc b/storage/innobase/trx/trx0roll.cc index f1a19d91d0b..e827cda46eb 100644 --- a/storage/innobase/trx/trx0roll.cc +++ b/storage/innobase/trx/trx0roll.cc @@ -722,8 +722,6 @@ func_exit: && !srv_undo_sources && srv_fast_shutdown) { fake_prepared: trx->state = TRX_STATE_PREPARED; - trx_sys->n_prepared_trx++; - trx_sys->n_prepared_recovered_trx++; *all = FALSE; goto func_exit; } diff --git a/storage/innobase/trx/trx0sys.cc b/storage/innobase/trx/trx0sys.cc index 6529de31fb5..415afc4a90b 100644 --- a/storage/innobase/trx/trx0sys.cc +++ b/storage/innobase/trx/trx0sys.cc @@ -924,13 +924,7 @@ trx_sys_close(void) } /* Only prepared transactions may be left in the system. Free them. */ - ut_a(UT_LIST_GET_LEN(trx_sys->rw_trx_list) == trx_sys->n_prepared_trx - || !srv_was_started - || srv_read_only_mode - || srv_force_recovery >= SRV_FORCE_NO_TRX_UNDO); - while (trx_t* trx = UT_LIST_GET_FIRST(trx_sys->rw_trx_list)) { - UT_LIST_REMOVE(trx_sys->rw_trx_list, trx); trx_free_prepared(trx); } @@ -976,17 +970,41 @@ trx_sys_any_active_transactions(void) trx_sys_mutex_enter(); - total_trx = UT_LIST_GET_LEN(trx_sys->rw_trx_list); + for (trx_t* trx = UT_LIST_GET_FIRST(trx_sys->rw_trx_list); + trx != NULL; + trx = UT_LIST_GET_NEXT(trx_list, trx)) { + ut_ad(trx->in_rw_trx_list); + trx_mutex_enter(trx); + switch (trx->state) { + case TRX_STATE_NOT_STARTED: + DBUG_ASSERT(!"invalid state"); + /* fall through */ + case TRX_STATE_PREPARED: + case TRX_STATE_PREPARED_RECOVERED: + break; + case TRX_STATE_ACTIVE: + case TRX_STATE_COMMITTED_IN_MEMORY: + total_trx++; + } + trx_mutex_exit(trx); + } for (trx_t* trx = UT_LIST_GET_FIRST(trx_sys->mysql_trx_list); trx != NULL; trx = UT_LIST_GET_NEXT(mysql_trx_list, trx)) { - total_trx += trx->state != TRX_STATE_NOT_STARTED; + ut_ad(trx->in_mysql_trx_list); + trx_mutex_enter(trx); + /* This may count some ACTIVE transactions twice, + both in rw_trx_list and mysql_trx_list. */ + total_trx += trx->state == TRX_STATE_ACTIVE; + /* Any PREPARED or COMMITTED transactions must be + in rw_trx_list, so it suffices to count them there. */ + ut_ad(trx->in_rw_trx_list + || trx->state == TRX_STATE_NOT_STARTED + || trx->state == TRX_STATE_ACTIVE); + trx_mutex_exit(trx); } - ut_a(total_trx >= trx_sys->n_prepared_trx); - total_trx -= trx_sys->n_prepared_trx; - trx_sys_mutex_exit(); return(total_trx); diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc index 1f6ed7a48d4..d6fbc65ea55 100644 --- a/storage/innobase/trx/trx0trx.cc +++ b/storage/innobase/trx/trx0trx.cc @@ -526,6 +526,11 @@ trx_free_prepared( /*==============*/ trx_t* trx) /*!< in, own: trx object */ { + ut_ad(trx->state == TRX_STATE_PREPARED + || trx->state == TRX_STATE_PREPARED_RECOVERED + || !srv_was_started + || srv_read_only_mode + || srv_force_recovery >= SRV_FORCE_NO_TRX_UNDO); ut_a(trx_state_eq(trx, TRX_STATE_PREPARED) || trx_state_eq(trx, TRX_STATE_PREPARED_RECOVERED) || (trx->is_recovered @@ -545,7 +550,9 @@ trx_free_prepared( ut_a(!trx->read_only); - ut_d(trx->in_rw_trx_list = FALSE); + ut_ad(trx->in_rw_trx_list); + UT_LIST_REMOVE(trx_sys->rw_trx_list, trx); + ut_d(trx->in_rw_trx_list = false); DBUG_LOG("trx", "Free prepared: " << trx); trx->state = TRX_STATE_NOT_STARTED; @@ -594,7 +601,6 @@ trx_disconnect_from_mysql( ut_ad(trx_state_eq(trx, TRX_STATE_PREPARED)); trx->is_recovered = true; - trx_sys->n_prepared_recovered_trx++; trx->mysql_thd = NULL; /* todo/fixme: suggest to do it at innodb prepare */ trx->will_lock = 0; @@ -756,8 +762,6 @@ trx_resurrect_insert( << " was in the XA prepared state."; trx->state = TRX_STATE_PREPARED; - trx_sys->n_prepared_trx++; - trx_sys->n_prepared_recovered_trx++; } else { trx->state = TRX_STATE_COMMITTED_IN_MEMORY; } @@ -815,13 +819,8 @@ trx_resurrect_update_in_prepared_state( if (undo->state == TRX_UNDO_PREPARED) { ib::info() << "Transaction " << trx_get_id_for_print(trx) << " was in the XA prepared state."; - - if (trx_state_eq(trx, TRX_STATE_NOT_STARTED)) { - trx_sys->n_prepared_trx++; - trx_sys->n_prepared_recovered_trx++; - } else { - ut_ad(trx_state_eq(trx, TRX_STATE_PREPARED)); - } + ut_ad(trx_state_eq(trx, TRX_STATE_NOT_STARTED) + || trx_state_eq(trx, TRX_STATE_PREPARED)); trx->state = TRX_STATE_PREPARED; } else { @@ -2609,10 +2608,9 @@ trx_prepare( /*--------------------------------------*/ ut_a(trx->state == TRX_STATE_ACTIVE); - trx_sys_mutex_enter(); + trx_mutex_enter(trx); trx->state = TRX_STATE_PREPARED; - trx_sys->n_prepared_trx++; - trx_sys_mutex_exit(); + trx_mutex_exit(trx); /*--------------------------------------*/ if (lsn) { From b07beff8943d083cf900c8f2ea0a386a84d9f8db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Tue, 3 Sep 2019 12:31:37 +0300 Subject: [PATCH 09/14] MDEV-15326: InnoDB: Failing assertion: !other_lock MySQL 5.7.9 (and MariaDB 10.2.2) introduced a race condition between InnoDB transaction commit and the conversion of implicit locks into explicit ones. The assertion failure can be triggered with a test that runs 3 concurrent single-statement transactions in a loop on a simple table: CREATE TABLE t (a INT PRIMARY KEY) ENGINE=InnoDB; thread1: INSERT INTO t SET a=1; thread2: DELETE FROM t; thread3: SELECT * FROM t FOR UPDATE; -- or DELETE FROM t; The failure scenarios are like the following: (1) The INSERT statement is being committed, waiting for lock_sys->mutex. (2) At the time of the failure, both the DELETE and SELECT transactions are active but have not logged any changes yet. (3) The transaction where the !other_lock assertion fails started lock_rec_convert_impl_to_expl(). (4) After this point, the commit of the INSERT removed the transaction from trx_sys->rw_trx_set, in trx_erase_lists(). (5) The other transaction consulted trx_sys->rw_trx_set and determined that there is no implicit lock. Hence, it grabbed the lock. (6) The !other_lock assertion fails in lock_rec_add_to_queue() for the lock_rec_convert_impl_to_expl(), because the lock was 'stolen'. This assertion failure looks genuine, because the INSERT transaction is still active (trx->state=TRX_STATE_ACTIVE). The problematic step (4) was introduced in mysql/mysql-server@e27e0e0bb75b4d35e87059816f1cc370c09890ad which fixed something related to MVCC (covered by the test innodb.innodb-read-view). Basically, it reintroduced an error that had been mentioned in an earlier commit mysql/mysql-server@a17be6963fc0d9210fa0642d3985b7219cdaf0c5: "The active transaction was removed from trx_sys->rw_trx_set prematurely." Our fix goes along the following lines: (a) Implicit locks will released by assigning trx->state=TRX_STATE_COMMITTED_IN_MEMORY as the first step. This transition will no longer be protected by lock_sys_t::mutex, only by trx->mutex. This idea is by Sergey Vojtovich. (b) We detach the transaction from trx_sys before starting to release explicit locks. (c) All callers of trx_rw_is_active() and trx_rw_is_active_low() must recheck trx->state after acquiring trx->mutex. (d) Before releasing any explicit locks, we will ensure that any activity by other threads to convert implicit locks into explicit will have ceased, by checking !trx_is_referenced(trx). There was a glitch in this check when it was part of lock_trx_release_locks(); at the end we would release trx->mutex and acquire lock_sys->mutex and trx->mutex, and fail to recheck (trx_is_referenced() is protected by trx_t::mutex). (e) Explicit locks can be released in batches (LOCK_RELEASE_INTERVAL=1000) just like we did before. trx_t::state: Document that the transition to COMMITTED is only protected by trx_t::mutex, no longer by lock_sys_t::mutex. trx_rw_is_active_low(), trx_rw_is_active(): Document that the transaction state should be rechecked after acquiring trx_t::mutex. trx_t::commit_state(): New function to change a transaction to committed state, to release implicit locks. trx_t::release_locks(): New function to release the explicit locks after commit_state(). lock_trx_release_locks(): Move much of the logic to the caller (which must invoke trx_t::commit_state() and trx_t::release_locks() as needed), and assert that the transaction will have locks. trx_get_trx_by_xid(): Make the parameter a pointer to const. lock_rec_other_trx_holds_expl(): Recheck trx->state after acquiring trx->mutex, and avoid a redundant lookup of the transaction. lock_rec_queue_validate(): Recheck impl_trx->state while holding impl_trx->mutex. row_vers_impl_x_locked(), row_vers_impl_x_locked_low(): Document that the transaction state must be rechecked after trx_mutex_enter(). trx_free_prepared(): Adjust for the changes to lock_trx_release_locks(). --- sql/handler.h | 4 +- storage/innobase/include/lock0lock.h | 13 +- storage/innobase/include/row0vers.h | 3 +- storage/innobase/include/trx0sys.h | 41 ++--- storage/innobase/include/trx0sys.ic | 81 +++++----- storage/innobase/include/trx0trx.h | 53 +++---- storage/innobase/lock/lock0lock.cc | 227 ++++++++------------------- storage/innobase/row/row0vers.cc | 49 +++--- storage/innobase/trx/trx0roll.cc | 6 +- storage/innobase/trx/trx0trx.cc | 153 +++++++++++------- storage/innobase/trx/trx0undo.cc | 4 +- 11 files changed, 280 insertions(+), 354 deletions(-) diff --git a/sql/handler.h b/sql/handler.h index fcf9e7b0a6e..a51d5dae01a 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -575,9 +575,9 @@ struct xid_t { char data[XIDDATASIZE]; // not \0-terminated ! xid_t() {} /* Remove gcc warning */ - bool eq(struct xid_t *xid) + bool eq(struct xid_t *xid) const { return !xid->is_null() && eq(xid->gtrid_length, xid->bqual_length, xid->data); } - bool eq(long g, long b, const char *d) + bool eq(long g, long b, const char *d) const { return !is_null() && g == gtrid_length && b == bqual_length && !memcmp(d, data, g+b); } void set(struct xid_t *xid) { memcpy(this, xid, xid->length()); } diff --git a/storage/innobase/include/lock0lock.h b/storage/innobase/include/lock0lock.h index 18bc3abfaf1..8369db8b879 100644 --- a/storage/innobase/include/lock0lock.h +++ b/storage/innobase/include/lock0lock.h @@ -499,14 +499,11 @@ lock_rec_unlock( const buf_block_t* block, /*!< in: buffer block containing rec */ const rec_t* rec, /*!< in: record */ lock_mode lock_mode);/*!< in: LOCK_S or LOCK_X */ -/*********************************************************************//** -Releases a transaction's locks, and releases possible other transactions -waiting because of these locks. Change the state of the transaction to -TRX_STATE_COMMITTED_IN_MEMORY. */ -void -lock_trx_release_locks( -/*===================*/ - trx_t* trx); /*!< in/out: transaction */ + +/** Release the explicit locks of a committing transaction, +and release possible other transactions waiting because of these locks. */ +void lock_trx_release_locks(trx_t* trx); + /*********************************************************************//** Removes locks on a table to be dropped or discarded. If remove_also_table_sx_locks is TRUE then table-level S and X locks are diff --git a/storage/innobase/include/row0vers.h b/storage/innobase/include/row0vers.h index ef60423506e..b5279e5851b 100644 --- a/storage/innobase/include/row0vers.h +++ b/storage/innobase/include/row0vers.h @@ -43,7 +43,8 @@ index record. @param[in] rec secondary index record @param[in] index secondary index @param[in] offsets rec_get_offsets(rec, index) -@return the active transaction; trx_release_reference() must be invoked +@return the active transaction; state must be rechecked after +trx_mutex_enter(), and trx_release_reference() must be invoked @retval NULL if the record was committed */ trx_t* row_vers_impl_x_locked( diff --git a/storage/innobase/include/trx0sys.h b/storage/innobase/include/trx0sys.h index 37ac8975783..c4b1636cfd2 100644 --- a/storage/innobase/include/trx0sys.h +++ b/storage/innobase/include/trx0sys.h @@ -1,7 +1,7 @@ /***************************************************************************** Copyright (c) 1996, 2016, Oracle and/or its affiliates. All Rights Reserved. -Copyright (c) 2017, MariaDB Corporation. +Copyright (c) 2017, 2019, MariaDB Corporation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software @@ -186,30 +186,21 @@ UNIV_INLINE trx_id_t trx_rw_min_trx_id(void); /*===================*/ -/****************************************************************//** -Checks if a rw transaction with the given id is active. -@return transaction instance if active, or NULL */ -UNIV_INLINE -trx_t* -trx_rw_is_active_low( -/*=================*/ - trx_id_t trx_id, /*!< in: trx id of the transaction */ - ibool* corrupt); /*!< in: NULL or pointer to a flag - that will be set if corrupt */ -/****************************************************************//** -Checks if a rw transaction with the given id is active. If the caller is -not holding trx_sys->mutex, the transaction may already have been -committed. -@return transaction instance if active, or NULL; */ -UNIV_INLINE -trx_t* -trx_rw_is_active( -/*=============*/ - trx_id_t trx_id, /*!< in: trx id of the transaction */ - ibool* corrupt, /*!< in: NULL or pointer to a flag - that will be set if corrupt */ - bool do_ref_count); /*!< in: if true then increment the - trx_t::n_ref_count */ +/** Look up a rw transaction with the given id. +@param[in] trx_id transaction identifier +@param[out] corrupt flag that will be set if trx_id is corrupted +@return transaction; its state should be rechecked after acquiring trx_t::mutex +@retval NULL if there is no transaction identified by trx_id. */ +inline trx_t* trx_rw_is_active_low(trx_id_t trx_id, bool* corrupt); + +/** Look up a rw transaction with the given id. +@param[in] trx_id transaction identifier +@param[out] corrupt flag that will be set if trx_id is corrupted +@param[in] ref_count whether to increment trx->n_ref +@return transaction; its state should be rechecked after acquiring trx_t::mutex +@retval NULL if there is no active transaction identified by trx_id. */ +inline trx_t* trx_rw_is_active(trx_id_t trx_id, bool* corrupt, bool ref_count); + #if defined UNIV_DEBUG || defined UNIV_BLOB_LIGHT_DEBUG /***********************************************************//** Assert that a transaction has been recovered. diff --git a/storage/innobase/include/trx0sys.ic b/storage/innobase/include/trx0sys.ic index bfd345a27db..ac84404a397 100644 --- a/storage/innobase/include/trx0sys.ic +++ b/storage/innobase/include/trx0sys.ic @@ -1,7 +1,7 @@ /***************************************************************************** Copyright (c) 1996, 2015, Oracle and/or its affiliates. All Rights Reserved. -Copyright (c) 2018, MariaDB Corporation. +Copyright (c) 2018, 2019, MariaDB Corporation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software @@ -302,61 +302,38 @@ trx_rw_min_trx_id(void) return(id); } -/****************************************************************//** -Checks if a rw transaction with the given id is active. If the caller is -not holding lock_sys->mutex, the transaction may already have been committed. -@return transaction instance if active, or NULL */ -UNIV_INLINE -trx_t* -trx_rw_is_active_low( -/*=================*/ - trx_id_t trx_id, /*!< in: trx id of the transaction */ - ibool* corrupt) /*!< in: NULL or pointer to a flag - that will be set if corrupt */ +/** Look up a rw transaction with the given id. +@param[in] trx_id transaction identifier +@param[out] corrupt flag that will be set if trx_id is corrupted +@return transaction; its state should be rechecked after acquiring trx_t::mutex +@retval NULL if there is no transaction identified by trx_id. */ +inline trx_t* trx_rw_is_active_low(trx_id_t trx_id, bool* corrupt) { - trx_t* trx; - ut_ad(trx_sys_mutex_own()); if (trx_id < trx_rw_min_trx_id_low()) { - - trx = NULL; } else if (trx_id >= trx_sys->max_trx_id) { /* There must be corruption: we let the caller handle the diagnostic prints in this case. */ - trx = NULL; if (corrupt != NULL) { - *corrupt = TRUE; - } - } else { - trx = trx_get_rw_trx_by_id(trx_id); - - if (trx != NULL - && trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)) { - - trx = NULL; + *corrupt = true; } + } else if (trx_t* trx = trx_get_rw_trx_by_id(trx_id)) { + return trx; } - return(trx); + return NULL; } -/****************************************************************//** -Checks if a rw transaction with the given id is active. If the caller is -not holding lock_sys->mutex, the transaction may already have been -committed. -@return transaction instance if active, or NULL; */ -UNIV_INLINE -trx_t* -trx_rw_is_active( -/*=============*/ - trx_id_t trx_id, /*!< in: trx id of the transaction */ - ibool* corrupt, /*!< in: NULL or pointer to a flag - that will be set if corrupt */ - bool do_ref_count) /*!< in: if true then increment the - trx_t::n_ref_count */ +/** Look up a rw transaction with the given id. +@param[in] trx_id transaction identifier +@param[out] corrupt flag that will be set if trx_id is corrupted +@param[in] ref_count whether to increment trx->n_ref +@return transaction; its state should be rechecked after acquiring trx_t::mutex +@retval NULL if there is no active transaction identified by trx_id. */ +inline trx_t* trx_rw_is_active(trx_id_t trx_id, bool* corrupt, bool ref_count) { ut_ad(trx_id); @@ -364,13 +341,25 @@ trx_rw_is_active( trx_t* trx = trx_rw_is_active_low(trx_id, corrupt); - if (trx && do_ref_count) { - trx_mutex_enter(trx); - ut_ad(!trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)); + if (trx && ref_count) { + TrxMutex* trx_mutex = &trx->mutex; + mutex_enter(trx_mutex); + ut_ad(!trx_state_eq(trx, TRX_STATE_NOT_STARTED)); ut_ad(trx->id == trx_id); ut_ad(trx->n_ref >= 0); - ++trx->n_ref; - trx_mutex_exit(trx); + if (trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)) { + /* We have an early state check here to avoid + committer starvation in a wait loop for + transaction references, when there's a stream of + trx_rw_is_active() calls from other threads. + The trx->state may change to COMMITTED after + trx_mutex is released, and it will have to be + rechecked by the caller after reacquiring the mutex. */ + trx = NULL; + } else { + ++trx->n_ref; + } + mutex_exit(trx_mutex); } trx_sys_mutex_exit(); diff --git a/storage/innobase/include/trx0trx.h b/storage/innobase/include/trx0trx.h index 62f7f172453..796e6b8bda6 100644 --- a/storage/innobase/include/trx0trx.h +++ b/storage/innobase/include/trx0trx.h @@ -249,16 +249,13 @@ trx_recover_for_mysql( /*==================*/ XID* xid_list, /*!< in/out: prepared transactions */ ulint len); /*!< in: number of slots in xid_list */ -/*******************************************************************//** -This function is used to find one X/Open XA distributed transaction -which is in the prepared state -@return trx or NULL; on match, the trx->xid will be invalidated; -note that the trx may have been committed, unless the caller is -holding lock_sys->mutex */ -trx_t * -trx_get_trx_by_xid( -/*===============*/ - XID* xid); /*!< in: X/Open XA transaction identifier */ +/** Look up an X/Open distributed transaction in XA PREPARE state. +@param[in] xid X/Open XA transaction identifier +@return transaction on match (the trx_t::xid will be invalidated); +note that the trx may have been committed before the caller acquires +trx_t::mutex +@retval NULL if no match */ +trx_t* trx_get_trx_by_xid(const XID* xid); /**********************************************************************//** If required, flushes the log to disk if we called trx_commit_for_mysql() with trx->flush_log_later == TRUE. */ @@ -578,6 +575,9 @@ Check transaction state */ ut_ad(!MVCC::is_view_active((t)->read_view)); \ ut_ad((t)->lock.wait_thr == NULL); \ ut_ad(UT_LIST_GET_LEN((t)->lock.trx_locks) == 0); \ + ut_ad((t)->lock.table_locks.empty()); \ + ut_ad(!(t)->autoinc_locks \ + || ib_vector_is_empty((t)->autoinc_locks)); \ ut_ad((t)->dict_operation == TRX_DICT_OP_NONE); \ } while(0) @@ -754,8 +754,8 @@ so without holding any mutex. The following are exceptions to this: * trx_rollback_resurrected() may access resurrected (connectionless) transactions while the system is already processing new user -transactions. The trx_sys->mutex prevents a race condition between it -and lock_trx_release_locks() [invoked by trx_commit()]. +transactions. The trx_sys->mutex and trx->is_recovered prevent +a race condition between it and trx_commit(). * trx_print_low() may access transactions not associated with the current thread. The caller must be holding trx_sys->mutex and lock_sys->mutex. @@ -767,7 +767,7 @@ holding trx_sys->mutex exclusively. * The locking code (in particular, lock_deadlock_recursive() and lock_rec_convert_impl_to_expl()) will access transactions associated to other connections. The locks of transactions are protected by -lock_sys->mutex and sometimes by trx->mutex. */ +lock_sys->mutex (insertions also by trx->mutex). */ /** Represents an instance of rollback segment along with its state variables.*/ struct trx_undo_ptr_t { @@ -873,14 +873,12 @@ struct trx_t { ACTIVE->COMMITTED is possible when the transaction is in rw_trx_list. - Transitions to COMMITTED are protected by both lock_sys->mutex - and trx->mutex. - - NOTE: Some of these state change constraints are an overkill, - currently only required for a consistent view for printing stats. - This unnecessarily adds a huge cost for the general case. */ - + Transitions to COMMITTED are protected by trx_t::mutex. */ trx_state_t state; + /** whether this is a recovered transaction that should be + rolled back by trx_rollback_or_clean_recovered(). + Protected by trx_t::mutex for transactions that are in trx_sys. */ + bool is_recovered; ReadView* read_view; /*!< consistent read view used in the transaction, or NULL if not yet set */ @@ -895,13 +893,8 @@ struct trx_t { trx_lock_t lock; /*!< Information about the transaction locks and state. Protected by - trx->mutex or lock_sys->mutex - or both */ - bool is_recovered; /*!< 0=normal transaction, - 1=recovered, must be rolled back, - protected by trx_sys->mutex when - trx->in_rw_trx_list holds */ - + lock_sys->mutex (insertions also + by trx_t::mutex). */ /* These fields are not protected by any mutex. */ const char* op_info; /*!< English text describing the @@ -1185,6 +1178,12 @@ public: return flush_observer; } + /** Transition to committed state, to release implicit locks. */ + inline void commit_state(); + + /** Release any explicit locks of a committing transaction. */ + inline void release_locks(); + private: /** Assign a rollback segment for modifying temporary tables. @return the assigned rollback segment */ diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index 8634f7e73c5..49c0e1da743 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -1273,30 +1273,31 @@ lock_rec_other_trx_holds_expl( trx_t* holds = NULL; lock_mutex_enter(); + mutex_enter(&trx_sys->mutex); + trx_mutex_enter(trx); - if (trx_t* impl_trx = trx_rw_is_active(trx->id, NULL, false)) { - ulint heap_no = page_rec_get_heap_no(rec); - mutex_enter(&trx_sys->mutex); + ut_ad(!trx_state_eq(trx, TRX_STATE_NOT_STARTED)); + if (!trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)) { + const ulint heap_no = page_rec_get_heap_no(rec); for (trx_t* t = UT_LIST_GET_FIRST(trx_sys->rw_trx_list); t != NULL; t = UT_LIST_GET_NEXT(trx_list, t)) { lock_t* expl_lock = lock_rec_has_expl( precise_mode, block, heap_no, t); - - if (expl_lock && expl_lock->trx != impl_trx) { + if (expl_lock && expl_lock->trx != trx) { /* An explicit lock is held by trx other than the trx holding the implicit lock. */ holds = expl_lock->trx; break; } } - - mutex_exit(&trx_sys->mutex); } lock_mutex_exit(); + mutex_exit(&trx_sys->mutex); + trx_mutex_exit(trx); return(holds); } @@ -5107,7 +5108,7 @@ lock_trx_table_locks_find( { bool found = false; - trx_mutex_enter(trx); + ut_ad(trx_mutex_own(trx)); for (lock_list::const_iterator it = trx->lock.table_locks.begin(), end = trx->lock.table_locks.end(); it != end; ++it) { @@ -5130,8 +5131,6 @@ lock_trx_table_locks_find( ut_a(lock->un_member.tab_lock.table != NULL); } - trx_mutex_exit(trx); - return(found); } @@ -5155,21 +5154,20 @@ lock_table_queue_validate( /* lock->trx->state cannot change from or to NOT_STARTED while we are holding the trx_sys->mutex. It may change - from ACTIVE to PREPARED, but it may not change to - COMMITTED, because we are holding the lock_sys->mutex. */ + from ACTIVE or PREPARED to PREPARED or COMMITTED. */ + trx_mutex_enter(lock->trx); ut_ad(trx_assert_started(lock->trx)); - - if (!lock_get_wait(lock)) { - + if (trx_state_eq(lock->trx, TRX_STATE_COMMITTED_IN_MEMORY)) { + } else if (!lock_get_wait(lock)) { ut_a(!lock_table_other_has_incompatible( lock->trx, 0, table, lock_get_mode(lock))); } else { - ut_a(lock_table_has_to_wait_in_queue(lock)); } ut_a(lock_trx_table_locks_find(lock->trx, lock)); + trx_mutex_exit(lock->trx); } return(TRUE); @@ -5191,7 +5189,6 @@ lock_rec_queue_validate( const dict_index_t* index, /*!< in: index, or NULL if not known */ const ulint* offsets)/*!< in: rec_get_offsets(rec, index) */ { - const trx_t* impl_trx; const lock_t* lock; ulint heap_no; @@ -5217,40 +5214,34 @@ lock_rec_queue_validate( lock != NULL; lock = lock_rec_get_next_const(heap_no, lock)) { + ut_ad(!index || lock->index == index); + + trx_mutex_enter(lock->trx); ut_ad(!trx_is_ac_nl_ro(lock->trx)); - - if (lock_get_wait(lock)) { - ut_a(lock_rec_has_to_wait_in_queue(lock)); - } - - if (index != NULL) { - ut_a(lock->index == index); - } + ut_ad(trx_state_eq(lock->trx, + TRX_STATE_COMMITTED_IN_MEMORY) + || !lock_get_wait(lock) + || lock_rec_has_to_wait_in_queue(lock)); + trx_mutex_exit(lock->trx); } goto func_exit; } ut_ad(page_rec_is_leaf(rec)); + ut_ad(lock_mutex_own()); - if (index == NULL) { - + if (!index || !index->is_primary()) { /* Nothing we can do */ + } else if (trx_t* impl_trx = trx_rw_is_active_low( + lock_clust_rec_some_has_impl(rec, index, offsets), + NULL)) { + /* impl_trx could have been committed before we + acquire its mutex, but not thereafter. */ - } else if (dict_index_is_clust(index)) { - trx_id_t trx_id; - - /* Unlike the non-debug code, this invariant can only succeed - if the check and assertion are covered by the lock mutex. */ - - trx_id = lock_clust_rec_some_has_impl(rec, index, offsets); - impl_trx = trx_rw_is_active_low(trx_id, NULL); - - ut_ad(lock_mutex_own()); - /* impl_trx cannot be committed until lock_mutex_exit() - because lock_trx_release_locks() acquires lock_sys->mutex */ - - if (!impl_trx) { + mutex_enter(&impl_trx->mutex); + ut_ad(impl_trx->state != TRX_STATE_NOT_STARTED); + if (impl_trx->state == TRX_STATE_COMMITTED_IN_MEMORY) { } else if (const lock_t* other_lock = lock_rec_other_has_expl_req( LOCK_S, block, true, heap_no, @@ -5292,6 +5283,8 @@ lock_rec_queue_validate( ut_ad(lock_rec_has_expl(LOCK_X | LOCK_REC_NOT_GAP, block, heap_no, impl_trx)); } + + mutex_exit(&impl_trx->mutex); } for (lock = lock_rec_get_first(lock_sys->rec_hash, block, heap_no); @@ -5779,29 +5772,23 @@ lock_rec_convert_impl_to_expl_for_trx( trx_t* trx, /*!< in/out: active transaction */ ulint heap_no)/*!< in: rec heap number to lock */ { - ut_ad(trx_is_referenced(trx)); - DEBUG_SYNC_C("before_lock_rec_convert_impl_to_expl_for_trx"); lock_mutex_enter(); - + trx_mutex_enter(trx); ut_ad(!trx_state_eq(trx, TRX_STATE_NOT_STARTED)); if (!trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY) && !lock_rec_has_expl(LOCK_X | LOCK_REC_NOT_GAP, block, heap_no, trx)) { - - ulint type_mode; - - type_mode = (LOCK_REC | LOCK_X | LOCK_REC_NOT_GAP); - - lock_rec_add_to_queue( - type_mode, block, heap_no, index, trx, FALSE); + lock_rec_add_to_queue(LOCK_REC | LOCK_X | LOCK_REC_NOT_GAP, + block, heap_no, index, trx, true); } lock_mutex_exit(); - - trx_release_reference(trx); + ut_ad(trx->n_ref > 0); + --trx->n_ref; + trx_mutex_exit(trx); DEBUG_SYNC_C("after_lock_rec_convert_impl_to_expl_for_trx"); } @@ -6541,118 +6528,24 @@ lock_unlock_table_autoinc( } } -/*********************************************************************//** -Releases a transaction's locks, and releases possible other transactions -waiting because of these locks. Change the state of the transaction to -TRX_STATE_COMMITTED_IN_MEMORY. */ -void -lock_trx_release_locks( -/*===================*/ - trx_t* trx) /*!< in/out: transaction */ +/** Release the explicit locks of a committing transaction, +and release possible other transactions waiting because of these locks. */ +void lock_trx_release_locks(trx_t* trx) { - check_trx_state(trx); - - ut_ad(trx_state_eq(trx, TRX_STATE_ACTIVE) - || trx_state_eq(trx, TRX_STATE_PREPARED) - || trx_state_eq(trx, TRX_STATE_PREPARED_RECOVERED) - || (trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY) - && trx->is_recovered - && !UT_LIST_GET_LEN(trx->lock.trx_locks))); - - bool release_lock; - - release_lock = (UT_LIST_GET_LEN(trx->lock.trx_locks) > 0); - - /* Don't take lock_sys mutex if trx didn't acquire any lock. */ - if (release_lock) { - - /* The transition of trx->state to TRX_STATE_COMMITTED_IN_MEMORY - is protected by both the lock_sys->mutex and the trx->mutex. */ - lock_mutex_enter(); - } - - trx_mutex_enter(trx); - - /* The following assignment makes the transaction committed in memory - and makes its changes to data visible to other transactions. - NOTE that there is a small discrepancy from the strict formal - visibility rules here: a human user of the database can see - modifications made by another transaction T even before the necessary - log segment has been flushed to the disk. If the database happens to - crash before the flush, the user has seen modifications from T which - will never be a committed transaction. However, any transaction T2 - which sees the modifications of the committing transaction T, and - which also itself makes modifications to the database, will get an lsn - larger than the committing transaction T. In the case where the log - flush fails, and T never gets committed, also T2 will never get - committed. */ - - /*--------------------------------------*/ - trx->state = TRX_STATE_COMMITTED_IN_MEMORY; - /*--------------------------------------*/ - - if (trx_is_referenced(trx)) { - - ut_a(release_lock); - - lock_mutex_exit(); - - while (trx_is_referenced(trx)) { - - trx_mutex_exit(trx); - - DEBUG_SYNC_C("waiting_trx_is_not_referenced"); - - /** Doing an implicit to explicit conversion - should not be expensive. */ - ut_delay(ut_rnd_interval(0, srv_spin_wait_delay)); - - trx_mutex_enter(trx); - } - - trx_mutex_exit(trx); - - lock_mutex_enter(); - - trx_mutex_enter(trx); - } - - ut_ad(!trx_is_referenced(trx)); - - /* If the background thread trx_rollback_or_clean_recovered() - is still active then there is a chance that the rollback - thread may see this trx as COMMITTED_IN_MEMORY and goes ahead - to clean it up calling trx_cleanup_at_db_startup(). This can - happen in the case we are committing a trx here that is left - in PREPARED state during the crash. Note that commit of the - rollback of a PREPARED trx happens in the recovery thread - while the rollback of other transactions happen in the - background thread. To avoid this race we unconditionally unset - the is_recovered flag. */ - - trx->is_recovered = false; - - trx_mutex_exit(trx); - - if (release_lock) { - - lock_release(trx); - - lock_mutex_exit(); - } + ut_ad(UT_LIST_GET_LEN(trx->lock.trx_locks)); + lock_mutex_enter(); + lock_release(trx); trx->lock.n_rec_locks = 0; - /* We don't remove the locks one by one from the vector for efficiency reasons. We simply reset it because we would have released all the locks anyway. */ trx->lock.table_locks.clear(); - ut_a(UT_LIST_GET_LEN(trx->lock.trx_locks) == 0); - ut_a(ib_vector_is_empty(trx->autoinc_locks)); - ut_a(trx->lock.table_locks.empty()); - + ut_ad(UT_LIST_GET_LEN(trx->lock.trx_locks) == 0); + ut_ad(ib_vector_is_empty(trx->autoinc_locks)); + lock_mutex_exit(); mem_heap_empty(trx->lock.lock_heap); } @@ -6724,18 +6617,16 @@ lock_table_locks_lookup( itself */ const trx_ut_list_t* trx_list) /*!< in: trx list to check */ { - trx_t* trx; - ut_a(table != NULL); ut_ad(lock_mutex_own()); ut_ad(trx_sys_mutex_own()); - for (trx = UT_LIST_GET_FIRST(*trx_list); + for (trx_t* trx = UT_LIST_GET_FIRST(*trx_list); trx != NULL; trx = UT_LIST_GET_NEXT(trx_list, trx)) { + const lock_t* lock; - const lock_t* lock; - + trx_mutex_enter(trx); check_trx_state(trx); for (lock = UT_LIST_GET_FIRST(trx->lock.trx_locks); @@ -6748,15 +6639,21 @@ lock_table_locks_lookup( ut_ad(!dict_index_is_online_ddl(lock->index) || dict_index_is_clust(lock->index)); if (lock->index->table == table) { - return(lock); + break; } } else if (lock->un_member.tab_lock.table == table) { - return(lock); + break; } } + + trx_mutex_exit(trx); + + if (lock) { + return lock; + } } - return(NULL); + return NULL; } #endif /* UNIV_DEBUG */ @@ -6921,7 +6818,7 @@ DeadlockChecker::start_print() if (srv_print_all_deadlocks) { ib::info() << "Transactions deadlock detected, dumping" - << " detailed information."; + " detailed information."; } } diff --git a/storage/innobase/row/row0vers.cc b/storage/innobase/row/row0vers.cc index 7db1b936348..533d6df1321 100644 --- a/storage/innobase/row/row0vers.cc +++ b/storage/innobase/row/row0vers.cc @@ -76,7 +76,8 @@ index record. @param[in] index secondary index @param[in] offsets rec_get_offsets(rec, index) @param[in,out] mtr mini-transaction -@return the active transaction; trx_release_reference() must be invoked +@return the active transaction; state must be rechecked after +trx_mutex_enter(), and trx_release_reference() must be invoked @retval NULL if the record was committed */ UNIV_INLINE trx_t* @@ -88,11 +89,6 @@ row_vers_impl_x_locked_low( const ulint* offsets, mtr_t* mtr) { - trx_id_t trx_id; - ibool corrupt; - ulint comp; - ulint rec_del; - const rec_t* version; rec_t* prev_version = NULL; ulint* clust_offsets; mem_heap_t* heap; @@ -109,11 +105,12 @@ row_vers_impl_x_locked_low( clust_offsets = rec_get_offsets( clust_rec, clust_index, NULL, true, ULINT_UNDEFINED, &heap); - trx_id = row_get_rec_trx_id(clust_rec, clust_index, clust_offsets); - corrupt = FALSE; + const trx_id_t trx_id = row_get_rec_trx_id( + clust_rec, clust_index, clust_offsets); - ut_ad(!dict_table_is_temporary(clust_index->table)); + ut_ad(!clust_index->table->is_temporary()); + bool corrupt = false; trx_t* trx = trx_rw_is_active(trx_id, &corrupt, true); if (trx == 0) { @@ -128,12 +125,12 @@ row_vers_impl_x_locked_low( DBUG_RETURN(0); } - comp = page_rec_is_comp(rec); + const ulint comp = page_rec_is_comp(rec); ut_ad(index->table == clust_index->table); ut_ad(!!comp == dict_table_is_comp(index->table)); ut_ad(!comp == !page_rec_is_comp(clust_rec)); - rec_del = rec_get_deleted_flag(rec, comp); + const ulint rec_del = rec_get_deleted_flag(rec, comp); if (dict_index_has_virtual(index)) { ulint n_ext; @@ -158,7 +155,7 @@ row_vers_impl_x_locked_low( modify rec, and does not necessarily have an implicit x-lock on rec. */ - for (version = clust_rec;; version = prev_version) { + for (const rec_t* version = clust_rec;; version = prev_version) { row_ext_t* ext; dtuple_t* row; dtuple_t* entry; @@ -178,16 +175,24 @@ row_vers_impl_x_locked_low( heap, &prev_version, NULL, dict_index_has_virtual(index) ? &vrow : NULL, 0); + trx_mutex_enter(trx); + const bool committed = trx_state_eq( + trx, TRX_STATE_COMMITTED_IN_MEMORY); + trx_mutex_exit(trx); + /* The oldest visible clustered index version must not be delete-marked, because we never start a transaction by inserting a delete-marked record. */ - ut_ad(prev_version - || !rec_get_deleted_flag(version, comp) - || !trx_rw_is_active(trx_id, NULL, false)); + ut_ad(committed || prev_version + || !rec_get_deleted_flag(version, comp)); /* Free version and clust_offsets. */ mem_heap_free(old_heap); + if (committed) { + goto not_locked; + } + if (prev_version == NULL) { /* We reached the oldest visible version without @@ -207,6 +212,7 @@ row_vers_impl_x_locked_low( or updated, the leaf page record always is created with a clear delete-mark flag. (We never insert a delete-marked record.) */ +not_locked: trx_release_reference(trx); trx = 0; } @@ -333,14 +339,14 @@ result_check: if (trx->id != prev_trx_id) { /* prev_version was the first version modified by the trx_id transaction: no implicit x-lock */ - - trx_release_reference(trx); - trx = 0; - break; + goto not_locked; } } - DBUG_PRINT("info", ("Implicit lock is held by trx:" TRX_ID_FMT, trx_id)); + if (trx) { + DBUG_PRINT("info", ("Implicit lock is held by trx:" TRX_ID_FMT, + trx_id)); + } if (v_heap != NULL) { mem_heap_free(v_heap); @@ -355,7 +361,8 @@ index record. @param[in] rec secondary index record @param[in] index secondary index @param[in] offsets rec_get_offsets(rec, index) -@return the active transaction; trx_release_reference() must be invoked +@return the active transaction; state must be rechecked after +trx_mutex_enter(), and trx_release_reference() must be invoked @retval NULL if the record was committed */ trx_t* row_vers_impl_x_locked( diff --git a/storage/innobase/trx/trx0roll.cc b/storage/innobase/trx/trx0roll.cc index e827cda46eb..20798323802 100644 --- a/storage/innobase/trx/trx0roll.cc +++ b/storage/innobase/trx/trx0roll.cc @@ -697,9 +697,9 @@ trx_rollback_resurrected( ut_ad(trx_sys_mutex_own()); /* The trx->is_recovered flag and trx->state are set - atomically under the protection of the trx->mutex (and - lock_sys->mutex) in lock_trx_release_locks(). We do not want - to accidentally clean up a non-recovered transaction here. */ + atomically under the protection of the trx->mutex in + trx_t::commit_state(). We do not want to accidentally clean up + a non-recovered transaction here. */ trx_mutex_enter(trx); if (!trx->is_recovered) { diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc index d6fbc65ea55..40dcc37b30b 100644 --- a/storage/innobase/trx/trx0trx.cc +++ b/storage/innobase/trx/trx0trx.cc @@ -519,6 +519,52 @@ trx_free_for_background(trx_t* trx) trx_free(trx); } +/** Transition to committed state, to release implicit locks. */ +inline void trx_t::commit_state() +{ + /* This makes the transaction committed in memory and makes its + changes to data visible to other transactions. NOTE that there is a + small discrepancy from the strict formal visibility rules here: a + user of the database can see modifications made by another + transaction T even before the necessary redo log segment has been + flushed to the disk. If the database happens to crash before the + flush, the user has seen modifications from T which will never be a + committed transaction. However, any transaction T2 which sees the + modifications of the committing transaction T, and which also itself + makes modifications to the database, will get an lsn larger than the + committing transaction T. In the case where the log flush fails, and + T never gets committed, also T2 will never get committed. */ + ut_ad(trx_mutex_own(this)); + ut_ad(state != TRX_STATE_NOT_STARTED); + ut_ad(state != TRX_STATE_COMMITTED_IN_MEMORY + || (is_recovered && !UT_LIST_GET_LEN(lock.trx_locks))); + state= TRX_STATE_COMMITTED_IN_MEMORY; + + /* If the background thread trx_rollback_or_clean_recovered() + is still active then there is a chance that the rollback + thread may see this trx as COMMITTED_IN_MEMORY and goes ahead + to clean it up calling trx_cleanup_at_db_startup(). This can + happen in the case we are committing a trx here that is left + in PREPARED state during the crash. Note that commit of the + rollback of a PREPARED trx happens in the recovery thread + while the rollback of other transactions happen in the + background thread. To avoid this race we unconditionally unset + the is_recovered flag. */ + is_recovered= false; + ut_ad(id || !trx_is_referenced(this)); +} + +/** Release any explicit locks of a committing transaction. */ +inline void trx_t::release_locks() +{ + DBUG_ASSERT(state == TRX_STATE_COMMITTED_IN_MEMORY); + + if (UT_LIST_GET_LEN(lock.trx_locks)) + lock_trx_release_locks(this); + else + lock.table_locks.clear(); /* Work around MDEV-20483 */ +} + /********************************************************************//** At shutdown, frees a transaction object that is in the PREPARED state. */ void @@ -526,6 +572,7 @@ trx_free_prepared( /*==============*/ trx_t* trx) /*!< in, own: trx object */ { + trx_mutex_enter(trx); ut_ad(trx->state == TRX_STATE_PREPARED || trx->state == TRX_STATE_PREPARED_RECOVERED || !srv_was_started @@ -543,7 +590,9 @@ trx_free_prepared( || srv_force_recovery >= SRV_FORCE_NO_TRX_UNDO))); ut_a(trx->magic_n == TRX_MAGIC_N); - lock_trx_release_locks(trx); + trx->commit_state(); + trx_mutex_exit(trx); + trx->release_locks(); trx_undo_free_prepared(trx); assert_trx_in_rw_list(trx); @@ -556,14 +605,7 @@ trx_free_prepared( DBUG_LOG("trx", "Free prepared: " << trx); trx->state = TRX_STATE_NOT_STARTED; - - /* Undo trx_resurrect_table_locks(). */ - lock_trx_lock_list_init(&trx->lock.trx_locks); - - /* Note: This vector is not guaranteed to be empty because the - transaction was never committed and therefore lock_trx_release() - was not called. */ - trx->lock.table_locks.clear(); + ut_ad(!UT_LIST_GET_LEN(trx->lock.trx_locks)); trx->id = 0; trx_free(trx); @@ -1630,8 +1672,8 @@ trx_commit_in_memory( /* Note: We are asserting without holding the lock mutex. But that is OK because this transaction is not waiting and cannot - be rolled back and no new locks can (or should not) be added - becuase it is flagged as a non-locking read-only transaction. */ + be rolled back and no new locks can (or should) be added + because it is flagged as a non-locking read-only transaction. */ ut_a(UT_LIST_GET_LEN(trx->lock.trx_locks) == 0); @@ -1653,35 +1695,43 @@ trx_commit_in_memory( DBUG_LOG("trx", "Autocommit in memory: " << trx); trx->state = TRX_STATE_NOT_STARTED; } else { - if (trx->id > 0) { - /* For consistent snapshot, we need to remove current - transaction from running transaction id list for mvcc - before doing commit and releasing locks. */ + trx_mutex_enter(trx); + trx->commit_state(); + trx_mutex_exit(trx); + + if (trx->id) { trx_erase_lists(trx, serialised); + + /* Wait for any implicit-to-explicit lock + conversions to cease, so that there will be no + race condition in lock_release(). */ + trx_mutex_enter(trx); + while (UNIV_UNLIKELY(trx_is_referenced(trx))) { + trx_mutex_exit(trx); + ut_delay(srv_spin_wait_delay); + trx_mutex_enter(trx); + } + trx_mutex_exit(trx); + + trx->release_locks(); + trx->id = 0; + } else { + ut_ad(trx->read_only || !trx->rsegs.m_redo.rseg); + ut_ad(!trx->in_rw_trx_list); + trx->release_locks(); } - lock_trx_release_locks(trx); - ut_ad(trx->read_only || !trx->rsegs.m_redo.rseg || trx->id); - - /* Remove the transaction from the list of active - transactions now that it no longer holds any user locks. */ - - ut_ad(trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)); DEBUG_SYNC_C("after_trx_committed_in_memory"); - if (trx->read_only || trx->rsegs.m_redo.rseg == NULL) { - + if (trx->read_only || !trx->rsegs.m_redo.rseg) { MONITOR_INC(MONITOR_TRX_RO_COMMIT); - if (trx->read_view != NULL) { + if (trx->read_view) { trx_sys->mvcc->view_close( trx->read_view, false); } - } else { MONITOR_INC(MONITOR_TRX_RW_COMMIT); } - - trx->id = 0; } ut_ad(!trx->rsegs.m_redo.update_undo); @@ -2723,18 +2773,13 @@ partial: return(int (count)); } -/*******************************************************************//** -This function is used to find one X/Open XA distributed transaction -which is in the prepared state +/** Look up an X/Open distributed transaction in XA PREPARE state. +@param[in] xid X/Open XA transaction identifier @return trx on match, the trx->xid will be invalidated; -note that the trx may have been committed, unless the caller is -holding lock_sys->mutex */ +note that the trx may have been committed before the caller +acquires trx_t::mutex */ static MY_ATTRIBUTE((warn_unused_result)) -trx_t* -trx_get_trx_by_xid_low( -/*===================*/ - XID* xid) /*!< in: X/Open XA transaction - identifier */ +trx_t* trx_get_trx_by_xid_low(const XID* xid) { trx_t* trx; @@ -2743,7 +2788,7 @@ trx_get_trx_by_xid_low( for (trx = UT_LIST_GET_FIRST(trx_sys->rw_trx_list); trx != NULL; trx = UT_LIST_GET_NEXT(trx_list, trx)) { - + trx_mutex_enter(trx); assert_trx_in_rw_list(trx); /* Compare two X/Open XA transaction id's: their @@ -2759,28 +2804,28 @@ trx_get_trx_by_xid_low( /* The commit of a prepared recovered Galera transaction needs a valid trx->xid for invoking trx_sys_update_wsrep_checkpoint(). */ - if (wsrep_is_wsrep_xid(trx->xid)) break; + if (!wsrep_is_wsrep_xid(trx->xid)) #endif - /* Invalidate the XID, so that subsequent calls - will not find it. */ - trx->xid->null(); + /* Invalidate the XID, so that subsequent calls + will not find it. */ + trx->xid->null(); + trx_mutex_exit(trx); break; } + + trx_mutex_exit(trx); } return(trx); } -/*******************************************************************//** -This function is used to find one X/Open XA distributed transaction -which is in the prepared state -@return trx or NULL; on match, the trx->xid will be invalidated; -note that the trx may have been committed, unless the caller is -holding lock_sys->mutex */ -trx_t* -trx_get_trx_by_xid( -/*===============*/ - XID* xid) /*!< in: X/Open XA transaction identifier */ +/** Look up an X/Open distributed transaction in XA PREPARE state. +@param[in] xid X/Open XA transaction identifier +@return transaction on match (the trx_t::xid will be invalidated); +note that the trx may have been committed before the caller acquires +trx_t::mutex +@retval NULL if no match */ +trx_t* trx_get_trx_by_xid(const XID* xid) { trx_t* trx; @@ -2793,7 +2838,7 @@ trx_get_trx_by_xid( /* Recovered/Resurrected transactions are always only on the trx_sys_t::rw_trx_list. */ - trx = trx_get_trx_by_xid_low((XID*)xid); + trx = trx_get_trx_by_xid_low(xid); trx_sys_mutex_exit(); diff --git a/storage/innobase/trx/trx0undo.cc b/storage/innobase/trx/trx0undo.cc index 7ff6ae8fc3f..336506c7b65 100644 --- a/storage/innobase/trx/trx0undo.cc +++ b/storage/innobase/trx/trx0undo.cc @@ -1820,7 +1820,7 @@ trx_undo_free_prepared( TRX_STATE_COMMITTED_IN_MEMORY)); /* fall through */ case TRX_UNDO_ACTIVE: - /* lock_trx_release_locks() assigns + /* trx_t::commit_state() assigns trx->is_recovered=false and trx->state = TRX_STATE_COMMITTED_IN_MEMORY, also for transactions that we faked @@ -1852,7 +1852,7 @@ trx_undo_free_prepared( TRX_STATE_COMMITTED_IN_MEMORY)); /* fall through */ case TRX_UNDO_ACTIVE: - /* lock_trx_release_locks() assigns + /* trx_t::commit_state() assigns trx->is_recovered=false and trx->state = TRX_STATE_COMMITTED_IN_MEMORY, also for transactions that we faked From dae1b3b04c839b25233700c7ff7def578c545508 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Tue, 3 Sep 2019 13:04:05 +0300 Subject: [PATCH 10/14] MDEV-15326: Backport trx_t::is_referenced() Backport the applicable part of Sergey Vojtovich's commit 0ca2ea1a6550bb718efc878ed451be69e8244cd4 from MariaDB Server 10.3. trx reference counter was updated under mutex and read without any protection. This is both slow and unsafe. Use atomic operations for reference counter accesses. --- storage/innobase/include/row0vers.h | 2 +- storage/innobase/include/trx0sys.ic | 8 +++- storage/innobase/include/trx0trx.h | 62 +++++++++++++++++++---------- storage/innobase/include/trx0trx.ic | 17 -------- storage/innobase/lock/lock0lock.cc | 9 +++-- storage/innobase/row/row0sel.cc | 2 +- storage/innobase/row/row0vers.cc | 8 ++-- storage/innobase/trx/trx0trx.cc | 8 +--- 8 files changed, 60 insertions(+), 56 deletions(-) diff --git a/storage/innobase/include/row0vers.h b/storage/innobase/include/row0vers.h index b5279e5851b..327bb546356 100644 --- a/storage/innobase/include/row0vers.h +++ b/storage/innobase/include/row0vers.h @@ -44,7 +44,7 @@ index record. @param[in] index secondary index @param[in] offsets rec_get_offsets(rec, index) @return the active transaction; state must be rechecked after -trx_mutex_enter(), and trx_release_reference() must be invoked +trx_mutex_enter(), and trx->release_reference() must be invoked @retval NULL if the record was committed */ trx_t* row_vers_impl_x_locked( diff --git a/storage/innobase/include/trx0sys.ic b/storage/innobase/include/trx0sys.ic index ac84404a397..0a4d583671f 100644 --- a/storage/innobase/include/trx0sys.ic +++ b/storage/innobase/include/trx0sys.ic @@ -346,7 +346,6 @@ inline trx_t* trx_rw_is_active(trx_id_t trx_id, bool* corrupt, bool ref_count) mutex_enter(trx_mutex); ut_ad(!trx_state_eq(trx, TRX_STATE_NOT_STARTED)); ut_ad(trx->id == trx_id); - ut_ad(trx->n_ref >= 0); if (trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)) { /* We have an early state check here to avoid committer starvation in a wait loop for @@ -357,7 +356,12 @@ inline trx_t* trx_rw_is_active(trx_id_t trx_id, bool* corrupt, bool ref_count) rechecked by the caller after reacquiring the mutex. */ trx = NULL; } else { - ++trx->n_ref; + /* The reference could be safely incremented after + releasing one of trx_mutex or trx_sys->mutex. + Holding trx->mutex here may prevent a few false + references that could have a negative performance + impact on trx_commit_in_memory(). */ + trx->reference(); } mutex_exit(trx_mutex); } diff --git a/storage/innobase/include/trx0trx.h b/storage/innobase/include/trx0trx.h index 796e6b8bda6..1d71920d6c5 100644 --- a/storage/innobase/include/trx0trx.h +++ b/storage/innobase/include/trx0trx.h @@ -500,18 +500,6 @@ void trx_set_rw_mode( trx_t* trx); -/** -Release the transaction. Decrease the reference count. -@param trx Transaction that is being released */ -UNIV_INLINE -void -trx_release_reference( - trx_t* trx); - -/** -Check if the transaction is being referenced. */ -#define trx_is_referenced(t) ((t)->n_ref > 0) - /** Transactions that aren't started by the MySQL server don't set the trx_t::mysql_thd field. For such transactions we set the lock @@ -571,7 +559,7 @@ Check transaction state */ ut_ad(trx_state_eq((t), TRX_STATE_NOT_STARTED)); \ ut_ad(!(t)->id); \ ut_ad(!(t)->has_logged()); \ - ut_ad(!(t)->n_ref); \ + ut_ad(!(t)->is_referenced()); \ ut_ad(!MVCC::is_view_active((t)->read_view)); \ ut_ad((t)->lock.wait_thr == NULL); \ ut_ad(UT_LIST_GET_LEN((t)->lock.trx_locks) == 0); \ @@ -801,6 +789,19 @@ struct trx_rsegs_t { }; struct trx_t { +private: + /** + Count of references. + + We can't release the locks nor commit the transaction until this reference + is 0. We can change the state to TRX_STATE_COMMITTED_IN_MEMORY to signify + that it is no longer "active". + */ + + int32_t n_ref; + + +public: TrxMutex mutex; /*!< Mutex protecting the fields state and lock (except some fields of lock, which are protected by @@ -1100,14 +1101,6 @@ struct trx_t { const char* start_file; /*!< Filename where it was started */ #endif /* UNIV_DEBUG */ - lint n_ref; /*!< Count of references, protected - by trx_t::mutex. We can't release the - locks nor commit the transaction until - this reference is 0. We can change - the state to COMMITTED_IN_MEMORY to - signify that it is no longer - "active". */ - XID* xid; /*!< X/Open XA transaction identification to identify a transaction branch */ @@ -1184,6 +1177,33 @@ public: /** Release any explicit locks of a committing transaction. */ inline void release_locks(); + + bool is_referenced() + { + return my_atomic_load32_explicit(&n_ref, MY_MEMORY_ORDER_RELAXED) > 0; + } + + + void reference() + { +#ifdef UNIV_DEBUG + int32_t old_n_ref= +#endif + my_atomic_add32_explicit(&n_ref, 1, MY_MEMORY_ORDER_RELAXED); + ut_ad(old_n_ref >= 0); + } + + + void release_reference() + { +#ifdef UNIV_DEBUG + int32_t old_n_ref= +#endif + my_atomic_add32_explicit(&n_ref, -1, MY_MEMORY_ORDER_RELAXED); + ut_ad(old_n_ref > 0); + } + + private: /** Assign a rollback segment for modifying temporary tables. @return the assigned rollback segment */ diff --git a/storage/innobase/include/trx0trx.ic b/storage/innobase/include/trx0trx.ic index 64681e00a62..4a5b1ba717f 100644 --- a/storage/innobase/include/trx0trx.ic +++ b/storage/innobase/include/trx0trx.ic @@ -210,23 +210,6 @@ ok: trx->dict_operation = op; } -/** -Release the transaction. Decrease the reference count. -@param trx Transaction that is being released */ -UNIV_INLINE -void -trx_release_reference( - trx_t* trx) -{ - trx_mutex_enter(trx); - - ut_ad(trx->n_ref > 0); - --trx->n_ref; - - trx_mutex_exit(trx); -} - - /** @param trx Get the active view for this transaction, if one exists @return the transaction's read view or NULL if one not assigned. */ diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index 49c0e1da743..b7e17097546 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -5772,10 +5772,12 @@ lock_rec_convert_impl_to_expl_for_trx( trx_t* trx, /*!< in/out: active transaction */ ulint heap_no)/*!< in: rec heap number to lock */ { - DEBUG_SYNC_C("before_lock_rec_convert_impl_to_expl_for_trx"); + ut_ad(page_rec_is_leaf(rec)); + DEBUG_SYNC_C("before_lock_rec_convert_impl_to_expl_for_trx"); lock_mutex_enter(); trx_mutex_enter(trx); + ut_ad(trx->is_referenced()); ut_ad(!trx_state_eq(trx, TRX_STATE_NOT_STARTED)); if (!trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY) @@ -5786,9 +5788,8 @@ lock_rec_convert_impl_to_expl_for_trx( } lock_mutex_exit(); - ut_ad(trx->n_ref > 0); - --trx->n_ref; trx_mutex_exit(trx); + trx->release_reference(); DEBUG_SYNC_C("after_lock_rec_convert_impl_to_expl_for_trx"); } @@ -5830,7 +5831,7 @@ lock_rec_convert_impl_to_expl( if (trx != 0) { ulint heap_no = page_rec_get_heap_no(rec); - ut_ad(trx_is_referenced(trx)); + ut_ad(trx->is_referenced()); /* If the transaction is still active and has no explicit x-lock set on the record, set one for it. diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc index 19130576425..dae51354196 100644 --- a/storage/innobase/row/row0sel.cc +++ b/storage/innobase/row/row0sel.cc @@ -5087,7 +5087,7 @@ wrong_offs: rec, index, offsets)) { /* The record belongs to an active transaction. We must acquire a lock. */ - trx_release_reference(trx); + trx->release_reference(); } else { /* The secondary index record does not point to a delete-marked clustered index diff --git a/storage/innobase/row/row0vers.cc b/storage/innobase/row/row0vers.cc index 533d6df1321..03e586d4bee 100644 --- a/storage/innobase/row/row0vers.cc +++ b/storage/innobase/row/row0vers.cc @@ -77,7 +77,7 @@ index record. @param[in] offsets rec_get_offsets(rec, index) @param[in,out] mtr mini-transaction @return the active transaction; state must be rechecked after -trx_mutex_enter(), and trx_release_reference() must be invoked +trx_mutex_enter(), and trx->release_reference() must be invoked @retval NULL if the record was committed */ UNIV_INLINE trx_t* @@ -213,7 +213,7 @@ row_vers_impl_x_locked_low( created with a clear delete-mark flag. (We never insert a delete-marked record.) */ not_locked: - trx_release_reference(trx); + trx->release_reference(); trx = 0; } @@ -362,7 +362,7 @@ index record. @param[in] index secondary index @param[in] offsets rec_get_offsets(rec, index) @return the active transaction; state must be rechecked after -trx_mutex_enter(), and trx_release_reference() must be invoked +trx_mutex_enter(), and trx->release_reference() must be invoked @retval NULL if the record was committed */ trx_t* row_vers_impl_x_locked( @@ -408,7 +408,7 @@ row_vers_impl_x_locked( trx = row_vers_impl_x_locked_low( clust_rec, clust_index, rec, index, offsets, &mtr); - ut_ad(trx == 0 || trx_is_referenced(trx)); + ut_ad(trx == 0 || trx->is_referenced()); } mtr_commit(&mtr); diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc index 40dcc37b30b..1dcca7c1f72 100644 --- a/storage/innobase/trx/trx0trx.cc +++ b/storage/innobase/trx/trx0trx.cc @@ -551,7 +551,7 @@ inline void trx_t::commit_state() background thread. To avoid this race we unconditionally unset the is_recovered flag. */ is_recovered= false; - ut_ad(id || !trx_is_referenced(this)); + ut_ad(id || !is_referenced()); } /** Release any explicit locks of a committing transaction. */ @@ -1705,13 +1705,9 @@ trx_commit_in_memory( /* Wait for any implicit-to-explicit lock conversions to cease, so that there will be no race condition in lock_release(). */ - trx_mutex_enter(trx); - while (UNIV_UNLIKELY(trx_is_referenced(trx))) { - trx_mutex_exit(trx); + while (UNIV_UNLIKELY(trx->is_referenced())) { ut_delay(srv_spin_wait_delay); - trx_mutex_enter(trx); } - trx_mutex_exit(trx); trx->release_locks(); trx->id = 0; From 01e455dbb875e077b04eb6ac231714fac8319787 Mon Sep 17 00:00:00 2001 From: Monty Date: Wed, 4 Sep 2019 01:59:35 +0300 Subject: [PATCH 11/14] Fix of query cache bug in Aria MDEV-5817 query cache bug (returning inconsistent/old result set) with aria table parallel inserts, row format = page The problem is that for transactional aria tables (row_type=PAGE and transactional=1), maria_lock_database() didn't flush the state or the query cache. Not flushing the state is correct for transactional tables as this is done by checkpoint, but not flushing the query cache was wrong and could cause concurrent SELECT queries to not be deleted from the cache. Fixed by introducing a flush of the query cache as part of commit, if the table has changed. t for transactional aria tables (row_type=PAGE and transactional=1), maria_lock_table() didn't flush their state or the query cache. --- storage/maria/ha_maria.cc | 21 +++++++++++++++++++-- storage/maria/maria_def.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc index 51839aa5ff4..e82c76fd938 100644 --- a/storage/maria/ha_maria.cc +++ b/storage/maria/ha_maria.cc @@ -2859,7 +2859,20 @@ static void reset_thd_trn(THD *thd, MARIA_HA *first_table) THD_TRN= NULL; for (MARIA_HA *table= first_table; table ; table= table->trn_next) + { _ma_reset_trn_for_table(table); + + /* + If table has changed by this statement, invalidate it from the query + cache + */ + if (table->row_changes != table->start_row_changes) + { + table->start_row_changes= table->row_changes; + DBUG_ASSERT(table->s->chst_invalidator != NULL); + (*table->s->chst_invalidator)(table->s->data_file_name.str); + } + } DBUG_VOID_RETURN; } @@ -3325,7 +3338,10 @@ static int maria_commit(handlerton *hton __attribute__ ((unused)), THD *thd, bool all) { TRN *trn= THD_TRN; + int res; + MARIA_HA *used_instances= (MARIA_HA*) trn->used_instances; DBUG_ENTER("maria_commit"); + trnman_reset_locked_tables(trn, 0); trnman_set_flags(trn, trnman_get_flags(trn) & ~TRN_STATE_INFO_LOGGED); @@ -3333,8 +3349,9 @@ static int maria_commit(handlerton *hton __attribute__ ((unused)), if ((thd->variables.option_bits & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) && !all) DBUG_RETURN(0); // end of statement - reset_thd_trn(thd, (MARIA_HA*) trn->used_instances); - DBUG_RETURN(ma_commit(trn)); // end of transaction + res= ma_commit(trn); + reset_thd_trn(thd, used_instances); + DBUG_RETURN(res); } diff --git a/storage/maria/maria_def.h b/storage/maria/maria_def.h index a23e00a24a3..cba35aabeb3 100644 --- a/storage/maria/maria_def.h +++ b/storage/maria/maria_def.h @@ -640,6 +640,7 @@ struct st_maria_handler invalidator_by_filename invalidator; /* query cache invalidator */ ulonglong last_auto_increment; /* auto value at start of statement */ ulonglong row_changes; /* Incremented for each change */ + ulonglong start_row_changes; /* Row changes since start trans */ ulong this_unique; /* uniq filenumber or thread */ ulong last_unique; /* last unique number */ ulong this_loop; /* counter for this open */ From 53ec9047c91c66644799ad058e998a7cfe1afef0 Mon Sep 17 00:00:00 2001 From: Sachin Date: Sat, 27 Jul 2019 19:57:46 +0530 Subject: [PATCH 12/14] MDEV-20137 rpl.mdev_17588 fails in buildbot with "Table doesn't exist" Fix the test case. --- mysql-test/suite/rpl/disabled.def | 1 - mysql-test/suite/rpl/r/mdev_17588.result | 14 ++++++-------- mysql-test/suite/rpl/t/mdev_17588.test | 23 ++++++++++++++--------- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/mysql-test/suite/rpl/disabled.def b/mysql-test/suite/rpl/disabled.def index 9d0a166f6dc..bdefb1660bd 100644 --- a/mysql-test/suite/rpl/disabled.def +++ b/mysql-test/suite/rpl/disabled.def @@ -15,5 +15,4 @@ rpl_get_master_version_and_clock : Bug#11766137 Jan 05 2011 joro Valgrind warnin rpl_partition_archive : MDEV-5077 2013-09-27 svoj Cannot exchange partition with archive table rpl_row_binlog_max_cache_size : MDEV-11092 rpl_row_index_choice : MDEV-11666 -mdev_17588: MDEV-20137 rpl_slave_grp_exec: MDEV-10514 diff --git a/mysql-test/suite/rpl/r/mdev_17588.result b/mysql-test/suite/rpl/r/mdev_17588.result index f6b4728bd88..54990a450e3 100644 --- a/mysql-test/suite/rpl/r/mdev_17588.result +++ b/mysql-test/suite/rpl/r/mdev_17588.result @@ -1,18 +1,16 @@ include/master-slave.inc [connection master] +connection slave; +include/stop_slave.inc +CHANGE MASTER TO master_use_gtid=slave_pos; +include/start_slave.inc connection master; create table t1 (a int) engine=innodb; create table t2 (a int); create table t3 (a int) engine=innodb; -include/save_master_gtid.inc connection slave; -include/wait_for_slave_sql_error.inc [errno=1286] -Last_Error = 'Error 'Unknown storage engine 'innodb'' on query. Default database: 'test'. Query: 'create table t1 (a int) engine=innodb'' -STOP SLAVE IO_THREAD; -include/wait_for_slave_to_stop.inc -SET GLOBAL SQL_SLAVE_SKIP_COUNTER=1; -include/start_slave.inc -include/sync_with_master_gtid.inc +include/wait_for_slave_sql_error_and_skip.inc [errno=1286] +# Asserted this: Status should be 'Slave has read all relay log...' show tables; Tables_in_test t2 diff --git a/mysql-test/suite/rpl/t/mdev_17588.test b/mysql-test/suite/rpl/t/mdev_17588.test index e9a340cbd25..44644158dff 100644 --- a/mysql-test/suite/rpl/t/mdev_17588.test +++ b/mysql-test/suite/rpl/t/mdev_17588.test @@ -1,23 +1,28 @@ --source include/master-slave.inc --source include/have_innodb.inc +--connection slave +--source include/stop_slave.inc +CHANGE MASTER TO master_use_gtid=slave_pos; +--source include/start_slave.inc + --connection master create table t1 (a int) engine=innodb; create table t2 (a int); create table t3 (a int) engine=innodb; ---source include/save_master_gtid.inc +--save_master_pos --connection slave # Using ER_UNKNOWN_STORAGE_ENGINE wont work let $slave_sql_errno= 1286; ---source include/wait_for_slave_sql_error.inc ---let $status_items= Last_Error ---source include/show_slave_status.inc -STOP SLAVE IO_THREAD; -source include/wait_for_slave_to_stop.inc; -SET GLOBAL SQL_SLAVE_SKIP_COUNTER=1; ---source include/start_slave.inc ---source include/sync_with_master_gtid.inc +--source include/wait_for_slave_sql_error_and_skip.inc +--sync_with_master + +--let $assert_text= Status should be 'Slave has read all relay log...' +--let $assert_cond= "[SHOW SLAVE STATUS, Slave_SQL_Running_State, 1]" +#Like "Slave has read all relay log%" +--source include/rpl_assert.inc + show tables; show create table t2; --error ER_NO_SUCH_TABLE From 8dca4cf53ff9d738d39730014d79205d6fd014fd Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Wed, 4 Sep 2019 14:02:01 +0200 Subject: [PATCH 13/14] MDEV-20403 Assertion `0' or Assertion `btr_validate_index(index, 0)' failed in row_upd_sec_index_entry or error code 126: Index is corrupted upon UPDATE with TIMESTAMP..ON UPDATE remove a special treatment of a bare DEFAULT keyword that made it behave inconsistently and differently from DEFAULT(column). Now all forms of the explicit assignment of a default column value behave identically, and all count as an explicitly assigned value (for the purpose of ON UPDATE NOW). followup for c7c481f4d91 --- mysql-test/r/default.result | 2 +- mysql-test/r/function_defaults.result | 29 +++++++++++++++++++++++++++ mysql-test/t/function_defaults.test | 25 +++++++++++++++++++++-- sql/field.cc | 27 ------------------------- sql/field.h | 1 - sql/item.cc | 4 +--- sql/sql_base.cc | 8 ++------ 7 files changed, 56 insertions(+), 40 deletions(-) diff --git a/mysql-test/r/default.result b/mysql-test/r/default.result index c2de5a578f1..f4ef9fcc8c8 100644 --- a/mysql-test/r/default.result +++ b/mysql-test/r/default.result @@ -3372,7 +3372,7 @@ insert into t1 values (b, 5, '5 the value of the DEFAULT(a), that is b'); select * from t1 order by t; a b t 5 5 1 column is omitted -5 5 2 column gets DEFAULT, keyword +4 5 2 column gets DEFAULT, keyword 4 5 3 column gets DEFAULT(a), expression 4 5 4 also expression DEFAULT(0)+0 4 5 5 the value of the DEFAULT(a), that is b diff --git a/mysql-test/r/function_defaults.result b/mysql-test/r/function_defaults.result index 1c63ae17a5d..4a8f64df352 100644 --- a/mysql-test/r/function_defaults.result +++ b/mysql-test/r/function_defaults.result @@ -3116,3 +3116,32 @@ select if(@new = j, 'correct', 'wrong') from t1; if(@new = j, 'correct', 'wrong') correct drop table t1; +create table t1 (a int, b varchar(20) default 'foo'); +insert t1 values (1,'bla'),(2, 'bar'); +select * from t1; +a b +1 bla +2 bar +update t1 set b=default where a=1; +select * from t1; +a b +1 foo +2 bar +drop table t1; +create table t1 ( +a int, +b timestamp default '2010-10-10 10:10:10' on update now(), +c varchar(100) default 'x'); +insert t1 (a) values (1),(2); +select * from t1; +a b c +1 2010-10-10 10:10:10 x +2 2010-10-10 10:10:10 x +set timestamp=unix_timestamp('2011-11-11 11-11-11'); +update t1 set b=default, c=default(b) where a=1; +select * from t1; +a b c +1 2010-10-10 10:10:10 2010-10-10 10:10:10 +2 2010-10-10 10:10:10 x +drop table t1; +set timestamp=default; diff --git a/mysql-test/t/function_defaults.test b/mysql-test/t/function_defaults.test index 1e3e86599e3..dd3ba109b2a 100644 --- a/mysql-test/t/function_defaults.test +++ b/mysql-test/t/function_defaults.test @@ -24,14 +24,14 @@ source 'include/function_defaults.inc'; # MDEV-20403 Assertion `0' or Assertion `btr_validate_index(index, 0)' failed in row_upd_sec_index_entry or error code 126: Index is corrupted upon UPDATE with TIMESTAMP..ON UPDATE # -# ON UPDATE DEFAULT NOW and indexed virtual columns +# ON UPDATE NOW and indexed virtual columns create table t1 (t timestamp, i int, v timestamp as (t) virtual, key(v)); insert t1 (t,i) values ('2006-03-01 23:59:59',1); update t1 set i = 2; check table t1; drop table t1; -# ON UPDATE DEFAULT NOW and triggers +# ON UPDATE NOW and triggers create table t1 (t timestamp, i int); create trigger tr1 before update on t1 for each row set @new:=new.t; insert t1 (t,i) values ('2006-03-01 23:59:59', 1); @@ -46,3 +46,24 @@ insert t1 (i) values (1); update t1, t1 as t2 set t1.i = 2; select if(@new = j, 'correct', 'wrong') from t1; drop table t1; + +# SET xxx=DEFAULT +create table t1 (a int, b varchar(20) default 'foo'); +insert t1 values (1,'bla'),(2, 'bar'); +select * from t1; +update t1 set b=default where a=1; +select * from t1; +drop table t1; + +# ON UPDATE NOW and SET xxx=DEFAULT +create table t1 ( + a int, + b timestamp default '2010-10-10 10:10:10' on update now(), + c varchar(100) default 'x'); +insert t1 (a) values (1),(2); +select * from t1; +set timestamp=unix_timestamp('2011-11-11 11-11-11'); +update t1 set b=default, c=default(b) where a=1; +select * from t1; +drop table t1; +set timestamp=default; diff --git a/sql/field.cc b/sql/field.cc index 30edee0e386..2b1ba0e1372 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -11106,33 +11106,6 @@ key_map Field::get_possible_keys() } -/** - Mark the field as having an explicit default value. - - @param value if available, the value that the field is being set to - - @note - Fields that have an explicit default value should not be updated - automatically via the DEFAULT or ON UPDATE functions. The functions - that deal with data change functionality (INSERT/UPDATE/LOAD), - determine if there is an explicit value for each field before performing - the data change, and call this method to mark the field. - - If the 'value' parameter is NULL, then the field is marked unconditionally - as having an explicit value. If 'value' is not NULL, then it can be further - analyzed to check if it really should count as a value. -*/ - -bool Field::set_explicit_default(Item *value) -{ - if (value->type() == Item::DEFAULT_VALUE_ITEM && - !((Item_default_value*)value)->arg) - return false; - set_has_explicit_value(); - return true; -} - - bool Field::validate_value_in_record_with_warn(THD *thd, const uchar *record) { my_bitmap_map *old_map= dbug_tmp_use_all_columns(table, table->read_set); diff --git a/sql/field.h b/sql/field.h index 73c36affce0..19a716cfd5d 100644 --- a/sql/field.h +++ b/sql/field.h @@ -990,7 +990,6 @@ public: { return bitmap_is_set(&table->has_value_set, field_index); } - bool set_explicit_default(Item *value); virtual bool binary() const { return 1; } virtual bool zero_pack() const { return 1; } diff --git a/sql/item.cc b/sql/item.cc index 58b00e41fb1..333d71ddf70 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -9010,8 +9010,6 @@ int Item_default_value::save_in_field(Field *field_arg, bool no_conversions) return Item_field::save_in_field(field_arg, no_conversions); } - if (field_arg->default_value && field_arg->default_value->flags) - return 0; // defaut fields will be set later, no need to do it twice return field_arg->save_in_field_default_value(context->error_processor == &view_error_processor); } @@ -9263,7 +9261,7 @@ bool Item_trigger_field::set_value(THD *thd, sp_rcontext * /*ctx*/, Item **it) int err_code= item->save_in_field(field, 0); field->table->copy_blobs= copy_blobs_saved; - field->set_explicit_default(item); + field->set_has_explicit_value(); return err_code < 0; } diff --git a/sql/sql_base.cc b/sql/sql_base.cc index f7f7d50bc85..5e34958bf59 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8068,7 +8068,7 @@ fill_record(THD *thd, TABLE *table_arg, List &fields, List &values, my_message(ER_UNKNOWN_ERROR, ER_THD(thd, ER_UNKNOWN_ERROR), MYF(0)); goto err; } - rfield->set_explicit_default(value); + rfield->set_has_explicit_value(); } if (update) @@ -8265,7 +8265,6 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List &values, { List_iterator_fast v(values); List tbl_list; - bool all_fields_have_values= true; Item *value; Field *field; bool abort_on_warning_saved= thd->abort_on_warning; @@ -8318,11 +8317,8 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List &values, else if (value->save_in_field(field, 0) < 0) goto err; - all_fields_have_values &= field->set_explicit_default(value); + field->set_has_explicit_value(); } - if (!all_fields_have_values && table->default_field && - table->update_default_fields(ignore_errors)) - goto err; /* Update virtual fields */ thd->abort_on_warning= FALSE; if (table->vfield && From f605ce08b5c0e6ed6907d0639bcc5b5630e9b40a Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Wed, 4 Sep 2019 14:20:37 +0200 Subject: [PATCH 14/14] more tests for DEFAULT and DEFAULT(column) in INSERT this is not ideal and needs to be fixed eventually, but it's consistent over all forms of INSERT. --- mysql-test/r/default.result | 18 +++++++++++++----- mysql-test/t/default.test | 15 ++++++++++----- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/mysql-test/r/default.result b/mysql-test/r/default.result index f4ef9fcc8c8..8335b553a8e 100644 --- a/mysql-test/r/default.result +++ b/mysql-test/r/default.result @@ -3364,11 +3364,15 @@ a b drop table t1; set sql_mode=default; create table t1 (a int default b, b int default 4, t text); -insert into t1 (b, t) values (5, '1 column is omitted'); -insert into t1 values (default, 5, '2 column gets DEFAULT, keyword'); -insert into t1 values (default(a), 5, '3 column gets DEFAULT(a), expression'); -insert into t1 values (default(a)+0, 5, '4 also expression DEFAULT(0)+0'); -insert into t1 values (b, 5, '5 the value of the DEFAULT(a), that is b'); +insert t1 (b, t) values (5, '1 column is omitted'); +insert t1 values (default, 5, '2 column gets DEFAULT, keyword'); +insert t1 values (default(a), 5, '3 column gets DEFAULT(a), expression'); +insert t1 values (default(a)+0, 5, '4 also expression DEFAULT(0)+0'); +insert t1 values (b, 5, '5 the value of the DEFAULT(a), that is b'); +insert t1 (t,b,a) values ('6 reversed, column gets DEFAULT, keyword', 5, default); +insert t1 (t,b,a) values ('7 reversed, column gets DEFAULT(a), expression', 5, default(a)); +insert t1 (t,b,a) values ('8 reversed, also expression DEFAULT(0)+0', 5, default(a)+0); +insert t1 (t,b,a) values ('9 reversed, the value of the DEFAULT(a), that is b', 5, b); select * from t1 order by t; a b t 5 5 1 column is omitted @@ -3376,6 +3380,10 @@ a b t 4 5 3 column gets DEFAULT(a), expression 4 5 4 also expression DEFAULT(0)+0 4 5 5 the value of the DEFAULT(a), that is b +5 5 6 reversed, column gets DEFAULT, keyword +5 5 7 reversed, column gets DEFAULT(a), expression +5 5 8 reversed, also expression DEFAULT(0)+0 +5 5 9 reversed, the value of the DEFAULT(a), that is b drop table t1; create table t1 (col1 int default(-(default(col1)))); ERROR 01000: Expression for field `col1` is referring to uninitialized field `col1` diff --git a/mysql-test/t/default.test b/mysql-test/t/default.test index bfea214f548..31cf589c487 100644 --- a/mysql-test/t/default.test +++ b/mysql-test/t/default.test @@ -2075,11 +2075,16 @@ set sql_mode=default; # MDEV-10201 Bad results for CREATE TABLE t1 (a INT DEFAULT b, b INT DEFAULT 4) # create table t1 (a int default b, b int default 4, t text); -insert into t1 (b, t) values (5, '1 column is omitted'); -insert into t1 values (default, 5, '2 column gets DEFAULT, keyword'); -insert into t1 values (default(a), 5, '3 column gets DEFAULT(a), expression'); -insert into t1 values (default(a)+0, 5, '4 also expression DEFAULT(0)+0'); -insert into t1 values (b, 5, '5 the value of the DEFAULT(a), that is b'); +insert t1 (b, t) values (5, '1 column is omitted'); +insert t1 values (default, 5, '2 column gets DEFAULT, keyword'); +insert t1 values (default(a), 5, '3 column gets DEFAULT(a), expression'); +insert t1 values (default(a)+0, 5, '4 also expression DEFAULT(0)+0'); +insert t1 values (b, 5, '5 the value of the DEFAULT(a), that is b'); +# and the same in a different order +insert t1 (t,b,a) values ('6 reversed, column gets DEFAULT, keyword', 5, default); +insert t1 (t,b,a) values ('7 reversed, column gets DEFAULT(a), expression', 5, default(a)); +insert t1 (t,b,a) values ('8 reversed, also expression DEFAULT(0)+0', 5, default(a)+0); +insert t1 (t,b,a) values ('9 reversed, the value of the DEFAULT(a), that is b', 5, b); select * from t1 order by t; drop table t1;