From e81e55e78306643031e237c884c48f1d3d7b76c6 Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 19 Jul 2007 19:28:00 +0400 Subject: [PATCH 1/2] A fix for Bug#29431 killing an insert delayed thread causes crash No test case, since the bug requires a stress case with 30 INSERT DELAYED threads and 1 killer thread to repeat. The patch is verified manually. Review fixes. The server that is running DELAYED inserts would deadlock itself or crash under high load if some of the delayed threads were KILLed in the meanwhile. The fix is to change internal lock acquisition order of delayed inserts subsystem and to ensure that Delayed_insert::table_list::db does not point to volatile memory in some cases. For details, please see a comment for sql_insert.cc. sql/sql_insert.cc: A fix for Bug#29431 killing an insert delayed thread causes crash 1) The deadlock was caused by different lock acquisition order between delayed_get_table and handle_delayed_insert. delayed_get_table would: - acquire LOCK_delayed_create - create a new Delayed_insert instance - acquire instance mutex (di->mutex) - "lock the instance in memory" by increasing di->locked_in_memory variable - start the delayed thread - release di->mutex - let the delayed thread open the delayed table - discover that the delayed thread was killed - try to unlock() the delayed insert instance in memory - in Delayed_insert::unlock() do * lock LOCK_delayed_insert * decrease locks_in_memory and discover it's 0 * attempt to lock di->mutex to broadcast di->cond condition <-- deadlock here Meanwhile, the delayed thread would * lock di->mutex * open the table * get killed * not notice that and attempt to lock LOCK_delayed_insert to register itself in the delayed insert list <-- deadlock here. Simply put, delayed_get_table used to lock LOCK_delayed_insert and then di->mutex, and handle_delayed_insert would lock di->mutex and then LOCK_delayed_insert. Fixed by moving registration in the list of delayed insert threads from handle_delayed_insert to delayed_get_table - so that now handle_delayed_insert doesn't need to acquire LOCK_delayed_insert mutex. 2) di->table_list.db was copied by-pointer from table_list.db of the first producer -- the one who initiated creation of the delayed insert thread. This producer might be long gone when the member is needed (handle_delayed_insert:open_ltable), e.g. by having been killed with KILL CONNECTION statement. Fixed by using a pointer to the consumer's deep copy of THD::db. 3) In find_handler, we shouldn't assume that Delayed_insert object already (or still) has a table opened all the time it is present in the delayed insert list. E.g. it's not the case when Delayed_insert decided to terminate, closed the table, but haven't yet unregistered from the list (see the end of handle_delayed_insert). --- sql/sql_insert.cc | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 73f8c5e4418..1a8cacd4f8e 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -1705,8 +1705,8 @@ Delayed_insert *find_handler(THD *thd, TABLE_LIST *table_list) Delayed_insert *tmp; while ((tmp=it++)) { - if (!strcmp(tmp->thd.db,table_list->db) && - !strcmp(table_list->table_name,tmp->table->s->table_name)) + if (!strcmp(table_list->db, tmp->table_list.db) && + !strcmp(table_list->table_name, tmp->table_list.table_name)) { tmp->lock(); break; @@ -1739,7 +1739,27 @@ Delayed_insert *find_handler(THD *thd, TABLE_LIST *table_list) Two latter cases indicate a request for lock upgrade. XXX: why do we regard INSERT DELAYED into a view as an error and - do not simply a lock upgrade? + do not simply perform a lock upgrade? + + TODO: The approach with using two mutexes to work with the + delayed thread list -- LOCK_delayed_insert and + LOCK_delayed_create -- is redundant, and we only need one of + them to protect the list. The reason we have two locks is that + we do not want to block look-ups in the list while we're waiting + for the newly created thread to open the delayed table. However, + this wait itself is redundant -- we always call get_local_table + later on, and there wait again until the created thread acquires + a table lock. + + As is redundant the concept of locks_in_memory, since we already + have another counter with similar semantics - tables_in_use, + both of them are devoted to counting the number of producers for + a given consumer (delayed insert thread), only at different + stages of producer-consumer relationship. + + 'dead' and 'status' variables in Delayed_insert are redundant + too, since there is already 'di->thd.killed' and + di->stacked_inserts. */ static @@ -1788,7 +1808,9 @@ bool delayed_get_table(THD *thd, TABLE_LIST *table_list) goto end_create; } tmp->table_list= *table_list; // Needed to open table + /* Replace volatile strings with local copies */ tmp->table_list.alias= tmp->table_list.table_name= tmp->thd.query; + tmp->table_list.db= tmp->thd.db; tmp->lock(); pthread_mutex_lock(&tmp->mutex); if ((error=pthread_create(&tmp->thd.real_id,&connection_attrib, @@ -1834,6 +1856,9 @@ bool delayed_get_table(THD *thd, TABLE_LIST *table_list) tmp->unlock(); goto end_create; } + pthread_mutex_lock(&LOCK_delayed_insert); + delayed_threads.append(tmp); + pthread_mutex_unlock(&LOCK_delayed_insert); } pthread_mutex_unlock(&LOCK_delayed_create); } @@ -2176,11 +2201,6 @@ pthread_handler_t handle_delayed_insert(void *arg) } di->table->copy_blobs=1; - /* One can now use this */ - pthread_mutex_lock(&LOCK_delayed_insert); - delayed_threads.append(di); - pthread_mutex_unlock(&LOCK_delayed_insert); - /* Tell client that the thread is initialized */ pthread_cond_signal(&di->cond_client); From 62eb5f17945972c1c1f6a78c95440c71afc7d7bc Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 19 Jul 2007 19:36:52 +0400 Subject: [PATCH 2/2] Rename all references to 'Delayed_insert' instances from 'tmp' to 'di' for consistency. --- sql/sql_insert.cc | 100 +++++++++++++++++++++++----------------------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 1a8cacd4f8e..19c9360b0ed 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -1702,18 +1702,18 @@ Delayed_insert *find_handler(THD *thd, TABLE_LIST *table_list) thd->proc_info="waiting for delay_list"; pthread_mutex_lock(&LOCK_delayed_insert); // Protect master list I_List_iterator it(delayed_threads); - Delayed_insert *tmp; - while ((tmp=it++)) + Delayed_insert *di; + while ((di= it++)) { - if (!strcmp(table_list->db, tmp->table_list.db) && - !strcmp(table_list->table_name, tmp->table_list.table_name)) + if (!strcmp(table_list->db, di->table_list.db) && + !strcmp(table_list->table_name, di->table_list.table_name)) { - tmp->lock(); + di->lock(); break; } } pthread_mutex_unlock(&LOCK_delayed_insert); // For unlink from list - return tmp; + return di; } @@ -1766,14 +1766,14 @@ static bool delayed_get_table(THD *thd, TABLE_LIST *table_list) { int error; - Delayed_insert *tmp; + Delayed_insert *di; DBUG_ENTER("delayed_get_table"); /* Must be set in the parser */ DBUG_ASSERT(table_list->db); /* Find the thread which handles this table. */ - if (!(tmp=find_handler(thd,table_list))) + if (!(di= find_handler(thd, table_list))) { /* No match. Create a new thread to handle the table, but @@ -1787,9 +1787,9 @@ bool delayed_get_table(THD *thd, TABLE_LIST *table_list) The first search above was done without LOCK_delayed_create. Another thread might have created the handler in between. Search again. */ - if (! (tmp= find_handler(thd, table_list))) + if (! (di= find_handler(thd, table_list))) { - if (!(tmp=new Delayed_insert())) + if (!(di= new Delayed_insert())) { my_error(ER_OUTOFMEMORY,MYF(0),sizeof(Delayed_insert)); thd->fatal_error(); @@ -1798,30 +1798,30 @@ bool delayed_get_table(THD *thd, TABLE_LIST *table_list) pthread_mutex_lock(&LOCK_thread_count); thread_count++; pthread_mutex_unlock(&LOCK_thread_count); - tmp->thd.set_db(table_list->db, strlen(table_list->db)); - tmp->thd.query= my_strdup(table_list->table_name,MYF(MY_WME)); - if (tmp->thd.db == NULL || tmp->thd.query == NULL) + di->thd.set_db(table_list->db, strlen(table_list->db)); + di->thd.query= my_strdup(table_list->table_name, MYF(MY_WME)); + if (di->thd.db == NULL || di->thd.query == NULL) { /* The error is reported */ - delete tmp; + delete di; thd->fatal_error(); goto end_create; } - tmp->table_list= *table_list; // Needed to open table + di->table_list= *table_list; // Needed to open table /* Replace volatile strings with local copies */ - tmp->table_list.alias= tmp->table_list.table_name= tmp->thd.query; - tmp->table_list.db= tmp->thd.db; - tmp->lock(); - pthread_mutex_lock(&tmp->mutex); - if ((error=pthread_create(&tmp->thd.real_id,&connection_attrib, - handle_delayed_insert,(void*) tmp))) + di->table_list.alias= di->table_list.table_name= di->thd.query; + di->table_list.db= di->thd.db; + di->lock(); + pthread_mutex_lock(&di->mutex); + if ((error= pthread_create(&di->thd.real_id, &connection_attrib, + handle_delayed_insert, (void*) di))) { DBUG_PRINT("error", ("Can't create thread to handle delayed insert (error %d)", error)); - pthread_mutex_unlock(&tmp->mutex); - tmp->unlock(); - delete tmp; + pthread_mutex_unlock(&di->mutex); + di->unlock(); + delete di; my_error(ER_CANT_CREATE_THREAD, MYF(0), error); thd->fatal_error(); goto end_create; @@ -1829,15 +1829,15 @@ bool delayed_get_table(THD *thd, TABLE_LIST *table_list) /* Wait until table is open */ thd->proc_info="waiting for handler open"; - while (!tmp->thd.killed && !tmp->table && !thd->killed) + while (!di->thd.killed && !di->table && !thd->killed) { - pthread_cond_wait(&tmp->cond_client,&tmp->mutex); + pthread_cond_wait(&di->cond_client, &di->mutex); } - pthread_mutex_unlock(&tmp->mutex); + pthread_mutex_unlock(&di->mutex); thd->proc_info="got old table"; - if (tmp->thd.killed) + if (di->thd.killed) { - if (tmp->thd.net.report_error) + if (di->thd.net.report_error) { /* Copy the error message. Note that we don't treat fatal @@ -1845,34 +1845,34 @@ bool delayed_get_table(THD *thd, TABLE_LIST *table_list) main thread. Use of my_message will enable stored procedures continue handlers. */ - my_message(tmp->thd.net.last_errno, tmp->thd.net.last_error, + my_message(di->thd.net.last_errno, di->thd.net.last_error, MYF(0)); } - tmp->unlock(); + di->unlock(); goto end_create; } if (thd->killed) { - tmp->unlock(); + di->unlock(); goto end_create; } pthread_mutex_lock(&LOCK_delayed_insert); - delayed_threads.append(tmp); + delayed_threads.append(di); pthread_mutex_unlock(&LOCK_delayed_insert); } pthread_mutex_unlock(&LOCK_delayed_create); } - pthread_mutex_lock(&tmp->mutex); - table_list->table= tmp->get_local_table(thd); - pthread_mutex_unlock(&tmp->mutex); + pthread_mutex_lock(&di->mutex); + table_list->table= di->get_local_table(thd); + pthread_mutex_unlock(&di->mutex); if (table_list->table) { DBUG_ASSERT(thd->net.report_error == 0); - thd->di=tmp; + thd->di= di; } /* Unlock the delayed insert object after its last access. */ - tmp->unlock(); + di->unlock(); DBUG_RETURN(table_list->table == NULL); end_create: @@ -2102,26 +2102,26 @@ void kill_delayed_threads(void) VOID(pthread_mutex_lock(&LOCK_delayed_insert)); // For unlink from list I_List_iterator it(delayed_threads); - Delayed_insert *tmp; - while ((tmp=it++)) + Delayed_insert *di; + while ((di= it++)) { - tmp->thd.killed= THD::KILL_CONNECTION; - if (tmp->thd.mysys_var) + di->thd.killed= THD::KILL_CONNECTION; + if (di->thd.mysys_var) { - pthread_mutex_lock(&tmp->thd.mysys_var->mutex); - if (tmp->thd.mysys_var->current_cond) + pthread_mutex_lock(&di->thd.mysys_var->mutex); + if (di->thd.mysys_var->current_cond) { /* We need the following test because the main mutex may be locked in handle_delayed_insert() */ - if (&tmp->mutex != tmp->thd.mysys_var->current_mutex) - pthread_mutex_lock(tmp->thd.mysys_var->current_mutex); - pthread_cond_broadcast(tmp->thd.mysys_var->current_cond); - if (&tmp->mutex != tmp->thd.mysys_var->current_mutex) - pthread_mutex_unlock(tmp->thd.mysys_var->current_mutex); + if (&di->mutex != di->thd.mysys_var->current_mutex) + pthread_mutex_lock(di->thd.mysys_var->current_mutex); + pthread_cond_broadcast(di->thd.mysys_var->current_cond); + if (&di->mutex != di->thd.mysys_var->current_mutex) + pthread_mutex_unlock(di->thd.mysys_var->current_mutex); } - pthread_mutex_unlock(&tmp->thd.mysys_var->mutex); + pthread_mutex_unlock(&di->thd.mysys_var->mutex); } } VOID(pthread_mutex_unlock(&LOCK_delayed_insert)); // For unlink from list