From d00441e328bd126d43fe88d157d6140013d17c90 Mon Sep 17 00:00:00 2001 From: "ingo@mysql.com" <> Date: Mon, 29 May 2006 15:26:23 +0200 Subject: [PATCH 1/3] Bug#19815 - CREATE/RENAME/DROP DATABASE can deadlock on a global read lock The order of acquiring LOCK_mysql_create_db and wait_if_global_read_lock() was wrong. It could happen that a thread held LOCK_mysql_create_db while waiting for the global read lock to be released. The thread with the global read lock could try to administrate a database too. It would first try to lock LOCK_mysql_create_db and hang... The check if the current thread has the global read lock is done in wait_if_global_read_lock(), which could not be reached because of the hang in LOCK_mysql_create_db. Now I exchanged the order of acquiring LOCK_mysql_create_db and wait_if_global_read_lock(). This makes wait_if_global_read_lock() fail with an error message for the thread with the global read lock. No deadlock happens. --- mysql-test/r/lock_multi.result | 8 +++++ mysql-test/t/lock_multi.test | 35 ++++++++++++++++++++ sql/sql_db.cc | 60 ++++++++++++++++++++++++++-------- 3 files changed, 89 insertions(+), 14 deletions(-) diff --git a/mysql-test/r/lock_multi.result b/mysql-test/r/lock_multi.result index 73e3a9d32e3..9c5652657a5 100644 --- a/mysql-test/r/lock_multi.result +++ b/mysql-test/r/lock_multi.result @@ -43,3 +43,11 @@ Field Type Null Key Default Extra a int(11) YES NULL unlock tables; drop table t1; +CREATE DATABASE mysqltest_1; +FLUSH TABLES WITH READ LOCK; + DROP DATABASE mysqltest_1; +DROP DATABASE mysqltest_1; +ERROR HY000: Can't execute the query because you have a conflicting read lock +UNLOCK TABLES; +DROP DATABASE mysqltest_1; +ERROR HY000: Can't drop database 'mysqltest_1'; database doesn't exist diff --git a/mysql-test/t/lock_multi.test b/mysql-test/t/lock_multi.test index 0d2266fc2ae..c7d2bcdfbc5 100644 --- a/mysql-test/t/lock_multi.test +++ b/mysql-test/t/lock_multi.test @@ -107,3 +107,38 @@ show columns from t1; connection locker; unlock tables; drop table t1; + +# +# Bug#19815 - CREATE/RENAME/DROP DATABASE can deadlock on a global read lock +# +connect (con1,localhost,root,,); +connect (con2,localhost,root,,); +# +connection con1; +CREATE DATABASE mysqltest_1; +FLUSH TABLES WITH READ LOCK; +# +# With bug in place: acquire LOCK_mysql_create_table and +# wait in wait_if_global_read_lock(). +connection con2; +send DROP DATABASE mysqltest_1; +--sleep 1 +# +# With bug in place: try to acquire LOCK_mysql_create_table... +# When fixed: Reject dropping db because of the read lock. +connection con1; +--error ER_CANT_UPDATE_WITH_READLOCK +DROP DATABASE mysqltest_1; +UNLOCK TABLES; +# +connection con2; +reap; +# +connection default; +disconnect con1; +disconnect con2; +# This must have been dropped by connection 2 already, +# which waited until the global read lock was released. +--error ER_DB_DROP_EXISTS +DROP DATABASE mysqltest_1; + diff --git a/sql/sql_db.cc b/sql/sql_db.cc index 4caa0076c60..a52972753a7 100644 --- a/sql/sql_db.cc +++ b/sql/sql_db.cc @@ -424,16 +424,27 @@ bool mysql_create_db(THD *thd, char *db, HA_CREATE_INFO *create_info, my_error(ER_DB_CREATE_EXISTS, MYF(0), db); DBUG_RETURN(-1); } - - VOID(pthread_mutex_lock(&LOCK_mysql_create_db)); - /* do not create database if another thread is holding read lock */ + /* + Do not create database if another thread is holding read lock. + Wait for global read lock before acquiring LOCK_mysql_create_db. + After wait_if_global_read_lock() we have protection against another + global read lock. If we would acquire LOCK_mysql_create_db first, + another thread could step in and get the global read lock before we + reach wait_if_global_read_lock(). If this thread tries the same as we + (admin a db), it would then go and wait on LOCK_mysql_create_db... + Furthermore wait_if_global_read_lock() checks if the current thread + has the global read lock and refuses the operation with + ER_CANT_UPDATE_WITH_READLOCK if applicable. + */ if (wait_if_global_read_lock(thd, 0, 1)) { error= -1; goto exit2; } + VOID(pthread_mutex_lock(&LOCK_mysql_create_db)); + /* Check directory */ strxmov(path, mysql_data_home, "/", db, NullS); path_len= unpack_dirname(path,path); // Convert if not unix @@ -537,9 +548,9 @@ bool mysql_create_db(THD *thd, char *db, HA_CREATE_INFO *create_info, } exit: + VOID(pthread_mutex_unlock(&LOCK_mysql_create_db)); start_waiting_global_read_lock(thd); exit2: - VOID(pthread_mutex_unlock(&LOCK_mysql_create_db)); DBUG_RETURN(error); } @@ -553,12 +564,23 @@ bool mysql_alter_db(THD *thd, const char *db, HA_CREATE_INFO *create_info) int error= 0; DBUG_ENTER("mysql_alter_db"); - VOID(pthread_mutex_lock(&LOCK_mysql_create_db)); - - /* do not alter database if another thread is holding read lock */ + /* + Do not alter database if another thread is holding read lock. + Wait for global read lock before acquiring LOCK_mysql_create_db. + After wait_if_global_read_lock() we have protection against another + global read lock. If we would acquire LOCK_mysql_create_db first, + another thread could step in and get the global read lock before we + reach wait_if_global_read_lock(). If this thread tries the same as we + (admin a db), it would then go and wait on LOCK_mysql_create_db... + Furthermore wait_if_global_read_lock() checks if the current thread + has the global read lock and refuses the operation with + ER_CANT_UPDATE_WITH_READLOCK if applicable. + */ if ((error=wait_if_global_read_lock(thd,0,1))) goto exit2; + VOID(pthread_mutex_lock(&LOCK_mysql_create_db)); + /* Check directory */ strxmov(path, mysql_data_home, "/", db, "/", MY_DB_OPT_FILE, NullS); fn_format(path, path, "", "", MYF(MY_UNPACK_FILENAME)); @@ -596,9 +618,9 @@ bool mysql_alter_db(THD *thd, const char *db, HA_CREATE_INFO *create_info) send_ok(thd, result); exit: + VOID(pthread_mutex_unlock(&LOCK_mysql_create_db)); start_waiting_global_read_lock(thd); exit2: - VOID(pthread_mutex_unlock(&LOCK_mysql_create_db)); DBUG_RETURN(error); } @@ -630,15 +652,26 @@ bool mysql_rm_db(THD *thd,char *db,bool if_exists, bool silent) TABLE_LIST* dropped_tables= 0; DBUG_ENTER("mysql_rm_db"); - VOID(pthread_mutex_lock(&LOCK_mysql_create_db)); - - /* do not drop database if another thread is holding read lock */ + /* + Do not drop database if another thread is holding read lock. + Wait for global read lock before acquiring LOCK_mysql_create_db. + After wait_if_global_read_lock() we have protection against another + global read lock. If we would acquire LOCK_mysql_create_db first, + another thread could step in and get the global read lock before we + reach wait_if_global_read_lock(). If this thread tries the same as we + (admin a db), it would then go and wait on LOCK_mysql_create_db... + Furthermore wait_if_global_read_lock() checks if the current thread + has the global read lock and refuses the operation with + ER_CANT_UPDATE_WITH_READLOCK if applicable. + */ if (wait_if_global_read_lock(thd, 0, 1)) { error= -1; goto exit2; } + VOID(pthread_mutex_lock(&LOCK_mysql_create_db)); + (void) sprintf(path,"%s/%s",mysql_data_home,db); length= unpack_dirname(path,path); // Convert if not unix strmov(path+length, MY_DB_OPT_FILE); // Append db option file name @@ -747,7 +780,6 @@ bool mysql_rm_db(THD *thd,char *db,bool if_exists, bool silent) exit: (void)sp_drop_db_routines(thd, db); /* QQ Ignore errors for now */ - start_waiting_global_read_lock(thd); /* If this database was the client's selected database, we silently change the client's selected database to nothing (to have an empty SELECT DATABASE() @@ -776,9 +808,9 @@ exit: thd->db= 0; thd->db_length= 0; } -exit2: VOID(pthread_mutex_unlock(&LOCK_mysql_create_db)); - + start_waiting_global_read_lock(thd); +exit2: DBUG_RETURN(error); } From 0cd7ac833a264ba7e9c0a569f2f889122c61d48a Mon Sep 17 00:00:00 2001 From: "acurtis@xiphis.org" <> Date: Tue, 30 May 2006 17:10:53 -0700 Subject: [PATCH 2/3] Bug#19648 "Merge table does not work with bit types" MERGE should have HA_CAN_BIT_FIELD feature bit set or else table row is formatted incorrectly. --- mysql-test/r/merge.result | 6 ++++++ mysql-test/t/merge.test | 9 +++++++++ sql/ha_myisammrg.h | 3 ++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/mysql-test/r/merge.result b/mysql-test/r/merge.result index 9a34d6fba58..568f83b7d6d 100644 --- a/mysql-test/r/merge.result +++ b/mysql-test/r/merge.result @@ -776,3 +776,9 @@ insert into t1 values ("Monty"),("WAX"),("Walrus"); alter table t1 engine=MERGE; ERROR HY000: Table storage engine for 't1' doesn't have this option drop table t1; +create table t1 (b bit(1)); +create table t2 (b bit(1)); +create table tm (b bit(1)) engine = merge union = (t1,t2); +select * from tm; +b +drop table tm, t1, t2; diff --git a/mysql-test/t/merge.test b/mysql-test/t/merge.test index 7ea14a811ed..400279a826b 100644 --- a/mysql-test/t/merge.test +++ b/mysql-test/t/merge.test @@ -390,4 +390,13 @@ insert into t1 values ("Monty"),("WAX"),("Walrus"); alter table t1 engine=MERGE; drop table t1; +# +# BUG#19648 - Merge table does not work with bit types +# +create table t1 (b bit(1)); +create table t2 (b bit(1)); +create table tm (b bit(1)) engine = merge union = (t1,t2); +select * from tm; +drop table tm, t1, t2; + # End of 5.0 tests diff --git a/sql/ha_myisammrg.h b/sql/ha_myisammrg.h index c762b7c286e..a73f368c51d 100644 --- a/sql/ha_myisammrg.h +++ b/sql/ha_myisammrg.h @@ -37,7 +37,8 @@ class ha_myisammrg: public handler { return (HA_REC_NOT_IN_SEQ | HA_AUTO_PART_KEY | HA_READ_RND_SAME | HA_NULL_IN_KEY | HA_CAN_INDEX_BLOBS | HA_FILE_BASED | - HA_CAN_INSERT_DELAYED | HA_ANY_INDEX_MAY_BE_UNIQUE); + HA_CAN_INSERT_DELAYED | HA_ANY_INDEX_MAY_BE_UNIQUE | + HA_CAN_BIT_FIELD); } ulong index_flags(uint inx, uint part, bool all_parts) const { From 6345502b538e24a0b442f6c895dde910c4cf0b4e Mon Sep 17 00:00:00 2001 From: "ingo@mysql.com" <> Date: Wed, 31 May 2006 10:22:44 +0200 Subject: [PATCH 3/3] Bug#19604 - CHECK TABLE with concurrent INSERT can reset auto_increment CHECK TABLE did temporarily clear the auto_increment value. It runs with a read lock, allowing other readers and conurrent INSERTs. The latter could grab the wrong value in this moment. CHECK TABLE does no longer modify the auto_increment value. Not even for a short moment. --- myisam/mi_check.c | 34 ++++++++++++++++------------------ myisam/mi_key.c | 14 ++++++-------- myisam/mi_update.c | 3 ++- myisam/mi_write.c | 3 ++- myisam/myisamdef.h | 2 +- 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/myisam/mi_check.c b/myisam/mi_check.c index 7d87ecd8595..e0f04965650 100644 --- a/myisam/mi_check.c +++ b/myisam/mi_check.c @@ -453,25 +453,24 @@ int chk_key(MI_CHECK *param, register MI_INFO *info) if ((uint) share->base.auto_key -1 == key) { /* Check that auto_increment key is bigger than max key value */ - ulonglong save_auto_value=info->s->state.auto_increment; - info->s->state.auto_increment=0; + ulonglong auto_increment; info->lastinx=key; _mi_read_key_record(info, 0L, info->rec_buff); - update_auto_increment(info, info->rec_buff); - if (info->s->state.auto_increment > save_auto_value) + auto_increment= retrieve_auto_increment(info, info->rec_buff); + if (auto_increment > info->s->state.auto_increment) { - mi_check_print_warning(param, - "Auto-increment value: %s is smaller than max used value: %s", - llstr(save_auto_value,buff2), - llstr(info->s->state.auto_increment, buff)); + mi_check_print_warning(param, "Auto-increment value: %s is smaller " + "than max used value: %s", + llstr(info->s->state.auto_increment,buff2), + llstr(auto_increment, buff)); } if (param->testflag & T_AUTO_INC) { - set_if_bigger(info->s->state.auto_increment, - param->auto_increment_value); + set_if_bigger(info->s->state.auto_increment, + auto_increment); + set_if_bigger(info->s->state.auto_increment, + param->auto_increment_value); } - else - info->s->state.auto_increment=save_auto_value; /* Check that there isn't a row with auto_increment = 0 in the table */ mi_extra(info,HA_EXTRA_KEYREAD,0); @@ -481,8 +480,8 @@ int chk_key(MI_CHECK *param, register MI_INFO *info) { /* Don't count this as a real warning, as myisamchk can't correct it */ uint save=param->warning_printed; - mi_check_print_warning(param, - "Found row where the auto_increment column has the value 0"); + mi_check_print_warning(param, "Found row where the auto_increment " + "column has the value 0"); param->warning_printed=save; } mi_extra(info,HA_EXTRA_NO_KEYREAD,0); @@ -4099,11 +4098,10 @@ void update_auto_increment_key(MI_CHECK *param, MI_INFO *info, } else { - ulonglong auto_increment= (repair_only ? info->s->state.auto_increment : - param->auto_increment_value); - info->s->state.auto_increment=0; - update_auto_increment(info, record); + ulonglong auto_increment= retrieve_auto_increment(info, record); set_if_bigger(info->s->state.auto_increment,auto_increment); + if (!repair_only) + set_if_bigger(info->s->state.auto_increment, param->auto_increment_value); } mi_extra(info,HA_EXTRA_NO_KEYREAD,0); my_free((char*) record, MYF(0)); diff --git a/myisam/mi_key.c b/myisam/mi_key.c index 717a5dbd56e..17ea56f0210 100644 --- a/myisam/mi_key.c +++ b/myisam/mi_key.c @@ -507,22 +507,21 @@ int _mi_read_key_record(MI_INFO *info, my_off_t filepos, byte *buf) return(-1); /* Wrong data to read */ } - + /* - Update auto_increment info + Retrieve auto_increment info SYNOPSIS - update_auto_increment() + retrieve_auto_increment() info MyISAM handler record Row to update IMPLEMENTATION - Only replace the auto_increment value if it is higher than the previous - one. For signed columns we don't update the auto increment value if it's + For signed columns we don't retrieve the auto increment value if it's less than zero. */ -void update_auto_increment(MI_INFO *info,const byte *record) +ulonglong retrieve_auto_increment(MI_INFO *info,const byte *record) { ulonglong value= 0; /* Store unsigned values here */ longlong s_value= 0; /* Store signed values here */ @@ -587,6 +586,5 @@ void update_auto_increment(MI_INFO *info,const byte *record) and if s_value == 0 then value will contain either s_value or the correct value. */ - set_if_bigger(info->s->state.auto_increment, - (s_value > 0) ? (ulonglong) s_value : value); + return (s_value > 0) ? (ulonglong) s_value : value; } diff --git a/myisam/mi_update.c b/myisam/mi_update.c index 937c9983b45..f8b5cf55406 100644 --- a/myisam/mi_update.c +++ b/myisam/mi_update.c @@ -164,7 +164,8 @@ int mi_update(register MI_INFO *info, const byte *oldrec, byte *newrec) key_changed|= HA_STATE_CHANGED; /* Must update index file */ } if (auto_key_changed) - update_auto_increment(info,newrec); + set_if_bigger(info->s->state.auto_increment, + retrieve_auto_increment(info, newrec)); if (share->calc_checksum) info->state->checksum+=(info->checksum - old_checksum); diff --git a/myisam/mi_write.c b/myisam/mi_write.c index 5e79b2937cc..9ab8753f6d7 100644 --- a/myisam/mi_write.c +++ b/myisam/mi_write.c @@ -149,7 +149,8 @@ int mi_write(MI_INFO *info, byte *record) info->state->checksum+=info->checksum; } if (share->base.auto_key) - update_auto_increment(info,record); + set_if_bigger(info->s->state.auto_increment, + retrieve_auto_increment(info, record)); info->update= (HA_STATE_CHANGED | HA_STATE_AKTIV | HA_STATE_WRITTEN | HA_STATE_ROW_CHANGED); info->state->records++; diff --git a/myisam/myisamdef.h b/myisam/myisamdef.h index 6ccb52aff22..d589173f0e7 100644 --- a/myisam/myisamdef.h +++ b/myisam/myisamdef.h @@ -582,7 +582,7 @@ extern uint _mi_pack_key(MI_INFO *info,uint keynr,uchar *key,uchar *old, extern int _mi_read_key_record(MI_INFO *info,my_off_t filepos,byte *buf); extern int _mi_read_cache(IO_CACHE *info,byte *buff,my_off_t pos, uint length,int re_read_if_possibly); -extern void update_auto_increment(MI_INFO *info,const byte *record); +extern ulonglong retrieve_auto_increment(MI_INFO *info,const byte *record); extern byte *mi_alloc_rec_buff(MI_INFO *,ulong, byte**); #define mi_get_rec_buff_ptr(info,buf) \