Re: bk commit into 5.2 tree (davi:1.2605) BUG#25858

[Konstantin Osipov]

* Davi Arnaut <davi@stripped> [07/09/06 08:14]:
> ChangeSet@stripped, 2007-09-06 00:54:06-03:00, davi@stripped +21 -0
>   Bug#25858 Some DROP TABLE under LOCK TABLES can cause deadlocks
>   
>   When a client (connection) holds a lock on a table and attempts to
>   drop (obtain a exclusive lock) on a second table that is already held
>   by a second client and the second client then attempts to drop the table
>   that is held by the first client, leads to a circular wait deadlock. This
>   scenario is very similar to trying to drop (or rename) a table while
>   holding read locks and are correctly forbidden.
>   
>   The solution is to allow a drop table operation to continue only if the
>   table being dropped is write (exclusively) locked, or if the table is
>   temporary, or if the client is not holding any locks. Using this scheme
>   prevents the creation of a circular chain in which each client is waiting
>   for one table that the next client in the chain is holding.
>   
>   This is incompatible change, as can be seen by number of tests cases
>   that needed to be fixed, but is consistent with respect to behavior of
>   the different scenarios in which the circular wait might happen.

This patch is good and does the job.

However, the deadlock is potentially present wherever we call
lock_table_name() under LOCK TABLES for any table that is not in the locked
list.

Could you perhaps move the part that checks that the table is in
the list of LOCK TABLES to lock_table_name?

Here's how it could look like:

  for (table=(TABLE*) hash_first(&open_cache, (byte*)key, key_length, &state);
       table ;
       table = (TABLE*) hash_next(&open_cache, (byte*)key, key_length, &state))
    if (table->in_use == thd)
    {
--> here
      if (table->reginfo.lock_type < TL_WRITE)
      {
        found_locked_table= TRUE;
        continue;
      }
      DBUG_RETURN(0);
    }

--> here
  if (thd->locked_tables)
  {
     if (found_locked_table)
       my_message("table has %s a READ lock and we need a WRITE lock");
     else
       my_message("table %s was not locked with lock tables");
     DBUG_RETURN(1);
  }

  if (!(table= table_cache_insert_placeholder(thd, key, key_length)))
    DBUG_RETURN(-1);


I'm eager to hear what that would break and why it won't work.

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY