diff --git a/include/my_pthread.h b/include/my_pthread.h index ee2c801ff6ee0b7cd72a2e90f4e4e38584d46e74..6f60a6df2c1e3ce78a3424fcb80c788770fdda72 100644 --- a/include/my_pthread.h +++ b/include/my_pthread.h @@ -536,9 +536,15 @@ void safe_mutex_end(FILE *file); #define pthread_cond_timedwait(A,B,C) safe_cond_timedwait((A),(B),(C),__FILE__,__LINE__) #define pthread_mutex_trylock(A) pthread_mutex_lock(A) #define pthread_mutex_t safe_mutex_t -#define safe_mutex_assert_owner(mp) DBUG_ASSERT((mp)->count > 0 && pthread_equal(pthread_self(),(mp)->thread)) +#define safe_mutex_assert_owner(mp) \ + DBUG_ASSERT((mp)->count > 0 && \ + pthread_equal(pthread_self(), (mp)->thread)) +#define safe_mutex_assert_not_owner(mp) \ + DBUG_ASSERT(! (mp)->count || \ + ! pthread_equal(pthread_self(), (mp)->thread)) #else #define safe_mutex_assert_owner(mp) +#define safe_mutex_assert_not_owner(mp) #endif /* SAFE_MUTEX */ /* READ-WRITE thread locking */ diff --git a/mysql-test/r/handler.result b/mysql-test/r/handler.result index 072d4582cbc90ecc8aecc51f4f6d0fcf83674926..133683fb273fa2b12bdf9535e91006d0c78c7f7c 100644 --- a/mysql-test/r/handler.result +++ b/mysql-test/r/handler.result @@ -445,3 +445,40 @@ drop table t2; drop table t3; drop table t4; drop table t5; +create table t1 (c1 int); +insert into t1 values (1); +handler t1 open; +handler t1 read first; +c1 +1 +send the below to another connection, do not wait for the result + optimize table t1; +proceed with the normal connection +handler t1 read next; +c1 +1 +handler t1 close; +read the result from the other connection +Table Op Msg_type Msg_text +test.t1 optimize status OK +proceed with the normal connection +drop table t1; +create table t1 (c1 int); +insert into t1 values (14397); +flush tables with read lock; +drop table t1; +ERROR HY000: Can't execute the query because you have a conflicting read lock +send the below to another connection, do not wait for the result + drop table t1; +proceed with the normal connection +select * from t1; +c1 +14397 +unlock tables; +read the result from the other connection +proceed with the normal connection +select * from t1; +ERROR 42S02: Table 'test.t1' doesn't exist +drop table if exists t1; +Warnings: +Note 1051 Unknown table 't1' diff --git a/mysql-test/t/handler.test b/mysql-test/t/handler.test index 1bb9b1d350409e7c22691110c8a4bfdd14a4b5ce..f3e14c3cd2be0729004eb57c0184a62a9c67dba8 100644 --- a/mysql-test/t/handler.test +++ b/mysql-test/t/handler.test @@ -347,4 +347,79 @@ drop table t3; drop table t4; drop table t5; +# +# Bug#14397 - OPTIMIZE TABLE with an open HANDLER causes a crash +# +create table t1 (c1 int); +insert into t1 values (1); +# client 1 +handler t1 open; +handler t1 read first; +# client 2 +connect (con2,localhost,root,,); +connection con2; +--exec echo send the below to another connection, do not wait for the result +send optimize table t1; +--sleep 1 +# client 1 +--exec echo proceed with the normal connection +connection default; +handler t1 read next; +handler t1 close; +# client 2 +--exec echo read the result from the other connection +connection con2; +reap; +# client 1 +--exec echo proceed with the normal connection +connection default; +drop table t1; + # End of 4.1 tests + +# +# Addendum to Bug#14397 - OPTIMIZE TABLE with an open HANDLER causes a crash +# Show that DROP TABLE can no longer deadlock against +# FLUSH TABLES WITH READ LOCK. This is a 5.0 issue. +# +create table t1 (c1 int); +insert into t1 values (14397); +flush tables with read lock; +# The thread with the global read lock cannot drop the table itself: +--error 1223 +drop table t1; +# +# client 2 +# We need a second connection to try the drop. +# The drop waits for the global read lock to go away. +# Without the addendum fix it locked LOCK_open before entering the wait loop. +connection con2; +--exec echo send the below to another connection, do not wait for the result +send drop table t1; +--sleep 1 +# +# client 1 +# Now we need something that wants LOCK_open. A simple table access which +# opens the table does the trick. +--exec echo proceed with the normal connection +connection default; +# This would hang on LOCK_open without the 5.0 addendum fix. +select * from t1; +# Release the read lock. This should make the DROP go through. +unlock tables; +# +# client 2 +# Read the result of the drop command. +connection con2; +--exec echo read the result from the other connection +reap; +# +# client 1 +# Now back to normal operation. The table should not exist any more. +--exec echo proceed with the normal connection +connection default; +--error 1146 +select * from t1; +# Just to be sure and not confuse the next test case writer. +drop table if exists t1; + diff --git a/sql/lock.cc b/sql/lock.cc index f4c4a781e459442c1ceafc118ddeea19c562fbac..fe8dcb3aa5eb1cd5c099ea53ffaf863dc2e90a02 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -815,10 +815,13 @@ static void print_lock_error(int error, const char *table) access to them is protected with a mutex LOCK_global_read_lock - (XXX: one should never take LOCK_open if LOCK_global_read_lock is taken, - otherwise a deadlock may occur - see mysql_rm_table. Other mutexes could - be a problem too - grep the code for global_read_lock if you want to use - any other mutex here) + (XXX: one should never take LOCK_open if LOCK_global_read_lock is + taken, otherwise a deadlock may occur. Other mutexes could be a + problem too - grep the code for global_read_lock if you want to use + any other mutex here) Also one must not hold LOCK_open when calling + wait_if_global_read_lock(). When the thread with the global read lock + tries to close its tables, it needs to take LOCK_open in + close_thread_table(). How blocking of threads by global read lock is achieved: that's advisory. Any piece of code which should be blocked by global read lock must @@ -937,6 +940,13 @@ bool wait_if_global_read_lock(THD *thd, bool abort_on_refresh, DBUG_ENTER("wait_if_global_read_lock"); LINT_INIT(old_message); + /* + Assert that we do not own LOCK_open. If we would own it, other + threads could not close their tables. This would make a pretty + deadlock. + */ + safe_mutex_assert_not_owner(&LOCK_open); + (void) pthread_mutex_lock(&LOCK_global_read_lock); if ((need_exit_cond= must_wait)) { diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index 1719253a45865e65bea51d313a98d5c2b4500c70..2a65b99585a368cb83cf538539358dd723d306ff 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -890,7 +890,8 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen); bool mysql_ha_close(THD *thd, TABLE_LIST *tables); bool mysql_ha_read(THD *, TABLE_LIST *,enum enum_ha_read_modes,char *, List<Item> *,enum ha_rkey_function,Item *,ha_rows,ha_rows); -int mysql_ha_flush(THD *thd, TABLE_LIST *tables, uint mode_flags); +int mysql_ha_flush(THD *thd, TABLE_LIST *tables, uint mode_flags, + bool is_locked); /* mysql_ha_flush mode_flags bits */ #define MYSQL_HA_CLOSE_FINAL 0x00 #define MYSQL_HA_REOPEN_ON_USAGE 0x01 diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1e7fce9001fd5ea4c5f4095afd690830638dbf20..771a70ffebbebc62c4d4c7329b1c74c5f37331b5 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -311,7 +311,8 @@ bool close_cached_tables(THD *thd, bool if_wait_for_refresh, thd->proc_info="Flushing tables"; close_old_data_files(thd,thd->open_tables,1,1); - mysql_ha_flush(thd, tables, MYSQL_HA_REOPEN_ON_USAGE | MYSQL_HA_FLUSH_ALL); + mysql_ha_flush(thd, tables, MYSQL_HA_REOPEN_ON_USAGE | MYSQL_HA_FLUSH_ALL, + TRUE); bool found=1; /* Wait until all threads has closed all the tables we had locked */ DBUG_PRINT("info", @@ -1238,7 +1239,7 @@ TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root, /* close handler tables which are marked for flush */ if (thd->handler_tables) - mysql_ha_flush(thd, (TABLE_LIST*) NULL, MYSQL_HA_REOPEN_ON_USAGE); + mysql_ha_flush(thd, (TABLE_LIST*) NULL, MYSQL_HA_REOPEN_ON_USAGE, TRUE); for (table=(TABLE*) hash_search(&open_cache,(byte*) key,key_length) ; table && table->in_use ; @@ -1642,7 +1643,7 @@ bool wait_for_tables(THD *thd) { thd->some_tables_deleted=0; close_old_data_files(thd,thd->open_tables,0,dropping_tables != 0); - mysql_ha_flush(thd, (TABLE_LIST*) NULL, MYSQL_HA_REOPEN_ON_USAGE); + mysql_ha_flush(thd, (TABLE_LIST*) NULL, MYSQL_HA_REOPEN_ON_USAGE, TRUE); if (!table_is_used(thd->open_tables,1)) break; (void) pthread_cond_wait(&COND_refresh,&LOCK_open); diff --git a/sql/sql_class.cc b/sql/sql_class.cc index fc9df020b6c84bf5c4f0499d0173d53fc0ea8d07..7ffe60ec2b21bcd53d85f20eb739909fd5995f4e 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -376,7 +376,7 @@ void THD::cleanup(void) close_thread_tables(this); } mysql_ha_flush(this, (TABLE_LIST*) 0, - MYSQL_HA_CLOSE_FINAL | MYSQL_HA_FLUSH_ALL); + MYSQL_HA_CLOSE_FINAL | MYSQL_HA_FLUSH_ALL, FALSE); hash_free(&handler_tables_hash); delete_dynamic(&user_var_events); hash_free(&user_vars); diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc index cc45a7001cd17d422d75ab7bbe7f88b6397ceb0b..07f4de26707fd69dd5b28d54e8e5aa5beb1b60ab 100644 --- a/sql/sql_handler.cc +++ b/sql/sql_handler.cc @@ -336,6 +336,7 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables, ha_rows select_limit_cnt, ha_rows offset_limit_cnt) { TABLE_LIST *hash_tables; + TABLE **table_ptr; TABLE *table; MYSQL_LOCK *lock; List<Item> list; @@ -368,6 +369,28 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables, DBUG_PRINT("info-in-hash",("'%s'.'%s' as '%s' tab %p", hash_tables->db, hash_tables->table_name, hash_tables->alias, table)); + /* Table might have been flushed. */ + if (table && (table->s->version != refresh_version)) + { + /* + We must follow the thd->handler_tables chain, as we need the + address of the 'next' pointer referencing this table + for close_thread_table(). + */ + for (table_ptr= &(thd->handler_tables); + *table_ptr && (*table_ptr != table); + table_ptr= &(*table_ptr)->next) + {} + (*table_ptr)->file->ha_index_or_rnd_end(); + VOID(pthread_mutex_lock(&LOCK_open)); + if (close_thread_table(thd, table_ptr)) + { + /* Tell threads waiting for refresh that something has happened */ + VOID(pthread_cond_broadcast(&COND_refresh)); + } + VOID(pthread_mutex_unlock(&LOCK_open)); + table= hash_tables->table= NULL; + } if (!table) { /* @@ -594,6 +617,7 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables, MYSQL_HA_REOPEN_ON_USAGE mark for reopen. MYSQL_HA_FLUSH_ALL flush all tables, not only those marked for flush. + is_locked If LOCK_open is locked. DESCRIPTION The list of HANDLER tables may be NULL, in which case all HANDLER @@ -601,7 +625,6 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables, If 'tables' is NULL and MYSQL_HA_FLUSH_ALL is not set, all HANDLER tables marked for flush are closed. Broadcasts a COND_refresh condition, for every table closed. - The caller must lock LOCK_open. NOTE Since mysql_ha_flush() is called when the base table has to be closed, @@ -611,10 +634,12 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables, 0 ok */ -int mysql_ha_flush(THD *thd, TABLE_LIST *tables, uint mode_flags) +int mysql_ha_flush(THD *thd, TABLE_LIST *tables, uint mode_flags, + bool is_locked) { TABLE_LIST *tmp_tables; TABLE **table_ptr; + bool did_lock= FALSE; DBUG_ENTER("mysql_ha_flush"); DBUG_PRINT("enter", ("tables: %p mode_flags: 0x%02x", tables, mode_flags)); @@ -640,6 +665,12 @@ int mysql_ha_flush(THD *thd, TABLE_LIST *tables, uint mode_flags) (*table_ptr)->s->db, (*table_ptr)->s->table_name, (*table_ptr)->alias)); + /* The first time it is required, lock for close_thread_table(). */ + if (! did_lock && ! is_locked) + { + VOID(pthread_mutex_lock(&LOCK_open)); + did_lock= TRUE; + } mysql_ha_flush_table(thd, table_ptr, mode_flags); continue; } @@ -658,6 +689,12 @@ int mysql_ha_flush(THD *thd, TABLE_LIST *tables, uint mode_flags) if ((mode_flags & MYSQL_HA_FLUSH_ALL) || ((*table_ptr)->s->version != refresh_version)) { + /* The first time it is required, lock for close_thread_table(). */ + if (! did_lock && ! is_locked) + { + VOID(pthread_mutex_lock(&LOCK_open)); + did_lock= TRUE; + } mysql_ha_flush_table(thd, table_ptr, mode_flags); continue; } @@ -665,6 +702,10 @@ int mysql_ha_flush(THD *thd, TABLE_LIST *tables, uint mode_flags) } } + /* Release the lock if it was taken by this function. */ + if (did_lock) + VOID(pthread_mutex_unlock(&LOCK_open)); + DBUG_RETURN(0); } @@ -696,8 +737,8 @@ static int mysql_ha_flush_table(THD *thd, TABLE **table_ptr, uint mode_flags) table->alias, mode_flags)); if ((hash_tables= (TABLE_LIST*) hash_search(&thd->handler_tables_hash, - (byte*) (*table_ptr)->alias, - strlen((*table_ptr)->alias) + 1))) + (byte*) table->alias, + strlen(table->alias) + 1))) { if (! (mode_flags & MYSQL_HA_REOPEN_ON_USAGE)) { @@ -712,6 +753,7 @@ static int mysql_ha_flush_table(THD *thd, TABLE **table_ptr, uint mode_flags) } (*table_ptr)->file->ha_index_or_rnd_end(); + safe_mutex_assert_owner(&LOCK_open); if (close_thread_table(thd, table_ptr)) { /* Tell threads waiting for refresh that something has happened */ diff --git a/sql/sql_table.cc b/sql/sql_table.cc index c0748abf3338cbbc599eca743dc4a834cf72603b..0d6ef9bb76776b1db915fc93fca6169a98313ea3 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -103,23 +103,28 @@ bool mysql_rm_table(THD *thd,TABLE_LIST *tables, my_bool if_exists, /* mark for close and remove all cached entries */ - thd->mysys_var->current_mutex= &LOCK_open; - thd->mysys_var->current_cond= &COND_refresh; - VOID(pthread_mutex_lock(&LOCK_open)); - if (!drop_temporary) { if ((error= wait_if_global_read_lock(thd, 0, 1))) { my_error(ER_TABLE_NOT_LOCKED_FOR_WRITE, MYF(0), tables->table_name); - goto err; + DBUG_RETURN(TRUE); } else need_start_waiters= TRUE; } + + /* + Acquire LOCK_open after wait_if_global_read_lock(). If we would hold + LOCK_open during wait_if_global_read_lock(), other threads could not + close their tables. This would make a pretty deadlock. + */ + thd->mysys_var->current_mutex= &LOCK_open; + thd->mysys_var->current_cond= &COND_refresh; + VOID(pthread_mutex_lock(&LOCK_open)); + error= mysql_rm_table_part2(thd, tables, if_exists, drop_temporary, 0, 0); -err: pthread_mutex_unlock(&LOCK_open); pthread_mutex_lock(&thd->mysys_var->mutex); @@ -232,7 +237,7 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists, char *db=table->db; db_type table_type= DB_TYPE_UNKNOWN; - mysql_ha_flush(thd, table, MYSQL_HA_CLOSE_FINAL); + mysql_ha_flush(thd, table, MYSQL_HA_CLOSE_FINAL, TRUE); if (!close_temporary_table(thd, db, table->table_name)) { tmp_table_deleted=1; @@ -2171,7 +2176,7 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) DBUG_RETURN(TRUE); - mysql_ha_flush(thd, tables, MYSQL_HA_CLOSE_FINAL); + mysql_ha_flush(thd, tables, MYSQL_HA_CLOSE_FINAL, FALSE); for (table= tables; table; table= table->next_local) { char table_name[NAME_LEN*2+2]; @@ -3128,7 +3133,7 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name, new_db= db; used_fields=create_info->used_fields; - mysql_ha_flush(thd, table_list, MYSQL_HA_CLOSE_FINAL); + mysql_ha_flush(thd, table_list, MYSQL_HA_CLOSE_FINAL, FALSE); /* DISCARD/IMPORT TABLESPACE is always alone in an ALTER TABLE */ if (alter_info->tablespace_op != NO_TABLESPACE_OP) DBUG_RETURN(mysql_discard_or_import_tablespace(thd,table_list,