From 2f3779d31cbbabcb171c22237253c82146118446 Mon Sep 17 00:00:00 2001 From: Monty Date: Sun, 20 May 2018 14:19:14 +0300 Subject: Fixes for Aria transaction handling with lock tables MDEV-10130 Assertion `share->in_trans == 0' failed in storage/maria/ma_close.c MDEV-10378 Assertion `trn' failed in virtual int ha_maria::start_stmt The problem was that maria_handler->trn was not properly reset at commit/rollback and ha_maria::exernal_lock() could get confused because. There was some old code in ha_maria::implicit_commit() that tried to take care of this, but it was not bullet proof. Fixed by adding list of all tables that is part of the maria transaction to TRN. A nice side effect was of the fix is that loops in ha_maria::implict_commit() got to be much simpler. Other things: - Fixed a bug in mysql_admin_table() where argument open_for_modify was wrongly reset for the next table in the chain - rollback admin command also in case of fatal error. - Split _ma_set_trn_for_table() to three version to simplify code and debugging. - Several new asserts to detect the original problem (that file was not properly removed from trn before calling ma_close()) --- sql/sql_admin.cc | 12 ++++++------ sql/sql_handler.cc | 4 +++- 2 files changed, 9 insertions(+), 7 deletions(-) (limited to 'sql') diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc index 4fe51a2189e..10fcbd56e18 100644 --- a/sql/sql_admin.cc +++ b/sql/sql_admin.cc @@ -302,7 +302,7 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, HA_CHECK_OPT* check_opt, const char *operator_name, thr_lock_type lock_type, - bool open_for_modify, + bool org_open_for_modify, bool repair_table_use_frm, uint extra_open_options, int (*prepare_func)(THD *, TABLE_LIST *, @@ -359,10 +359,10 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, for (table= tables; table; table= table->next_local) { char table_name[SAFE_NAME_LEN*2+2]; - char* db = table->db; + char *db= table->db; bool fatal_error=0; bool open_error; - + bool open_for_modify= org_open_for_modify; DBUG_PRINT("admin", ("table: '%s'.'%s'", table->db, table->table_name)); strxmov(table_name, db, ".", table->table_name, NullS); thd->open_options|= extra_open_options; @@ -395,8 +395,8 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, /* CHECK TABLE command is allowed for views as well. Check on alter flags - to differentiate from ALTER TABLE...CHECK PARTITION on which view is not - allowed. + to differentiate from ALTER TABLE...CHECK PARTITION on which view is + not allowed. */ if (lex->alter_info.flags & Alter_info::ALTER_ADMIN_PARTITION || view_operator_func == NULL) @@ -1053,7 +1053,7 @@ send_result_message: } } /* Error path, a admin command failed. */ - if (thd->transaction_rollback_request) + if (thd->transaction_rollback_request || fatal_error) { /* Unlikely, but transaction rollback was requested by one of storage diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc index 5fc7c20d409..52ce42e2e6a 100644 --- a/sql/sql_handler.cc +++ b/sql/sql_handler.cc @@ -155,10 +155,11 @@ static void mysql_ha_close_table(SQL_HANDLER *handler) { THD *thd= handler->thd; TABLE *table= handler->table; + DBUG_ENTER("mysql_ha_close_table"); /* check if table was already closed */ if (!table) - return; + DBUG_VOID_RETURN; if (!table->s->tmp_table) { @@ -184,6 +185,7 @@ static void mysql_ha_close_table(SQL_HANDLER *handler) } my_free(handler->lock); handler->init(); + DBUG_VOID_RETURN; } /* -- cgit v1.2.1 From da71c1bad79ee11009b3b28a2a745709e04020cf Mon Sep 17 00:00:00 2001 From: Monty Date: Tue, 22 May 2018 14:36:06 +0300 Subject: MDEV-16229 Replication aborts with ER_VIEW_SELECT_TMPTABLE after half-failed RENAME Problem was that detection of temporary tables was all wrong for RENAME TABLE. (Temporary tables where opened by top level call to open_temporary_tables(), which can't detect if a temporary table was renamed to something and then reused). Fixed by adding proper parsing of rename list to check against the current name of a table at each rename stage. Also change do_rename_temporary() to check against the current state of temporary tables, not according to the state of start of RENAME TABLE. --- sql/sql_parse.cc | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- sql/sql_rename.cc | 2 +- 2 files changed, 64 insertions(+), 3 deletions(-) (limited to 'sql') diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 1162aa901d1..28d11ef2e5c 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -495,6 +495,8 @@ void init_update_queries(void) There are other statements that deal with temporary tables and open them, but which are not listed here. The thing is that the order of pre-opening temporary tables for those statements is somewhat custom. + + Note that SQLCOM_RENAME_TABLE should not be in this list! */ sql_command_flags[SQLCOM_CREATE_TABLE]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_DROP_TABLE]|= CF_PREOPEN_TMP_TABLES; @@ -508,7 +510,6 @@ void init_update_queries(void) sql_command_flags[SQLCOM_INSERT_SELECT]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_DELETE]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_DELETE_MULTI]|= CF_PREOPEN_TMP_TABLES; - sql_command_flags[SQLCOM_RENAME_TABLE]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_REPLACE_SELECT]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_SELECT]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_SET_OPTION]|= CF_PREOPEN_TMP_TABLES; @@ -5333,6 +5334,60 @@ static bool execute_show_status(THD *thd, TABLE_LIST *all_tables) } +/* + Find out if a table is a temporary table + + A table is a temporary table if it's a temporary table or + there has been before a temporary table that has been renamed + to the current name. + + Some examples: + A->B B is a temporary table if and only if A is a temp. + A->B, B->C Second B is temp if A is temp + A->B, A->C Second A can't be temp as if A was temp then B is temp + and Second A can only be a normal table. C is also not temp +*/ + +static TABLE *find_temporary_table_for_rename(THD *thd, + TABLE_LIST *first_table, + TABLE_LIST *cur_table) +{ + TABLE_LIST *table; + TABLE *res= 0; + bool found= 0; + DBUG_ENTER("find_temporary_table_for_rename"); + + /* Find last instance when cur_table is in TO part */ + for (table= first_table; + table != cur_table; + table= table->next_local->next_local) + { + TABLE_LIST *next= table->next_local; + + if (!strcmp(table->get_db_name(), cur_table->get_db_name()) && + !strcmp(table->get_table_name(), cur_table->get_table_name())) + { + /* Table was moved away, can't be same as 'table' */ + found= 1; + res= 0; // Table can't be a temporary table + } + if (!strcmp(next->get_db_name(), cur_table->get_db_name()) && + !strcmp(next->get_table_name(), cur_table->get_table_name())) + { + /* + Table has matching name with new name of this table. cur_table should + have same temporary type as this table. + */ + found= 1; + res= table->table; + } + } + if (!found) + res= find_temporary_table(thd, table); + DBUG_RETURN(res); +} + + static bool execute_rename_table(THD *thd, TABLE_LIST *first_table, TABLE_LIST *all_tables) { @@ -5349,13 +5404,19 @@ static bool execute_rename_table(THD *thd, TABLE_LIST *first_table, &table->next_local->grant.m_internal, 0, 0)) return 1; + + /* check if these are refering to temporary tables */ + table->table= find_temporary_table_for_rename(thd, first_table, table); + table->next_local->table= table->table; + TABLE_LIST old_list, new_list; /* we do not need initialize old_list and new_list because we will - come table[0] and table->next[0] there + copy table[0] and table->next[0] there */ old_list= table[0]; new_list= table->next_local[0]; + if (check_grant(thd, ALTER_ACL | DROP_ACL, &old_list, FALSE, 1, FALSE) || (!test_all_bits(table->next_local->grant.privilege, INSERT_ACL | CREATE_ACL) && diff --git a/sql/sql_rename.cc b/sql/sql_rename.cc index 1a9cb842e6a..119f9063724 100644 --- a/sql/sql_rename.cc +++ b/sql/sql_rename.cc @@ -222,7 +222,7 @@ do_rename_temporary(THD *thd, TABLE_LIST *ren_table, TABLE_LIST *new_table, new_alias= (lower_case_table_names == 2) ? new_table->alias : new_table->table_name; - if (is_temporary_table(new_table)) + if (find_temporary_table(thd, new_table)) { my_error(ER_TABLE_EXISTS_ERROR, MYF(0), new_alias); DBUG_RETURN(1); // This can't be skipped -- cgit v1.2.1 From 908676dfd9d981fd0f37a7cf9332abac522f1936 Mon Sep 17 00:00:00 2001 From: Monty Date: Tue, 22 May 2018 23:05:01 +0300 Subject: MDEV-15308 Assertion `ha_alter_info->alter_info->drop_list.elements Problem was that handle_if_exists_options() didn't correct alter_info->flags when things was removed from the list. --- sql/sql_table.cc | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) (limited to 'sql') diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 27d579a6b19..376c1362cc7 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -5808,10 +5808,28 @@ drop_create_field: List_iterator drop_it(alter_info->drop_list); Alter_drop *drop; bool remove_drop; + ulonglong left_flags= 0; while ((drop= drop_it++)) { + ulonglong cur_flag= 0; + switch (drop->type) { + case Alter_drop::COLUMN: + cur_flag= Alter_info::ALTER_DROP_COLUMN; + break; + case Alter_drop::FOREIGN_KEY: + cur_flag= Alter_info::DROP_FOREIGN_KEY; + break; + case Alter_drop::KEY: + cur_flag= Alter_info::ALTER_DROP_INDEX; + break; + default: + break; + } if (!drop->drop_if_exists) + { + left_flags|= cur_flag; continue; + } remove_drop= TRUE; if (drop->type == Alter_drop::COLUMN) { @@ -5887,12 +5905,15 @@ drop_create_field: ER_CANT_DROP_FIELD_OR_KEY, ER(ER_CANT_DROP_FIELD_OR_KEY), drop->name); drop_it.remove(); - if (alter_info->drop_list.is_empty()) - alter_info->flags&= ~(Alter_info::ALTER_DROP_COLUMN | - Alter_info::ALTER_DROP_INDEX | - Alter_info::DROP_FOREIGN_KEY); } + else + left_flags|= cur_flag; } + /* Reset state to what's left in drop list */ + alter_info->flags&= ~(Alter_info::ALTER_DROP_COLUMN | + Alter_info::ALTER_DROP_INDEX | + Alter_info::DROP_FOREIGN_KEY); + alter_info->flags|= left_flags; } /* ALTER TABLE ADD KEY IF NOT EXISTS */ -- cgit v1.2.1 From a816aa066e5c879a92819d694a93d245e6ec6e47 Mon Sep 17 00:00:00 2001 From: Monty Date: Wed, 23 May 2018 11:26:49 +0300 Subject: Fixed ASAN heap-use-after-free handler::ha_index_or_rnd_end MDEV-16123 ASAN heap-use-after-free handler::ha_index_or_rnd_end MDEV-13828 Segmentation fault on RENAME TABLE Problem was that destructor called methods for closed table. Fixed by removing code in destructor. --- sql/sql_statistics.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'sql') diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index ce320e87a4f..471749ad346 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -1376,7 +1376,8 @@ public: ~Stat_table_write_iter() { - cleanup(); + /* Ensure that cleanup has been run */ + DBUG_ASSERT(rowid_buf == 0); } }; -- cgit v1.2.1