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

[Dmitri Lenev]

Hello Ingo!

Here is my comments about your patch:

* Ingo Struewing <ingo@stripped> [07/12/10 21:56]:
> 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.

I would also mentioned in the ChangeSet comment that to achieve
this you have changed handler interface slightly and that now 
handler::store_lock() can return less locks than were requested
by handler::lock_count() method and that SQL-layer code was
adjusted accordingly.

...

>   
>   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.
> 

...

> 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.

I think it is better to omit "This is however a change in the handler
interface." sentence from this comment since it will become irrelevant
after your patch will be pushed in the main tree.

> +
> +    @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.

IMO this @todo item, altough appropriate for the process of patch
review and discussion, is unnecessary in its final version and only
add to confusion. After all your patch tries to address it by
adjusting code in sql/lock.cc.

...

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

Since you are initializing this mutex here you should also destroy
it somewhere... Please add call to pthread_mutex_destroy() to 
appropriate place in code (probably to myrg_close()?).

...

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

Hmm... For the sake of consistency I would also added locking and
unlocking of MYRG_INFO::mutex to the myrg_detach_children().
Yes it is not strictly necessary as now one can't call this method
concurrently with ha_myisammrg::store_lock() thanks to LOCK_open mutex.
But it looks very confusing when we rely on one mutex in attach and on
another mutex in detach and at least deserves a comment. Since I don't
think that approach with two mutexes is really future-proof I would
prefer that you added locking/unlocking of MYRG_INFO::mutex to
the myrg_detach_children().

I think it is OK to push this patch after addressing/discussing
pthread_mutex_destroy() and myrg_detach_children() issues and
after considering other comments.

-- 
Dmitri Lenev, Software Developer
MySQL AB, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification