List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:October 7 2010 11:28am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch
(magne.mahre:3309) Bug#49177
View as plain text  
Hi Magne, Kostja,

On Thu, Oct 07, 2010 at 12:55:19PM +0400, Konstantin Osipov wrote:
> * 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...
+1

Though no need for nested loop here.

> 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.
Well, there is a comment justifying this behaviour: "Not exactly right...". :)

I can hardly imagine situation when mi_panic() is called simultaneously,
even more hardly can I imagine reasons why we should handle this situation.

At this point caller must have closed all table instances, no new opens
are allowed. It should be enforced by our plugins architecture. That means
we should be able to safely do DBUG_ASSERT(!myisam_open_list) here.

Unfortunately history is not verbose. I consider this loop either a dead-code
or as something that was supposed to be used out of MySQL project. Well, may be
it is just a kind of protection against MI_INFO objects leak.

Regards,
Sergey
-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
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