List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:October 7 2010 8:55am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)
Bug#49177
View as plain text  
* Magne Mahre <magne.mahre@stripped> [10/09/30 14:26]:
> === modified file 'storage/myisam/mi_panic.c'
> --- a/storage/myisam/mi_panic.c	2009-12-05 01:26:15 +0000
> +++ b/storage/myisam/mi_panic.c	2010-09-30 09:29:51 +0000
> @@ -26,54 +26,55 @@
>  int mi_panic(enum ha_panic_function flag)
>  {
>    int error=0;
> -  LIST *list_element,*next_open;
>    MI_INFO *info;
> +  uint idx;
>    DBUG_ENTER("mi_panic");
>  
>    mysql_mutex_lock(&THR_LOCK_myisam);
> -  for (list_element=myisam_open_list ; list_element ; list_element=next_open)
> +  if (myisam_open_cache)
>    {
> -    next_open=list_element->next;		/* Save if close */
> -    info=(MI_INFO*) list_element->data;
> -    switch (flag) {
> -    case HA_PANIC_CLOSE:
> -      mysql_mutex_unlock(&THR_LOCK_myisam);     /* Not exactly right... */
> -      if (mi_close(info))
> -	error=my_errno;
> -      mysql_mutex_lock(&THR_LOCK_myisam);
> -      break;
> -    case HA_PANIC_WRITE:		/* Do this to free databases */
> +    for (idx= 0; idx < myisam_open_cache->records; ++idx)
> +    {
> +      info=(MI_INFO*) my_hash_element(myisam_open_cache, idx);
> +      switch (flag) {
> +      case HA_PANIC_CLOSE:
> +        mysql_mutex_unlock(&THR_LOCK_myisam);     /* Not exactly right... */
> +        if (mi_close(info))
> +          error=my_errno;

I don't understand how it works at all. 

mi_close() will erase the table entry from the hash. This
invalidates the iterator. I think for () loop needs to be changed
to
while (some_table_deleted)
{
  for (idx= 0; idx < myisam_open_cache_records; ++idx)
  {
    continue;
  }
  some_table_deleted= false;
}

or alike...

Besides, in a fully concurrent environment, what is the guarantee
that mi_close() is not called twice on the same info? (The old
code seems to also have been prone to this bug).

I mean specifically when mi_panic is called simultaneously from
two threads:

thread1: take THR_LOCK_myisam, get the first record, release THR_LOCK_myisam
thread2: take THR_LOCK_myisam, get the first record, release THR_LOCK_myisam
thread1: mi_close first record
thread2: mi_close first record.

I think we need something like "panic_in_progress" to prevent
this situation, or add a comment that mi_panic should never be
called concurrently with itself.



-- 
Thread
bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309) Bug#49177Magne Mahre30 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)Bug#49177Konstantin Osipov7 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch(magne.mahre:3309) Bug#49177Sergey Vojtovich7 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)Bug#49177Magne Mæhre21 Oct
  • Re: bzr commit into mysql-next-mr-bugfixing branch(magne.mahre:3309) Bug#49177Sergey Vojtovich8 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)Bug#49177Magne Mæhre21 Oct