List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:August 23 2010 3:32pm
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (jon.hauglid:3193)
Bug#54332
View as plain text  
Hello!

Here is my review of your patch:

* Jon Olav Hauglid <jon.hauglid@stripped> [10/08/23 18:54]:
> #At file:///export/home/x/mysql-5.5-bugfixing-bug54332/ based on
> revid:chris.powers@stripped
> 
>  3193 Jon Olav Hauglid	2010-08-23
>       Bug #54332 Deadlock with two connections doing LOCK TABLE+INSERT DELAYED
>       
>       The problem was that deadlocks involving INSERT DELAYED were not detected.

...

> === modified file 'sql/sql_insert.cc'
> --- a/sql/sql_insert.cc	2010-08-20 08:24:32 +0000
> +++ b/sql/sql_insert.cc	2010-08-23 14:49:15 +0000
> @@ -548,10 +548,34 @@ bool open_and_lock_for_insert_delayed(TH
>      DBUG_RETURN(TRUE);
>    }
>  
> -  if (delayed_get_table(thd, table_list))
> +  /*
> +    In order for the deadlock detector to be able to find any deadlocks
> +    caused by the handler thread locking this table, we take the metadata
> +    lock inside the connection thread. If this goes ok, the ticket is cloned
> +    and added to the list of granted locks held by the handler thread.
> +  */
> +  MDL_ticket *mdl_savepoint= thd->mdl_context.mdl_savepoint();
> +  if (thd->mdl_context.acquire_lock(&table_list->mdl_request,
> +                                    thd->variables.lock_wait_timeout))
> +    /*
> +      If a lock can't be acquired, it makes no sense to try normal insert.
> +      Therefore we just abort the statement.
> +    */
>      DBUG_RETURN(TRUE);
>  
> -  if (table_list->table)
> +  /*
> +    If a lock was acquired above, we should release it after delayed_get_table()
> +    has cloned the ticket for the handler thread. Note that acquire_lock() can
> +    succeed because of a lock already held by the connection. In this case we
> +    should not release it here.
> +  */
> +  MDL_ticket *table_ticket = mdl_savepoint == thd->mdl_context.mdl_savepoint() ?
> +    NULL: thd->mdl_context.mdl_savepoint();
> +

I think that rather than rely on how MDL_context::mdl_savepoint() is implemented
it is better to use explicit MDL_context::has_lock() method instead. Please
consider doing such a change as a follow-up patch.

> +  bool error= FALSE;
> +  if (delayed_get_table(thd, table_list))
> +    error= TRUE;
> +  else if (table_list->table)
>    {
>      /*
>        Open tables used for sub-selects or in stored functions, will also
> @@ -560,16 +584,25 @@ bool open_and_lock_for_insert_delayed(TH
>      if (open_and_lock_tables(thd, table_list->next_global, TRUE, 0))
>      {
>        end_delayed_insert(thd);
> -      DBUG_RETURN(TRUE);
> +      error= TRUE;
> +    }
> +    else
> +    {
> +      /*
> +        First table was not processed by open_and_lock_tables(),
> +        we need to set updatability flag "by hand".
> +      */
> +      if (!table_list->derived && !table_list->view)
> +        table_list->updatable= 1;  // usual table
>      }
> -    /*
> -      First table was not processed by open_and_lock_tables(),
> -      we need to set updatability flag "by hand".
> -    */
> -    if (!table_list->derived && !table_list->view)
> -      table_list->updatable= 1;  // usual table
> -    DBUG_RETURN(FALSE);
>    }
> +
> +  if (table_ticket)
> +    thd->mdl_context.release_lock(table_ticket);
> +  table_list->mdl_request.ticket= NULL;
> +
> +  if (error || table_list->table)
> +    DBUG_RETURN(error);
>  #endif
>    /*
>      * This is embedded library and we don't have auxiliary
> @@ -2025,6 +2058,20 @@ bool delayed_get_table(THD *thd, TABLE_L
>        /* Replace volatile strings with local copies */
>        di->table_list.alias= di->table_list.table_name= di->thd.query();
>        di->table_list.db= di->thd.db;
> +
> +      /*
> +        Clone the ticket representing the lock on the target table for
> +        the insert and add it to the list of granted metadata locks held by
> +        the handler thread. This is safe since the handler thread is
> +        not holding nor waiting on any metadata locks.
> +      */
> +      if (di->thd.mdl_context.clone_ticket(&table_list->mdl_request))
> +      {
> +        delete di;
> +        my_error(ER_OUT_OF_RESOURCES, MYF(ME_FATALERROR));
> +        goto end_create;
> +      }
> +

It is not very obvious that clone_ticket() overwrites value of
TABLE_LIST::MDL_request::ticket. But I guess it is OK since it
is reset in delayed_get_table(). Maybe it makes sense to add
comment noting this here or to place where delayed_get_table()
does this?

>        di->lock();
>        mysql_mutex_lock(&di->mutex);
>        if ((error= mysql_thread_create(key_thread_delayed_insert,
> @@ -2036,6 +2083,7 @@ bool delayed_get_table(THD *thd, TABLE_L
>  		    error));
>          mysql_mutex_unlock(&di->mutex);
>  	di->unlock();
> +        di->thd.mdl_context.release_lock(table_list->mdl_request.ticket);
>  	delete di;
>  	my_error(ER_CANT_CREATE_THREAD, MYF(ME_FATALERROR), error);
>          goto end_create;
>

I think it is OK to push this patch after considering above points and
reporting problems with INSERT DELAYED + FLUSH TABLES and INSERT DELAYED
+ MERGE tables which we have discussed on IRC as a separate bugs.

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

Are you MySQL certified?  http://www.mysql.com/certification
Thread
bzr commit into mysql-5.5-bugfixing branch (jon.hauglid:3193) Bug#54332Jon Olav Hauglid23 Aug
  • Re: bzr commit into mysql-5.5-bugfixing branch (jon.hauglid:3193)Bug#54332Dmitry Lenev23 Aug