A prerequisite patch for the fix for Bug#46224

"HANDLER statements within a transaction might lead to deadlocks".
Introduce a notion of a sentinel to MDL_context. A sentinel
is a ticket that separates all tickets in the context into two
groups: before and after it. Currently we can have (and need) only
one designated sentinel -- it separates all locks taken by LOCK
TABLE or HANDLER statement, which must survive COMMIT and ROLLBACK
and all other locks, which must be released at COMMIT or ROLLBACK.
The tricky part is maintaining the sentinel up to date when
someone release its corresponding ticket. This can happen, e.g.
if someone issues DROP TABLE under LOCK TABLES (generally,
see all calls to release_all_locks_for_name()).
MDL_context::release_ticket() is modified to take care of it.

******
A fix and a test case for Bug#46224 "HANDLER statements within a
transaction might lead to deadlocks".

An attempt to mix HANDLER SQL statements, which are transaction-
agnostic, an open multi-statement transaction,
and DDL against the involved tables (in a concurrent connection) 
could lead to a deadlock. The deadlock would occur when
HANDLER OPEN or HANDLER READ would have to wait on a conflicting
metadata lock. If the connection that issued HANDLER statement
also had other metadata locks (say, acquired in scope of a 
transaction), a classical deadlock situation of mutual wait
could occur.

Incompatible change: entering LOCK TABLES mode automatically
closes all open HANDLERs in the current connection.

Incompatible change: previously an attempt to wait on a lock
in a connection that has an open HANDLER statement could wait
indefinitely/deadlock. After this patch, an error ER_LOCK_DEADLOCK
is produced.

The idea of the fix is to merge thd->handler_mdl_context
with the main mdl_context of the connection, used for transactional
locks. This makes deadlock detection possible, since all waits
with locks are "visible" and available to analysis in a single
MDL context of the connection.

Since HANDLER locks and transactional locks have a different life
cycle -- HANDLERs are explicitly open and closed, and so
are HANDLER locks, explicitly acquired and released, whereas
transactional locks "accumulate" till the end of a transaction
and are released only with COMMIT, ROLLBACK and ROLLBACK TO SAVEPOINT,
a concept of "sentinel" was introduced to MDL_context.
All locks, HANDLER and others, reside in the same linked list.
However, a selected element of the list separates locks with
different life cycle. HANDLER locks always reside at the
end of the list, after the sentinel. Transactional locks are
prepended to the beginning of the list, before the sentinel.
Thus, ROLLBACK, COMMIT or ROLLBACK TO SAVEPOINT, only
release those locks that reside before the sentinel. HANDLER locks
must be released explicitly as part of HANDLER CLOSE statement,
or an implicit close. 
The same approach with sentinel
is also employed for LOCK TABLES locks. Since HANDLER and LOCK TABLES
statement has never worked together, the implementation is
made simple and only maintains one sentinel, which is used either
for HANDLER locks, or for LOCK TABLES locks.

1 files changed, 81 insertions, 124 deletions

diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc
index 94f5e84fb10..ccfe21d1af5 100644
--- a/sql/sql_handler.cc
+++ b/sql/sql_handler.cc
@@ -33,27 +33,21 @@
 */
 
 /*
-  There are two containers holding information about open handler tables.
-  The first is 'thd->handler_tables'. It is a linked list of TABLE objects.
-  It is used like 'thd->open_tables' in the table cache. The trick is to
-  exchange these two lists during open and lock of tables. Thus the normal
-  table cache code can be used.
-  The second container is a HASH. It holds objects of the type TABLE_LIST.
-  Despite its name, no lists of tables but only single structs are hashed
-  (the 'next' pointer is always NULL). The reason for theis second container
-  is, that we want handler tables to survive FLUSH TABLE commands. A table
-  affected by FLUSH TABLE must be closed so that other threads are not
-  blocked by handler tables still in use. Since we use the normal table cache
-  functions with 'thd->handler_tables', the closed tables are removed from
-  this list. Hence we need the original open information for the handler
-  table in the case that it is used again. This information is handed over
-  to mysql_ha_open() as a TABLE_LIST. So we store this information in the
-  second container, where it is not affected by FLUSH TABLE. The second
-  container is implemented as a hash for performance reasons. Consequently,
-  we use it not only for re-opening a handler table, but also for the
-  HANDLER ... READ commands. For this purpose, we store a pointer to the
-  TABLE structure (in the first container) in the TBALE_LIST object in the
-  second container. When the table is flushed, the pointer is cleared.
+  The information about open HANDLER objects is stored in a HASH.
+  It holds objects of type TABLE_LIST, which are indexed by table
+  name/alias, and allows us to quickly find a HANDLER table for any
+  operation at hand - be it HANDLER READ or HANDLER CLOSE.
+
+  It also allows us to maintain an "open" HANDLER even in cases
+  when there is no physically open cursor. E.g. a FLUSH TABLE
+  statement in this or some other connection demands that all open
+  HANDLERs against the flushed table are closed. In order to
+  preserve the information about an open HANDLER, we don't perform
+  a complete HANDLER CLOSE, but only close the TABLE object.  The
+  corresponding TABLE_LIST is kept in the cache with 'table'
+  pointer set to NULL. The table will be reopened on next access
+  (this, however, leads to loss of cursor position, unless the
+  cursor points at the first record).
 */
 
 #include "mysql_priv.h"
@@ -124,32 +118,19 @@ static void mysql_ha_hash_free(TABLE_LIST *tables)
 
 static void mysql_ha_close_table(THD *thd, TABLE_LIST *tables)
 {
-  TABLE **table_ptr;
-  MDL_ticket *mdl_ticket;
 
-  /*
-    Though we could take the table pointer from hash_tables->table,
-    we must follow the thd->handler_tables chain anyway, as we need the
-    address of the 'next' pointer referencing this table
-    for close_thread_table().
-  */
-  for (table_ptr= &(thd->handler_tables);
-       *table_ptr && (*table_ptr != tables->table);
-         table_ptr= &(*table_ptr)->next)
-    ;
-
-  if (*table_ptr)
+  if (tables->table && !tables->table->s->tmp_table)
   {
-    (*table_ptr)->file->ha_index_or_rnd_end();
-    mdl_ticket= (*table_ptr)->mdl_ticket;
+    /* Non temporary table. */
+    tables->table->file->ha_index_or_rnd_end();
     pthread_mutex_lock(&LOCK_open);
-    if (close_thread_table(thd, table_ptr))
+    if (close_thread_table(thd, &tables->table))
     {
       /* Tell threads waiting for refresh that something has happened */
       broadcast_refresh();
     }
     pthread_mutex_unlock(&LOCK_open);
-    thd->handler_mdl_context.release_lock(mdl_ticket);
+    thd->mdl_context.release_lock(tables->mdl_request.ticket);
   }
   else if (tables->table)
   {
@@ -158,6 +139,7 @@ static void mysql_ha_close_table(THD *thd, TABLE_LIST *tables)
     table->file->ha_index_or_rnd_end();
     table->query_id= thd->query_id;
     table->open_by_handler= 0;
+    mark_tmp_table_for_reuse(table);
   }
 
   /* Mark table as closed, ready for re-open if necessary. */
@@ -195,7 +177,7 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen)
   uint          dblen, namelen, aliaslen, counter;
   bool          error;
   TABLE         *backup_open_tables;
-  MDL_context   backup_mdl_context;
+  MDL_ticket    *mdl_savepoint;
   DBUG_ENTER("mysql_ha_open");
   DBUG_PRINT("enter",("'%s'.'%s' as '%s'  reopen: %d",
                       tables->db, tables->table_name, tables->alias,
@@ -265,6 +247,8 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen)
     memcpy(hash_tables->table_name, tables->table_name, namelen);
     memcpy(hash_tables->alias, tables->alias, aliaslen);
     hash_tables->mdl_request.init(MDL_key::TABLE, db, name, MDL_SHARED);
+    /* for now HANDLER can be used only for real TABLES */
+    hash_tables->required_type= FRMTYPE_TABLE;
 
     /* add to hash */
     if (my_hash_insert(&thd->handler_tables_hash, (uchar*) hash_tables))
@@ -283,16 +267,11 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen)
     from open_tables(), thd->open_tables will contain only the opened
     table.
 
-    The thd->handler_tables list is kept as-is to avoid deadlocks if
-    open_table(), called by open_tables(), needs to back-off because
-    of a pending exclusive metadata lock or flush for the table being
-    opened.
-
     See open_table() back-off comments for more details.
   */
   backup_open_tables= thd->open_tables;
   thd->open_tables= NULL;
-  thd->mdl_context.backup_and_reset(&backup_mdl_context);
+  mdl_savepoint= thd->mdl_context.mdl_savepoint();
 
   /*
     open_tables() will set 'hash_tables->table' if successful.
@@ -300,53 +279,47 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen)
   */
   DBUG_ASSERT(! hash_tables->table);
 
-  /* for now HANDLER can be used only for real TABLES */
-  hash_tables->required_type= FRMTYPE_TABLE;
   /*
     We use open_tables() here, rather than, say,
     open_ltable() or open_table() because we would like to be able
     to open a temporary table.
   */
   error= open_tables(thd, &hash_tables, &counter, 0);
-  if (thd->open_tables)
+
+  if (! error &&
+      ! (hash_tables->table->file->ha_table_flags() & HA_CAN_SQL_HANDLER))
   {
-    if (thd->open_tables->next)
-    {
-      /*
-        We opened something that is more than a single table.
-        This happens with MERGE engine. Don't try to link
-        this mess into thd->handler_tables list, close it
-        and report an error. We must do it right away
-        because mysql_ha_close_table(), called down the road,
-        can close a single table only.
-      */
-      close_thread_tables(thd);
-      thd->mdl_context.release_all_locks();
-      my_error(ER_ILLEGAL_HA, MYF(0), hash_tables->alias);
-      error= TRUE;
-    }
-    else
-    {
-      /* Merge the opened table into handler_tables list. */
-      thd->open_tables->next= thd->handler_tables;
-      thd->handler_tables= thd->open_tables;
-    }
+    my_error(ER_ILLEGAL_HA, MYF(0), tables->alias);
+    error= TRUE;
+  }
+  if (!error &&
+      hash_tables->mdl_request.ticket &&
+      thd->mdl_context.has_lock(mdl_savepoint,
+                                hash_tables->mdl_request.ticket))
+  {
+    /* The ticket returned is within a savepoint. Make a copy.  */
+    error= thd->mdl_context.clone_ticket(&hash_tables->mdl_request);
+    hash_tables->table->mdl_ticket= hash_tables->mdl_request.ticket;
   }
-  thd->handler_mdl_context.merge(&thd->mdl_context);
-
-  thd->open_tables= backup_open_tables;
-  thd->mdl_context.restore_from_backup(&backup_mdl_context);
-
   if (error)
-    goto err;
-
-  /* There can be only one table in '*tables'. */
-  if (! (hash_tables->table->file->ha_table_flags() & HA_CAN_SQL_HANDLER))
   {
-    my_error(ER_ILLEGAL_HA, MYF(0), tables->alias);
-    goto err;
+    close_thread_tables(thd);
+    thd->open_tables= backup_open_tables;
+    thd->mdl_context.rollback_to_savepoint(mdl_savepoint);
+    if (!reopen)
+      my_hash_delete(&thd->handler_tables_hash, (uchar*) hash_tables);
+    else
+      hash_tables->table= NULL;
+    DBUG_PRINT("exit",("ERROR"));
+    DBUG_RETURN(TRUE);
   }
+  thd->open_tables= backup_open_tables;
+  if (hash_tables->mdl_request.ticket)
+    thd->mdl_context.
+      move_ticket_after_lt_or_ha_sentinel(hash_tables->mdl_request.ticket);
 
+  /* Assert that the above check prevent opening of views and merge tables. */
+  DBUG_ASSERT(hash_tables->table->next == NULL);
   /*
     If it's a temp table, don't reset table->query_id as the table is
     being used by this handler. Otherwise, no meaning at all.
@@ -357,14 +330,6 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen)
     my_ok(thd);
   DBUG_PRINT("exit",("OK"));
   DBUG_RETURN(FALSE);
-
-err:
-  if (hash_tables->table)
-    mysql_ha_close_table(thd, hash_tables);
-  if (!reopen)
-    my_hash_delete(&thd->handler_tables_hash, (uchar*) hash_tables);
-  DBUG_PRINT("exit",("ERROR"));
-  DBUG_RETURN(TRUE);
 }
 
 
@@ -496,59 +461,41 @@ retry:
                          hash_tables->db, hash_tables->table_name,
                          hash_tables->alias, table));
     }
-    table->pos_in_table_list= tables;
-#if MYSQL_VERSION_ID < 40100
-    if (*tables->db && strcmp(table->table_cache_key, tables->db))
-    {
-      DBUG_PRINT("info",("wrong db"));
-      table= NULL;
-    }
-#endif
   }
   else
     table= NULL;
 
   if (!table)
   {
-#if MYSQL_VERSION_ID < 40100
-    char buff[MAX_DBKEY_LENGTH];
-    if (*tables->db)
-      strxnmov(buff, sizeof(buff)-1, tables->db, ".", tables->table_name,
-               NullS);
-    else
-      strncpy(buff, tables->alias, sizeof(buff));
-    my_error(ER_UNKNOWN_TABLE, MYF(0), buff, "HANDLER");
-#else
     my_error(ER_UNKNOWN_TABLE, MYF(0), tables->alias, "HANDLER");
-#endif
     goto err0;
   }
-  tables->table=table;
 
   /* save open_tables state */
   backup_open_tables= thd->open_tables;
+  /* Always a one-element list, see mysql_ha_open(). */
+  DBUG_ASSERT(hash_tables->table->next == NULL);
   /*
     mysql_lock_tables() needs thd->open_tables to be set correctly to
-    be able to handle aborts properly. When the abort happens, it's
-    safe to not protect thd->handler_tables because it won't close any
-    tables.
+    be able to handle aborts properly.
   */
-  thd->open_tables= thd->handler_tables;
+  thd->open_tables= hash_tables->table;
+
 
-  lock= mysql_lock_tables(thd, &tables->table, 1, 0, &need_reopen);
+  lock= mysql_lock_tables(thd, &thd->open_tables, 1, 0, &need_reopen);
 
-  /* restore previous context */
+  /*
+    In 5.1 and earlier, mysql_lock_tables() could replace the TABLE
+    object with another one (reopen it). This is no longer the case
+    with new MDL.
+  */
+  DBUG_ASSERT(hash_tables->table == thd->open_tables);
+  /* Restore previous context. */
   thd->open_tables= backup_open_tables;
 
   if (need_reopen)
   {
     mysql_ha_close_table(thd, hash_tables);
-    /*
-      The lock might have been aborted, we need to manually reset
-      thd->some_tables_deleted because handler's tables are closed
-      in a non-standard way. Otherwise we might loop indefinitely.
-    */
-    thd->some_tables_deleted= 0;
     goto retry;
   }
 
@@ -556,7 +503,8 @@ retry:
     goto err0; // mysql_lock_tables() printed error message already
 
   // Always read all columns
-  tables->table->read_set= &tables->table->s->all_set;
+  hash_tables->table->read_set= &hash_tables->table->s->all_set;
+  tables->table= hash_tables->table;
 
   if (cond)
   {
@@ -811,6 +759,14 @@ void mysql_ha_flush(THD *thd)
 
   safe_mutex_assert_not_owner(&LOCK_open);
 
+  /*
+    Don't try to flush open HANDLERs when we're working with
+    system tables. The main MDL context is backed up and we can't
+    properly release HANDLER locks stored there.
+  */
+  if (thd->state_flags & Open_tables_state::BACKUPS_AVAIL)
+    DBUG_VOID_RETURN;
+
   for (uint i= 0; i < thd->handler_tables_hash.records; i++)
   {
     hash_tables= (TABLE_LIST*) my_hash_element(&thd->handler_tables_hash, i);
@@ -818,9 +774,10 @@ void mysql_ha_flush(THD *thd)
       TABLE::mdl_ticket is 0 for temporary tables so we need extra check.
     */
     if (hash_tables->table &&
-        (hash_tables->table->mdl_ticket &&
-         hash_tables->table->mdl_ticket->has_pending_conflicting_lock() ||
-         hash_tables->table->s->needs_reopen()))
+        ((hash_tables->table->mdl_ticket &&
+         hash_tables->table->mdl_ticket->has_pending_conflicting_lock()) ||
+         (!hash_tables->table->s->tmp_table &&
+          hash_tables->table->s->needs_reopen())))
       mysql_ha_close_table(thd, hash_tables);
   }