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
