From 5392b4a32c9da296f7b2e6a5b83e3bff6c24871e Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Wed, 30 Oct 2019 19:31:26 +0100 Subject: [PATCH 1/2] MDEV-20354 All but last insert ignored in InnoDB tables when table locked mysql_insert() first opens all affected tables (which implicitly starts a transaction in InnoDB), then stat tables. A failure to open a stat table caused open_tables() to abort the current stmt transaction (trans_rollback_stmt()). So, from the server point of view the following ha_write_row()-s happened outside of a transactions, and the server didn't bother to commit them. The server has a mechanism to prevent a transaction being unexpectedly committed or rolled back in the middle of a statement - if an operation takes place _in a sub-statement_ it cannot change the transaction state. Operations on stat tables are exactly that - they are not allowed to change a transaction state. Put them in a sub-statement to make sure they don't. --- mysql-test/suite/innodb/r/stat_tables.result | 15 +++++++++++++++ mysql-test/suite/innodb/t/stat_tables.test | 17 +++++++++++++++++ sql/sql_class.h | 4 ++++ sql/sql_statistics.cc | 2 ++ 4 files changed, 38 insertions(+) create mode 100644 mysql-test/suite/innodb/r/stat_tables.result create mode 100644 mysql-test/suite/innodb/t/stat_tables.test diff --git a/mysql-test/suite/innodb/r/stat_tables.result b/mysql-test/suite/innodb/r/stat_tables.result new file mode 100644 index 00000000000..bb449570479 --- /dev/null +++ b/mysql-test/suite/innodb/r/stat_tables.result @@ -0,0 +1,15 @@ +rename table mysql.table_stats to mysql.table_stats_save; +flush tables; +set use_stat_tables= PREFERABLY; +create table t1 (a int) engine=InnoDB; +start transaction; +insert t1 values (1); +insert t1 values (2); +commit; +select * from t1; +a +1 +2 +drop table t1; +rename table mysql.table_stats_save to mysql.table_stats; +flush tables; diff --git a/mysql-test/suite/innodb/t/stat_tables.test b/mysql-test/suite/innodb/t/stat_tables.test new file mode 100644 index 00000000000..68344b3f425 --- /dev/null +++ b/mysql-test/suite/innodb/t/stat_tables.test @@ -0,0 +1,17 @@ +source include/have_innodb.inc; + +# +# MDEV-20354 All but last insert ignored in InnoDB tables when table locked +# +rename table mysql.table_stats to mysql.table_stats_save; +flush tables; +set use_stat_tables= PREFERABLY; +create table t1 (a int) engine=InnoDB; +start transaction; +insert t1 values (1); +insert t1 values (2); +commit; +select * from t1; +drop table t1; +rename table mysql.table_stats_save to mysql.table_stats; +flush tables; diff --git a/sql/sql_class.h b/sql/sql_class.h index 6c622648ac7..56bb9f45877 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1444,12 +1444,16 @@ public: /** @class Sub_statement_state @brief Used to save context when executing a function or trigger + + operations on stat tables aren't technically a sub-statement, but they are + similar in a sense that they cannot change the transaction status. */ /* Defines used for Sub_statement_state::in_sub_stmt */ #define SUB_STMT_TRIGGER 1 #define SUB_STMT_FUNCTION 2 +#define SUB_STMT_STAT_TABLES 4 class Sub_statement_state diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index 4a1ed9cde4e..f002a5a5eab 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -268,7 +268,9 @@ inline int open_stat_tables(THD *thd, TABLE_LIST *tables, thd->push_internal_handler(&deh); init_table_list_for_stat_tables(tables, for_write); init_mdl_requests(tables); + thd->in_sub_stmt|= SUB_STMT_STAT_TABLES; rc= open_system_tables_for_read(thd, tables, backup); + thd->in_sub_stmt&= ~SUB_STMT_STAT_TABLES; thd->pop_internal_handler(); From 313855766f3ce608001d686553ef6e087a70d264 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Wed, 30 Oct 2019 19:42:16 +0100 Subject: [PATCH 2/2] cleanup * use compile_time_assert instead of DBUG_ASSERT * don't use thd->clear_error(), because * the error was already consumed by the error handler, so there is nothing to clear * it's dangerous to clear errors indiscriminately, if the error came from outside of read_statistics_for_tables() it must not be cleared --- sql/sql_statistics.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index f002a5a5eab..11e2db9ae44 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -257,10 +257,8 @@ index_stat_def= {INDEX_STAT_N_FIELDS, index_stat_fields, 4, index_stat_pk_col}; Open all statistical tables and lock them */ -static -inline int open_stat_tables(THD *thd, TABLE_LIST *tables, - Open_tables_backup *backup, - bool for_write) +static int open_stat_tables(THD *thd, TABLE_LIST *tables, + Open_tables_backup *backup, bool for_write) { int rc; @@ -275,7 +273,7 @@ inline int open_stat_tables(THD *thd, TABLE_LIST *tables, /* If the number of tables changes, we should revise the check below. */ - DBUG_ASSERT(STATISTICS_TABLES == 3); + compile_time_assert(STATISTICS_TABLES == 3); if (!rc && (stat_table_intact.check(tables[TABLE_STAT].table, &table_stat_def) || @@ -3258,10 +3256,7 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) DBUG_RETURN(0); if (open_stat_tables(thd, stat_tables, &open_tables_backup, FALSE)) - { - thd->clear_error(); DBUG_RETURN(1); - } for (TABLE_LIST *tl= tables; tl; tl= tl->next_global) {