From e223c32077788ca9d398efa23f2431eba4b3e7fc Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 11 Dec 2007 15:32:10 +0100 Subject: Bug#30273 - merge tables: Can't lock file (errno: 155) The patch for Bug 26379 (Combination of FLUSH TABLE and REPAIR TABLE corrupts a MERGE table) fixed this bug too. However it revealed a new bug that crashed the server. Flushing a merge table at the moment when it is between open and attach of children crashed the server. The flushing thread wants to abort locks on the flushed table. It calls ha_myisammrg::lock_count() and ha_myisammrg::store_lock() on the TABLE object of the other thread. Changed ha_myisammrg::lock_count() and ha_myisammrg::store_lock() to accept non-attached children. ha_myisammrg::lock_count() returns the number of MyISAM tables in the MERGE table so that the memory allocation done by get_lock_data() is done correctly, even if the children become attached before ha_myisammrg::store_lock() is called. ha_myisammrg::store_lock() will not return any lock if the children are not attached. This is however a change in the handler interface. lock_count() can now return a higher number than store_lock() stores locks. This is more safe than the reverse implementation would be. get_lock_data() in the SQL layer is adjusted accordingly. It sets MYSQL_LOCK::lock_count based on the number of locks returned by the handler::store_lock() calls, not based on the numbers returned by the handler::lock_count() calls. The latter are only used for allocation of memory now. No test case. The test suite cannot reliably run FLUSH between lock_count() and store_lock() of another thread. The bug report contains a program that can repeat the problem with some probability. include/myisammrg.h: Bug#30273 - merge tables: Can't lock file (errno: 155) Added mutex to struct st_myrg_info (MYRG_INFO). sql/handler.h: Bug#30273 - merge tables: Can't lock file (errno: 155) Extended comments for handler::lock_count() and handler::store_lock(). sql/lock.cc: Bug#30273 - merge tables: Can't lock file (errno: 155) Changed get_lock_data() so that the final lock_count is taken from the number of locks returned from handler::store_lock() instead of from handler::lock_count(). sql/sql_base.cc: Fixed a purecov comment. (unrelated to the rest of the changeset) storage/myisammrg/ha_myisammrg.cc: Bug#30273 - merge tables: Can't lock file (errno: 155) Changed ha_myisammrg::lock_count() and ha_myisammrg::store_lock() to accept non-attached children. Protected ha_myisammrg::store_lock() by MYRG_INFO::mutex. storage/myisammrg/myrg_close.c: Bug#30273 - merge tables: Can't lock file (errno: 155) Added MYRG_INFO::mutex destruction to myrg_parent_close(). storage/myisammrg/myrg_open.c: Bug#30273 - merge tables: Can't lock file (errno: 155) Added MYRG_INFO::mutex initialization to myrg_parent_open(). Protected myrg_attach_children() and myrg_detach_children() by MYRG_INFO::mutex. Fixed a purecov comment. (unrelated to the rest of the changeset) --- storage/myisammrg/ha_myisammrg.cc | 31 +++++++++++++++++++++++++++++-- storage/myisammrg/myrg_close.c | 1 + storage/myisammrg/myrg_open.c | 17 ++++++++++++++++- 3 files changed, 46 insertions(+), 3 deletions(-) (limited to 'storage/myisammrg') diff --git a/storage/myisammrg/ha_myisammrg.cc b/storage/myisammrg/ha_myisammrg.cc index f91b0dc7a92..4d3d8362a03 100644 --- a/storage/myisammrg/ha_myisammrg.cc +++ b/storage/myisammrg/ha_myisammrg.cc @@ -901,7 +901,14 @@ int ha_myisammrg::external_lock(THD *thd, int lock_type) uint ha_myisammrg::lock_count(void) const { - DBUG_ASSERT(this->file->children_attached); + /* + Return the real lock count even if the children are not attached. + This method is used for allocating memory. If we would return 0 + to another thread (e.g. doing FLUSH TABLE), and attach the children + before the other thread calls store_lock(), then we would return + more locks in store_lock() than we claimed by lock_count(). The + other tread would overrun its memory. + */ return file->tables; } @@ -911,7 +918,24 @@ THR_LOCK_DATA **ha_myisammrg::store_lock(THD *thd, enum thr_lock_type lock_type) { MYRG_TABLE *open_table; - DBUG_ASSERT(this->file->children_attached); + + /* + This method can be called while another thread is attaching the + children. If the processor reorders instructions or write to memory, + 'children_attached' could be set before 'open_tables' has all the + pointers to the children. Use of a mutex here and in + myrg_attach_children() forces consistent data. + */ + pthread_mutex_lock(&this->file->mutex); + + /* + When MERGE table is open, but not yet attached, other threads + could flush it, which means call mysql_lock_abort_for_thread() + on this threads TABLE. 'children_attached' is FALSE in this + situaton. Since the table is not locked, return no lock data. + */ + if (!this->file->children_attached) + goto end; /* purecov: tested */ for (open_table=file->open_tables ; open_table != file->end_table ; @@ -921,6 +945,9 @@ THR_LOCK_DATA **ha_myisammrg::store_lock(THD *thd, if (lock_type != TL_IGNORE && open_table->table->lock.type == TL_UNLOCK) open_table->table->lock.type=lock_type; } + + end: + pthread_mutex_unlock(&this->file->mutex); return to; } diff --git a/storage/myisammrg/myrg_close.c b/storage/myisammrg/myrg_close.c index b402a35a253..97216ed47fe 100644 --- a/storage/myisammrg/myrg_close.c +++ b/storage/myisammrg/myrg_close.c @@ -58,6 +58,7 @@ int myrg_close(MYRG_INFO *info) pthread_mutex_lock(&THR_LOCK_open); myrg_open_list=list_delete(myrg_open_list,&info->open_list); pthread_mutex_unlock(&THR_LOCK_open); + VOID(pthread_mutex_destroy(&info->mutex)); my_free((uchar*) info,MYF(0)); if (error) { diff --git a/storage/myisammrg/myrg_open.c b/storage/myisammrg/myrg_open.c index 6fb1888f84d..b5002116164 100644 --- a/storage/myisammrg/myrg_open.c +++ b/storage/myisammrg/myrg_open.c @@ -33,7 +33,7 @@ myrg_attach_children(). Please duplicate changes in these functions or make common sub-functions. */ -/* purecov: begin unused */ +/* purecov: begin deadcode */ /* not used in MySQL server */ MYRG_INFO *myrg_open(const char *name, int mode, int handle_locking) { @@ -171,6 +171,7 @@ MYRG_INFO *myrg_open(const char *name, int mode, int handle_locking) VOID(my_close(fd,MYF(0))); end_io_cache(&file); + VOID(pthread_mutex_init(&m_info->mutex, MY_MUTEX_INIT_FAST)); m_info->open_list.data=(void*) m_info; pthread_mutex_lock(&THR_LOCK_open); myrg_open_list=list_add(myrg_open_list,&m_info->open_list); @@ -328,6 +329,7 @@ MYRG_INFO *myrg_parent_open(const char *parent_name, end_io_cache(&file_cache); VOID(my_close(fd, MYF(0))); + VOID(pthread_mutex_init(&m_info->mutex, MY_MUTEX_INIT_FAST)); m_info->open_list.data= (void*) m_info; pthread_mutex_lock(&THR_LOCK_open); @@ -393,6 +395,14 @@ int myrg_attach_children(MYRG_INFO *m_info, int handle_locking, DBUG_ENTER("myrg_attach_children"); DBUG_PRINT("myrg", ("handle_locking: %d", handle_locking)); + /* + This function can be called while another thread is trying to abort + locks of this MERGE table. If the processor reorders instructions or + write to memory, 'children_attached' could be set before + 'open_tables' has all the pointers to the children. Use of a mutex + here and in ha_myisammrg::store_lock() forces consistent data. + */ + pthread_mutex_lock(&m_info->mutex); rc= 1; errpos= 0; file_offset= 0; @@ -464,6 +474,7 @@ int myrg_attach_children(MYRG_INFO *m_info, int handle_locking, m_info->keys= min_keys; m_info->last_used_table= m_info->open_tables; m_info->children_attached= TRUE; + pthread_mutex_unlock(&m_info->mutex); DBUG_RETURN(0); err: @@ -473,6 +484,7 @@ err: my_free((char*) m_info->rec_per_key_part, MYF(0)); m_info->rec_per_key_part= NULL; } + pthread_mutex_unlock(&m_info->mutex); my_errno= save_errno; DBUG_RETURN(1); } @@ -494,6 +506,8 @@ err: int myrg_detach_children(MYRG_INFO *m_info) { DBUG_ENTER("myrg_detach_children"); + /* For symmetry with myrg_attach_children() we use the mutex here. */ + pthread_mutex_lock(&m_info->mutex); if (m_info->tables) { /* Do not attach/detach an empty child list. */ @@ -504,6 +518,7 @@ int myrg_detach_children(MYRG_INFO *m_info) m_info->del= 0; m_info->data_file_length= 0; m_info->options= 0; + pthread_mutex_unlock(&m_info->mutex); DBUG_RETURN(0); } -- cgit v1.2.1