bk commit into 5.1 tree (istruewing:1.2678) BUG#30273

[Ingo Struewing]

Below is the list of changes that have just been committed into a local
5.1 repository of istruewing. When istruewing does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet@stripped, 2007-12-10 19:52:12+01:00, istruewing@stripped +5 -0
  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.
  
  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@stripped, 2007-12-10 19:52:10+01:00, istruewing@stripped +1 -0
    Bug#30273 - merge tables: Can't lock file (errno: 155)
    Added mutex to struct st_myrg_info (MYRG_INFO).

  sql/handler.h@stripped, 2007-12-10 19:52:10+01:00, istruewing@stripped +10 -1
    Bug#30273 - merge tables: Can't lock file (errno: 155)
    Extended comments for handler::lock_count() and
    handler::store_lock().

  sql/lock.cc@stripped, 2007-12-10 19:52:10+01:00, istruewing@stripped +17 -3
    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().

  storage/myisammrg/ha_myisammrg.cc@stripped, 2007-12-10 19:52:10+01:00,
istruewing@stripped +37 -2
    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_open.c@stripped, 2007-12-10 19:52:10+01:00, istruewing@stripped
+13 -1
    Bug#30273 - merge tables: Can't lock file (errno: 155)
    Added MYRG_INFO::mutex initialization to myrg_parent_open().
    Protected myrg_attach_children() by MYRG_INFO::mutex.
    Fixed purecov comment.

diff -Nrup a/include/myisammrg.h b/include/myisammrg.h
--- a/include/myisammrg.h	2007-11-15 20:25:40 +01:00
+++ b/include/myisammrg.h	2007-12-10 19:52:10 +01:00
@@ -74,6 +74,7 @@ typedef struct st_myrg_info
   LIST	 open_list;
   QUEUE  by_key;
   ulong *rec_per_key_part;			/* for sql optimizing */
+  pthread_mutex_t mutex;
 } MYRG_INFO;
 
 
diff -Nrup a/sql/handler.h b/sql/handler.h
--- a/sql/handler.h	2007-09-07 15:41:46 +02:00
+++ b/sql/handler.h	2007-12-10 19:52:10 +01:00
@@ -1637,13 +1637,22 @@ public:
   virtual int repair_partitions(THD *thd)
   { return HA_ERR_WRONG_COMMAND; }
 
-  /* lock_count() can be more than one if the table is a MERGE */
+  /**
+    @note lock_count() can return > 1 if the table is MERGE or partitioned.
+  */
   virtual uint lock_count(void) const { return 1; }
   /**
     Is not invoked for non-transactional temporary tables.
 
+    @note store_lock() can return more than one lock if the table is MERGE
+    or partitioned.
+
     @note that one can NOT rely on table->in_use in store_lock().  It may
     refer to a different thread if called from mysql_lock_abort_for_thread().
+
+    @note If the table is MERGE, store_lock() can return less locks
+    than lock_count() claimed. This can happen when the MERGE children
+    are not attached when this is called from another thread.
   */
   virtual THR_LOCK_DATA **store_lock(THD *thd,
 				     THR_LOCK_DATA **to,
diff -Nrup a/sql/lock.cc b/sql/lock.cc
--- a/sql/lock.cc	2007-08-27 15:19:53 +02:00
+++ b/sql/lock.cc	2007-12-10 19:52:10 +01:00
@@ -847,9 +847,6 @@ static MYSQL_LOCK *get_lock_data(THD *th
   locks= locks_buf= sql_lock->locks= (THR_LOCK_DATA**) (sql_lock + 1);
   to= table_buf= sql_lock->table= (TABLE**) (locks + tables * 2);
   sql_lock->table_count=lock_count;
-  sql_lock->lock_count=tables;
-  DBUG_PRINT("info", ("sql_lock->table_count %d sql_lock->lock_count %d",
-                      sql_lock->table_count, sql_lock->lock_count));
 
   for (i=0 ; i < count ; i++)
   {
@@ -889,6 +886,23 @@ static MYSQL_LOCK *get_lock_data(THD *th
       for ( ; org_locks != locks ; org_locks++)
 	(*org_locks)->debug_print_param= (void *) table;
   }
+  /*
+    We do not use 'tables', because there are cases where store_lock()
+    returns less locks than lock_count() claimed. This can happen when
+    a FLUSH TABLES tries to abort locks from a MERGE table of another
+    thread. When that thread has just opened the table, but not yet
+    attached its children, it cannot return the locks. lock_count()
+    always returns the number of locks that an attached table has.
+    This is done to avoid the reverse situation: If lock_count() would
+    return 0 for a non-attached MERGE table, and that table becomes
+    attached between the calls to lock_count() and store_lock(), then
+    we would have allocated too little memory for the lock data. Now
+    we may allocate too much, but better safe than memory overrun.
+    And in the FLUSH case, the memory is released quickly anyway.
+  */
+  sql_lock->lock_count= locks - locks_buf;
+  DBUG_PRINT("info", ("sql_lock->table_count %d sql_lock->lock_count %d",
+                      sql_lock->table_count, sql_lock->lock_count));
   DBUG_RETURN(sql_lock);
 }
 
diff -Nrup a/storage/myisammrg/ha_myisammrg.cc b/storage/myisammrg/ha_myisammrg.cc
--- a/storage/myisammrg/ha_myisammrg.cc	2007-11-15 20:25:41 +01:00
+++ b/storage/myisammrg/ha_myisammrg.cc	2007-12-10 19:52:10 +01:00
@@ -901,7 +901,22 @@ int ha_myisammrg::external_lock(THD *thd
 
 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.
+
+    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.
+
+    @todo If matching numbers are vital, then attaching of MERGE
+    children must be done under LOCK_open, and lock_count() return 0
+    when the children are not attached.
+  */
   return file->tables;
 }
 
@@ -911,7 +926,24 @@ THR_LOCK_DATA **ha_myisammrg::store_lock
 					 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;
 
   for (open_table=file->open_tables ;
        open_table != file->end_table ;
@@ -921,6 +953,9 @@ THR_LOCK_DATA **ha_myisammrg::store_lock
     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 -Nrup a/storage/myisammrg/myrg_open.c b/storage/myisammrg/myrg_open.c
--- a/storage/myisammrg/myrg_open.c	2007-11-15 20:25:41 +01:00
+++ b/storage/myisammrg/myrg_open.c	2007-12-10 19:52:10 +01:00
@@ -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, i
 
   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 *
 
   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_in
   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_in
   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);
 }