Merge stella.local:/home2/mydev/mysql-5.1-bug30273

into  stella.local:/home2/mydev/mysql-5.1-axmrg

include/myisammrg.h

@@ -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;
 
 

sql/handler.h

@@ -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,

sql/lock.cc

@@ -847,9 +847,6 @@ static MYSQL_LOCK *get_lock_data(THD *thd, TABLE **table_ptr, uint count,
   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 *thd, TABLE **table_ptr, uint count,
       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);
 }
 

sql/sql_base.cc

@@ -8414,7 +8414,7 @@ int abort_and_upgrade_lock(ALTER_PARTITION_PARAM_TYPE *lpt)
     We also downgrade locks after the upgrade to WRITE_ONLY
 */
 
-/* purecov: begin unused */
+/* purecov: begin deadcode */
 void close_open_tables_and_downgrade(ALTER_PARTITION_PARAM_TYPE *lpt)
 {
   VOID(pthread_mutex_lock(&LOCK_open));

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;
 }
 

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)
   {

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);
 }