From 3b897f2bc5de31e7f166a02fccf5f3e4017f4a5f Mon Sep 17 00:00:00 2001 From: Mattias Jonsson Date: Wed, 17 Mar 2010 15:10:41 +0100 Subject: Bug#50561: ALTER PARTITIONS does not have adequate lock, breaks with concurrent I_S query There were two problem: 1) MYSQL_LOCK_IGNORE_FLUSH also ignored name locks 2) there was a race between abort_and_upgrade_locks and alter_close_tables (i.e. remove_table_from_cache and close_data_files_and_morph_locks) Which allowed the table to be opened with MYSQL_LOCK_IGNORE_FLUSH flag resulting in renaming a partition that was already in use, which could cause the table to be unusable. Solution was to not allow IGNORE_FLUSH to skip waiting for a named locked table. And to not release the LOCK_open mutex between the calls to remove_table_from_cache and close_data_files_and_morph_locks by merging the functions abort_and_upgrade_locks and alter_close_tables. --- sql/sql_base.cc | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) (limited to 'sql/sql_base.cc') diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 0e70eb93725..91794e061e4 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1244,6 +1244,7 @@ void close_thread_tables(THD *thd) table->s->table_name.str, (long) table)); #endif + DEBUG_SYNC(thd, "before_close_thread_tables"); /* We are assuming here that thd->derived_tables contains ONLY derived tables for this substatement. i.e. instead of approach which uses @@ -2502,7 +2503,7 @@ bool check_if_table_exists(THD *thd, TABLE_LIST *table, bool *exists) put in the thread-open-list. flags Bitmap of flags to modify how open works: MYSQL_LOCK_IGNORE_FLUSH - Open table even if - someone has done a flush or namelock on it. + someone has done a flush on it. No version number checking is done. MYSQL_OPEN_TEMPORARY_ONLY - Open only temporary table not the base table or view. @@ -2792,8 +2793,10 @@ TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root, ("Found table '%s.%s' with different refresh version", table_list->db, table_list->table_name)); - if (flags & MYSQL_LOCK_IGNORE_FLUSH) + /* Ignore FLUSH, but not name locks! */ + if (flags & MYSQL_LOCK_IGNORE_FLUSH && !table->open_placeholder) { + DBUG_ASSERT(table->db_stat); /* Force close at once after usage */ thd->version= table->s->version; continue; @@ -4448,7 +4451,7 @@ thr_lock_type read_lock_type_for_table(THD *thd, TABLE *table) counter - number of opened tables will be return using this parameter flags - bitmap of flags to modify how the tables will be open: MYSQL_LOCK_IGNORE_FLUSH - open table even if someone has - done a flush or namelock on it. + done a flush on it. NOTE Unless we are already in prelocked mode, this function will also precache @@ -5047,7 +5050,7 @@ int open_and_lock_tables_derived(THD *thd, TABLE_LIST *tables, bool derived) tables - list of tables for open flags - bitmap of flags to modify how the tables will be open: MYSQL_LOCK_IGNORE_FLUSH - open table even if someone has - done a flush or namelock on it. + done a flush on it. RETURN FALSE - ok @@ -8708,7 +8711,7 @@ bool is_equal(const LEX_STRING *a, const LEX_STRING *b) /* SYNOPSIS - abort_and_upgrade_lock() + abort_and_upgrade_lock_and_close_table() lpt Parameter passing struct All parameters passed through the ALTER_PARTITION_PARAM_TYPE object RETURN VALUE @@ -8717,7 +8720,7 @@ bool is_equal(const LEX_STRING *a, const LEX_STRING *b) Remember old lock level (for possible downgrade later on), abort all waiting threads and ensure that all keeping locks currently are completed such that we own the lock exclusively and no other interaction - is ongoing. + is ongoing. Close the table and hold the name lock. thd Thread object table Table object @@ -8726,17 +8729,26 @@ bool is_equal(const LEX_STRING *a, const LEX_STRING *b) old_lock_level Old lock level */ -int abort_and_upgrade_lock(ALTER_PARTITION_PARAM_TYPE *lpt) +int abort_and_upgrade_lock_and_close_table(ALTER_PARTITION_PARAM_TYPE *lpt) { uint flags= RTFC_WAIT_OTHER_THREAD_FLAG | RTFC_CHECK_KILLED_FLAG; - DBUG_ENTER("abort_and_upgrade_locks"); + const char *db= lpt->db; + const char *table_name= lpt->table_name; + THD *thd= lpt->thd; + DBUG_ENTER("abort_and_upgrade_lock_and_close_table"); lpt->old_lock_type= lpt->table->reginfo.lock_type; + safe_mutex_assert_not_owner(&LOCK_open); VOID(pthread_mutex_lock(&LOCK_open)); /* If MERGE child, forward lock handling to parent. */ - mysql_lock_abort(lpt->thd, lpt->table->parent ? lpt->table->parent : - lpt->table, TRUE); - VOID(remove_table_from_cache(lpt->thd, lpt->db, lpt->table_name, flags)); + mysql_lock_abort(thd, lpt->table->parent ? lpt->table->parent : lpt->table, + TRUE); + if (remove_table_from_cache(thd, db, table_name, flags)) + { + VOID(pthread_mutex_unlock(&LOCK_open)); + DBUG_RETURN(1); + } + close_data_files_and_morph_locks(thd, db, table_name); VOID(pthread_mutex_unlock(&LOCK_open)); DBUG_RETURN(0); } -- cgit v1.2.1 From b3f162ae337bc821e0bef95e03d8bd304794ce9d Mon Sep 17 00:00:00 2001 From: Mattias Jonsson Date: Thu, 18 Mar 2010 14:04:19 +0100 Subject: Additional fix for DEBUG_SYNC which failed for some rpl-tests, due to DBUG_ASSERT. (added in bug#50561) --- sql/sql_base.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'sql/sql_base.cc') diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 91794e061e4..61a8d5af815 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1244,7 +1244,12 @@ void close_thread_tables(THD *thd) table->s->table_name.str, (long) table)); #endif - DEBUG_SYNC(thd, "before_close_thread_tables"); +#if defined(ENABLED_DEBUG_SYNC) + /* debug_sync may not be initialized for some slave threads */ + if (thd->debug_sync_control) + DEBUG_SYNC(thd, "before_close_thread_tables"); +#endif + /* We are assuming here that thd->derived_tables contains ONLY derived tables for this substatement. i.e. instead of approach which uses -- cgit v1.2.1