From e54850db442c3e1b4dccac4e9638c4209f23c22e Mon Sep 17 00:00:00 2001 From: Kamil Holubicki Date: Thu, 15 Apr 2021 09:04:12 +0200 Subject: [PATCH] PS-7593: transaction-isolation level is not honored when changed at session level https://jira.percona.com/browse/PS-7593 Problem: 1. Conditions: log_bin = ON, transaction_isolation = READ-COMMITTED, binlog_format = ROW 2. Create two tables and execute REPLACE INTO ... SELECT 3. Change transaction_isolation to REPEATABLE-READ and binlog_format to STATEMENT 4. Execute REPLACE INTO ... SELECT again 5. Error informing that it is not possible to execute query is reported. Cause: 1. When the table is created and then accessed, it is cached in thd's table cache. It contains, inter alia, handler::cached_table_flags which is calculated upon transaction_isolation value. 2. After transaction_isolation and binlog_format change open_tables_for_query() and then sql_base.cc::open_table() finds table in Table_cache 2. As the table was cached with READ-COMMITTED it has HA_BINLOG_STMT_CAPABLE flag not set (handler::cached_table_flags) 3. Sql_cmd_insert_select::execute() and then handle_query() calls select->prepare(thd) BEFORE call to lock_tables() 4. Commit e8cc22afb14914e56e535cb3e61a2b4310033d0e added call to thd->decide_logging_format(). Decision is made upon cached value of HA_BINLOG_STMT_CAPABLE flag (not set). This leads to immediate error as the combination of READ-COMMITED and binlog_format STATEMENT is not allowed. 5. lock_tables is called which would cause HA_BINLOG_STMT_CAPABLE flag refresh (handler::cached_table_flags), but it is too late. Solution: Force recalculation of handler::cached_table_flags if they are used before locking the table (call introduced in above mentioned regression commit) --- .../r/transaction_isolation_change.result | 24 +++++++++++ .../t/transaction_isolation_change-master.opt | 1 + .../t/transaction_isolation_change.test | 42 +++++++++++++++++++ sql/binlog.cc | 7 +++- sql/handler.h | 15 ++++++- sql/sql_class.h | 2 +- sql/sql_insert.cc | 2 +- 7 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 mysql-test/r/transaction_isolation_change.result create mode 100644 mysql-test/t/transaction_isolation_change-master.opt create mode 100644 mysql-test/t/transaction_isolation_change.test diff --git a/mysql-test/r/transaction_isolation_change.result b/mysql-test/r/transaction_isolation_change.result new file mode 100644 index 00000000000..0da274ca317 --- /dev/null +++ b/mysql-test/r/transaction_isolation_change.result @@ -0,0 +1,24 @@ +SET @saved_transaction_isolation = @@session.transaction_isolation; +SET @saved_binlog_format = @@session.binlog_format; +SET @@session.transaction_isolation = "READ-COMMITTED"; +SET @@session.binlog_format = ROW; +CREATE TABLE t1 (a INT); +CREATE TABLE t2 (a INT); +REPLACE INTO t1(a) SELECT a FROM t2; +SET @@session.transaction_isolation = "REPEATABLE-READ"; +SET @@session.binlog_format = STATEMENT; +REPLACE INTO t1(a) SELECT a FROM t2; +Warnings: +Note 1592 Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT. REPLACE... SELECT is unsafe because the order in which rows are retrieved by the SELECT determines which (if any) rows are replaced. This order cannot be predicted and may differ on master and the slave. +DROP TABLE t1, t2; +CREATE TABLE t1 (a INT); +CREATE TABLE t2 (a INT); +SET @@session.binlog_format = STATEMENT; +SET @@session.transaction_isolation = "READ-UNCOMMITTED"; +SELECT a FROM t1; +a +SET @@session.transaction_isolation = "SERIALIZABLE"; +INSERT INTO t1(a) SELECT a FROM t2; +DROP TABLE t1, t2; +SET @@session.transaction_isolation = @saved_transaction_isolation; +SET @@session.binlog_format = @saved_binlog_format; diff --git a/mysql-test/t/transaction_isolation_change-master.opt b/mysql-test/t/transaction_isolation_change-master.opt new file mode 100644 index 00000000000..beae84b3862 --- /dev/null +++ b/mysql-test/t/transaction_isolation_change-master.opt @@ -0,0 +1 @@ +--log-bin diff --git a/mysql-test/t/transaction_isolation_change.test b/mysql-test/t/transaction_isolation_change.test new file mode 100644 index 00000000000..1d0351e840a --- /dev/null +++ b/mysql-test/t/transaction_isolation_change.test @@ -0,0 +1,42 @@ +SET @saved_transaction_isolation = @@session.transaction_isolation; +SET @saved_binlog_format = @@session.binlog_format; + +# +# READ-COMMITTED -> REPEATABLE-READ +# +SET @@session.transaction_isolation = "READ-COMMITTED"; +SET @@session.binlog_format = ROW; + +CREATE TABLE t1 (a INT); +CREATE TABLE t2 (a INT); +REPLACE INTO t1(a) SELECT a FROM t2; + +SET @@session.transaction_isolation = "REPEATABLE-READ"; +SET @@session.binlog_format = STATEMENT; + +REPLACE INTO t1(a) SELECT a FROM t2; + +DROP TABLE t1, t2; + + +# +# READ-UNCOMMITTED -> SERIALIZABLE +# +CREATE TABLE t1 (a INT); +CREATE TABLE t2 (a INT); + +SET @@session.binlog_format = STATEMENT; +SET @@session.transaction_isolation = "READ-UNCOMMITTED"; +SELECT a FROM t1; +SET @@session.transaction_isolation = "SERIALIZABLE"; +INSERT INTO t1(a) SELECT a FROM t2; + + +--disable_query_log +CALL mtr.add_suppression(".*Unsafe statement written to the binary log using statement format.*"); +--enable_query_log + +# cleanup +DROP TABLE t1, t2; +SET @@session.transaction_isolation = @saved_transaction_isolation; +SET @@session.binlog_format = @saved_binlog_format; diff --git a/sql/binlog.cc b/sql/binlog.cc index 1d9395bf57f..dbdb26c8927 100644 --- a/sql/binlog.cc +++ b/sql/binlog.cc @@ -11524,12 +11524,15 @@ const char * get_locked_tables_mode_name(enum_locked_tables_mode locked_tables_m @param[in] thd Client thread @param[in] tables Tables involved in the query + @param[in] use_cached_table_flags use cached value of + handler::cached_table_flags. Do not use cached value and force recalculation + in case of 'false'. @retval 0 No error; statement can be logged. @retval -1 One of the error conditions above applies (1, 2, 4, 5, 6 or 9). */ -int THD::decide_logging_format(TABLE_LIST *tables) +int THD::decide_logging_format(TABLE_LIST *tables, bool use_cached_table_flags) { DBUG_ENTER("THD::decide_logging_format"); DBUG_PRINT("info", ("query: %s", query().str)); @@ -11687,7 +11690,7 @@ int THD::decide_logging_format(TABLE_LIST *tables) continue; } - handler::Table_flags const flags= table->table->file->ha_table_flags(); + handler::Table_flags const flags= table->table->file->ha_table_flags(!use_cached_table_flags); DBUG_PRINT("info", ("table: %s; ha_table_flags: 0x%llx", table->table_name, flags)); diff --git a/sql/handler.h b/sql/handler.h index 92f30d32e02..0c343cf4f43 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -2317,7 +2317,7 @@ class handler :public Sql_alloc protected: TABLE_SHARE *table_share; /* The table definition */ TABLE *table; /* The current open table */ - Table_flags cached_table_flags; /* Set on init() and open() */ + mutable Table_flags cached_table_flags; /* Set on init() and open() */ ha_rows estimation_rows_to_insert; public: @@ -2597,8 +2597,19 @@ class handler :public Sql_alloc } /** The cached_table_flags is set at ha_open and ha_external_lock + + @param recalculate[in] Force flags recalculation instead of using cached + value. + + @retval table flags */ - Table_flags ha_table_flags() const { return cached_table_flags; } + Table_flags ha_table_flags(bool recalculate = false) const + { + if (recalculate) + cached_table_flags= table_flags(); + return cached_table_flags; + } + /** These functions represent the public interface to *users* of the handler class, hence they are *not* virtual. For the inheritance diff --git a/sql/sql_class.h b/sql/sql_class.h index 3442a6e4497..2fda577a849 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -4868,7 +4868,7 @@ class THD :public MDL_context_owner, locked_tables_mode= mode_arg; } void leave_locked_tables_mode(); - int decide_logging_format(TABLE_LIST *tables); + int decide_logging_format(TABLE_LIST *tables, bool use_cached_table_flags= true); /** is_dml_gtid_compatible() and is_ddl_gtid_compatible() check if the statement that is about to be processed will safely get a diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 4cb8a8245b6..b2f48a2327c 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -2197,7 +2197,7 @@ int Query_result_insert::prepare(List &values, SELECT_LEX_UNIT *u) table->file->extra(HA_EXTRA_INSERT_WITH_UPDATE); /* Decide the logging format prior to preparing table/record metadata */ - res= res || thd->decide_logging_format(table_list); + res= res || thd->decide_logging_format(table_list, false); if (!res) { prepare_triggers_for_insert_stmt(table);