From: Dmitry Lenev
Date: March 5 2011 8:02am
Subject: Re: bzr commit into mysql-5.5 branch (jon.hauglid:3372)
 Bug#11764779
List-Archive: http://lists.mysql.com/commits/132514
Message-Id: <20110305080247.GA13426@bandersnatch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

Hello, Jon Olav!

Here are my comments about your patch:

* Jon Olav Hauglid <jon.hauglid@stripped> [11/03/04 19:03]:
> #At file:///export/home/x/mysql-5.5-bug57649/ based on revid:jorgen.loland@stripped
> 
>  3372 Jon Olav Hauglid	2011-03-04
>       Bug #11764779 (former 57649)
>       FLUSH TABLES under FLUSH TABLES <list> WITH READ LOCK leads 
>       to assert failure.
>       
>       This assert was triggered if a statement tried up upgrade a metadata
>       lock with an active FLUSH TABLE <list> WITH READ LOCK. The assert 
>       checks that the connection already holds a global intention exclusive
>       metadata lock. However, FLUSH TABLE <list> WITH READ LOCK does not
>       acquire this lock in order to be compatible with FLUSH TABLES WITH
>       READ LOCK. Therefore any metadata lock upgrade caused the assert to
>       be triggered.
>       
>       This patch fixes the problem by preventing metadata lock upgrade
>       if the connection has an active FLUSH TABLE <list> WITH READ LOCK.
>       ER_TABLE_NOT_LOCKED_FOR_WRITE will instead be reported to the client.

...

> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc	2011-02-08 15:47:33 +0000
> +++ b/sql/sql_base.cc	2011-03-04 15:54:29 +0000
> @@ -3120,7 +3120,7 @@ TABLE *find_locked_table(TABLE *list, co
>     lock from the list of open tables, emit error if no such table
>     found.
>  
> -   @param list       List of TABLE objects to be searched
> +   @param thd        Thread context
>     @param db         Database name.
>     @param table_name Name of table.
>     @param no_error   Don't emit error if no suitable TABLE

IMO it makes sense add a note to the description of this function
mentioning that it also checks for the presence of global IX lock.

...

> @@ -3143,19 +3142,28 @@ TABLE *find_table_for_mdl_upgrade(TABLE
>        my_error(ER_TABLE_NOT_LOCKED, MYF(0), table_name);
>      return NULL;
>    }
> -  else
> +
> +  /*
> +    It is not safe to upgrade the metadata lock without GLOBAL IX lock.
> +    This can happen with FLUSH TABLES <list> WITH READ LOCK as we in these
> +    cases don't take a GLOBAL IX lock in order to be compatible with
> +    global read lock.
> +  */
> +  if (!thd->mdl_context.is_lock_owner(MDL_key::GLOBAL, "", "",
> +                                      MDL_INTENTION_EXCLUSIVE))
>    {
> -    while (tab->mdl_ticket != NULL &&
> -           !tab->mdl_ticket->is_upgradable_or_exclusive() &&
> -           (tab= find_locked_table(tab->next, db, table_name)))
> -      continue;
> -    if (!tab)
> -    {
> -      if (!no_error)
> -        my_error(ER_TABLE_NOT_LOCKED_FOR_WRITE, MYF(0), table_name);
> -      return 0;
> -    }
> +    my_error(ER_TABLE_NOT_LOCKED_FOR_WRITE, MYF(0), table_name);
> +    return NULL;
>    }

I think in this branch we should also respect value of no_error argument
and skip emitting error if it is set to true.

...

Otherwise I am OK with your patch and think that it can be pushed
after addressing/discussing the above issues.

Thank you for fixing this!!!

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

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