A new implementation for the TABLE_SHARE cache in MDL

subsystem. Fix a number of caveates that the previous
implementation suffered from, including unprotected
access to shared data and lax resource accounting
(share->ref_count) that could lead to deadlocks.

The new implementation still suffers from a number
of potential deadlocks in some edge cases, and this is 
still not enabled by default. Especially since performance
testing has shown that it gives only marginable (not even 
exceeding measuring accuracy) improvements.

@todo: 
- Remove calls to close_cached_tables() with REFRESH_FAST,
and have_lock, because they break the MDL cache. 
- rework FLUSH TABLES <list> to not use close_cached_tables()
- make sure that whenever we set TABLE_SHARE::version to
0 we free MDL cache references to it.


sql/mdl.cc:
  We may cache references to TABLE_SHARE objects in 
  MDL_lock objects for tables. Create a separate
  MDL_lock class to represent a table.
sql/mdl.h:
  Adjust the MDL caching API to avoid races.
sql/sql_base.cc:
  Move all caching functionality close together.
  Implement a solution for deadlocks caused by 
  close_cached_tables() when MDL cache is enabled (incomplete).
sql/sql_yacc.yy:
  Adjust FLUSH rule to do the necessary initialization of
  TABLE_LIST elements used in for FLUSH TABLES <list>, and thus
  work OK with flush_mdl_cache() function.

sql/mdl.cc

@@ -284,8 +284,8 @@ class MDL_lock
 public:
   /** The key of the object (data) being protected. */
   MDL_key key;
-  void   *cached_object;
-  mdl_cached_object_release_hook cached_object_release_hook;
+  /** A cached reference to the TABLE_SHARE. Protected by LOCK_open. */
+  void   *m_cached_object;
   /**
     Read-write lock protecting this lock context.
 
@@ -362,8 +362,7 @@ class MDL_lock
 
   MDL_lock(const MDL_key *key_arg)
   : key(key_arg),
-    cached_object(NULL),
-    cached_object_release_hook(NULL),
+    m_cached_object(NULL),
     m_ref_usage(0),
     m_ref_release(0),
     m_is_destroyed(FALSE)
@@ -371,6 +370,8 @@ class MDL_lock
     mysql_prlock_init(key_MDL_lock_rwlock, &m_rwlock);
   }
 
+  /* Overridden for TABLE objects, to support TABLE_SHARE cache in MDL. */
+  virtual void release_cached_object() {}
   virtual ~MDL_lock()
   {
     mysql_prlock_destroy(&m_rwlock);
@@ -458,6 +459,25 @@ class MDL_object_lock : public MDL_lock
 };
 
 
+/** 
+  A lock implementation for MDL_key::TABLE.
+*/
+
+class MDL_table_lock: public MDL_object_lock
+{
+public:
+  MDL_table_lock(const MDL_key *key_arg)
+    : MDL_object_lock(key_arg)
+  { }
+#ifdef DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
+  virtual void release_cached_object()
+  {
+    tdc_release_cached_share(&m_cached_object);
+  }
+#endif
+};
+
+
 static MDL_map mdl_locks;
 
 extern "C"
@@ -674,8 +694,7 @@ void MDL_map::remove(MDL_lock *lock)
 {
   uint ref_usage, ref_release;
 
-  if (lock->cached_object)
-    (*lock->cached_object_release_hook)(lock->cached_object);
+  lock->release_cached_object();
 
   /*
     Destroy the MDL_lock object, but ensure that anyone that is
@@ -839,6 +858,8 @@ inline MDL_lock *MDL_lock::create(const MDL_key *mdl_key)
   {
     case MDL_key::GLOBAL:
       return new MDL_global_lock(mdl_key);
+    case MDL_key::TABLE:
+      return new MDL_table_lock(mdl_key);
     default:
       return new MDL_object_lock(mdl_key);
   }
@@ -1181,11 +1202,6 @@ void MDL_lock::reschedule_waiters()
         */
         m_waiting.remove_ticket(ticket);
         m_granted.add_ticket(ticket);
-
-        /* If we are granting an X lock, release the cached object. */
-        if (ticket->get_type() == MDL_EXCLUSIVE && cached_object)
-          (*cached_object_release_hook)(cached_object);
-        cached_object= NULL;
       }
       /*
         If we could not update the wait slot of the waiter,
@@ -1655,14 +1671,13 @@ MDL_context::try_acquire_lock_impl(MDL_request *mdl_request,
   {
     lock->m_granted.add_ticket(ticket);
 
-    if (mdl_request->type == MDL_EXCLUSIVE && lock->cached_object)
-      (*lock->cached_object_release_hook)(lock->cached_object);
-    lock->cached_object= NULL;
-
     mysql_prlock_unlock(&lock->m_rwlock);
 
     m_tickets.push_front(ticket);
 
+    if (ticket->get_type() == MDL_EXCLUSIVE)
+      ticket->clear_cached_object();
+
     mdl_request->ticket= ticket;
   }
   else
@@ -1864,6 +1879,9 @@ MDL_context::acquire_lock(MDL_request *mdl_request, ulong lock_wait_timeout)
   */
   DBUG_ASSERT(wait_status == MDL_wait::GRANTED);
 
+  if (ticket->get_type() == MDL_EXCLUSIVE)
+    ticket->clear_cached_object();
+
   m_tickets.push_front(ticket);
 
   mdl_request->ticket= ticket;
@@ -2450,7 +2468,7 @@ bool MDL_ticket::has_pending_conflicting_lock() const
 
   This function has the following usage pattern:
     - try to acquire an MDL lock
-    - when done, call for mdl_get_cached_object(). If it returns NULL, our
+    - when done, call for get_cached_object(). If it returns NULL, our
       thread has the only lock on this table.
     - look up TABLE_SHARE in the table definition cache
     - call mdl_set_cache_object() to assign the share to the opaque pointer.
@@ -2460,28 +2478,33 @@ bool MDL_ticket::has_pending_conflicting_lock() const
 */
 
 void
-MDL_ticket::set_cached_object(void *cached_object,
-                              mdl_cached_object_release_hook release_hook)
+MDL_ticket::set_cached_object(void *cached_object)
 {
-  DBUG_ENTER("mdl_set_cached_object");
+  DBUG_ENTER("MDL_ticket::set_cached_object");
   DBUG_PRINT("enter", ("db=%s name=%s cached_object=%p",
                         m_lock->key.db_name(), m_lock->key.name(),
                         cached_object));
-  /*
-    TODO: This assumption works now since we do get_cached_object()
-          and set_cached_object() in the same critical section. Once
-          this becomes false we will have to call release_hook here and
-          use additional mutex protecting 'cached_object' member.
-  */
-  DBUG_ASSERT(!m_lock->cached_object);
+  mysql_mutex_assert_owner(&LOCK_open);
+  DBUG_ASSERT(m_lock->key.mdl_namespace() == MDL_key::TABLE);
+  DBUG_ASSERT(!m_lock->m_cached_object);
 
-  m_lock->cached_object= cached_object;
-  m_lock->cached_object_release_hook= release_hook;
+  m_lock->m_cached_object= cached_object;
 
   DBUG_VOID_RETURN;
 }
 
 
+/**
+  A helper function to flush the table share cached in MDL.
+  @pre The ticket is acquired.
+*/
+
+void MDL_ticket::clear_cached_object()
+{
+  m_lock->release_cached_object();
+}
+
+
 /**
   Get a pointer to an opaque object that associated with the lock.
 
@@ -2492,7 +2515,8 @@ MDL_ticket::set_cached_object(void *cached_object,
 
 void *MDL_ticket::get_cached_object()
 {
-  return m_lock->cached_object;
+  mysql_mutex_assert_owner(&LOCK_open);
+  return m_lock->m_cached_object;
 }
 
 

sql/mdl.h

@@ -397,8 +397,8 @@ class MDL_ticket
   bool has_pending_conflicting_lock() const;
 
   void *get_cached_object();
-  void set_cached_object(void *cached_object,
-                         mdl_cached_object_release_hook release_hook);
+  void set_cached_object(void *cached_object);
+  void clear_cached_object();
   MDL_context *get_ctx() const { return m_ctx; }
   bool is_upgradable_or_exclusive() const
   {
@@ -724,6 +724,7 @@ extern "C" const char *set_thd_proc_info(void *thd_arg, const char *info,
                                          const char *calling_function,
                                          const char *calling_file,
                                          const unsigned int calling_line);
+extern void tdc_release_cached_share(void *ptr);
 #ifndef DBUG_OFF
 extern mysql_mutex_t LOCK_open;
 #endif

sql/sql_base.cc

@@ -755,27 +755,6 @@ TABLE_SHARE *get_cached_table_share(const char *db, const char *table_name)
 }  
 
 
-#ifdef DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
-/**
-   @brief Mark table share as having one more user (increase its reference
-          count).
-
-   @param share Table share for which reference count should be increased.
-*/
-
-static void reference_table_share(TABLE_SHARE *share)
-{
-  DBUG_ENTER("reference_table_share");
-  DBUG_ASSERT(share->ref_count);
-  mysql_mutex_assert_owner(&LOCK_open);
-  share->ref_count++;
-  DBUG_PRINT("exit", ("share: 0x%lx  ref_count: %u",
-                     (ulong) share, share->ref_count));
-  DBUG_VOID_RETURN;
-}
-#endif
-
-
 /*
   Create a list for all open tables matching SQL expression
 
@@ -941,6 +920,141 @@ static void kill_delayed_threads_for_table(TABLE_SHARE *share)
 }
 
 
+#ifdef DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
+/**
+  Flush MDL cached objects.
+
+  How MDL table share cache works
+  -------------------------------
+  Since we take a table share from the table definition
+  cache only after taking an MDL lock, the MDL lock
+  object is a convenient place to cache a pointer
+  to the table share. However, not all SQL in MySQL
+  takes an MDL lock prior to working with the TDC,
+  various forms of FLUSH TABLES (including SET GLOBAL
+  read_only) being the one and only exception.
+
+  To make FLUSH TABLES work, and avoid having dangling
+  references to TABLE_SHARE objects in MDL subsystem
+  after a flush, we make sure that all references 
+  to table shares are released whenever a flush comes.
+  This is done in this function.
+
+  To sum up, the following invariants are held:
+  - no statement can work with a TABLE_SHARE without
+  a metadata lock. The only exception is FLUSH TABLES.
+  - a metadata lock object can be used to store
+  a cached reference (pointer) to the corresponding
+  TABLE_SHARE, if and only if this TABLE_SHARE is
+  not stale (version == refresh_version). In other words,
+  checking TABLE_SHARE version and setting the reference
+  must happen only in the same critical section protected
+  by LOCK_open.
+  - FLUSH will mark all subject TABLE_SHARE objects
+  as stale, and then will manually release all TABLE_SHARE
+  references in MDL cache. Since marking TABLE_SHARE
+  objects is done inside a critical section protected
+  by LOCK_open and prior to calling flush_mdl_cache(),
+  it's guaranteed that a flush will take place before
+  a new reference to the table share is established
+  in some other connection.
+*/
+
+bool flush_mdl_cache(THD *thd, TABLE_LIST *table_list)
+{
+  MDL_request_list mdl_requests;
+  MDL_request *mdl_request;
+
+  DBUG_ENTER("flush_mdl_cache");
+
+  if (table_list == NULL)
+  {
+    mysql_mutex_lock(&LOCK_open);
+    for (uint idx= 0 ; idx < table_def_cache.records; idx++)
+    {
+      TABLE_SHARE *share=(TABLE_SHARE*) my_hash_element(&table_def_cache,
+                                                        idx);
+      if (share->needs_reopen())
+      {
+        mdl_request= MDL_request::create(MDL_key::TABLE,
+                                         share->db.str,
+                                         share->table_name.str,
+                                         MDL_SHARED_HIGH_PRIO,
+                                         thd->mem_root);
+        if (! mdl_request)
+        {
+          mysql_mutex_unlock(&LOCK_open);
+          DBUG_RETURN(TRUE);
+        }
+        mdl_requests.push_front(mdl_request);
+      }
+    }
+    mysql_mutex_unlock(&LOCK_open);
+  }
+  else
+  {
+    for (TABLE_LIST *tables= table_list; tables; tables= tables->next_global)
+    {
+      DBUG_ASSERT(tables->mdl_request.type == MDL_SHARED_HIGH_PRIO);
+      mdl_requests.push_front(&tables->mdl_request);
+    }
+  }
+
+  for (MDL_request_list::Iterator it(mdl_requests);
+       (mdl_request= it++); )
+  {
+    if (thd->mdl_context.try_acquire_lock(mdl_request))
+      DBUG_RETURN(TRUE);
+    if (mdl_request->ticket)
+    {
+      mdl_request->ticket->clear_cached_object();
+      thd->mdl_context.release_lock(mdl_request->ticket);
+    }
+  }
+  DBUG_RETURN(FALSE);
+}
+
+
+/**
+   @brief Helper function used by MDL subsystem for releasing TABLE_SHARE
+          objects in cases when it no longer wants to cache reference to it.
+*/
+
+void tdc_release_cached_share(void *ptr)
+{
+  TABLE_SHARE **share= (TABLE_SHARE **) ptr;
+  mysql_mutex_lock(&LOCK_open);
+  if (*share)
+  {
+    release_table_share(*share);
+    *share= NULL;
+    broadcast_refresh();
+  }
+  mysql_mutex_unlock(&LOCK_open);
+}
+
+
+/**
+   @brief Mark table share as having one more user (increase its reference
+          count).
+
+   @param share Table share for which reference count should be increased.
+*/
+
+static void tdc_reference_table_share(TABLE_SHARE *share)
+{
+  DBUG_ENTER("tdc_reference_table_share");
+  DBUG_ASSERT(share->ref_count);
+  mysql_mutex_assert_owner(&LOCK_open);
+  share->ref_count++;
+  DBUG_PRINT("exit", ("share: 0x%lx  ref_count: %u",
+                     (ulong) share, share->ref_count));
+  DBUG_VOID_RETURN;
+}
+
+
+#endif // DISABLED_UNTIL_GRL_IS_MADE_PART_OF
+
 /*
   Close all tables which aren't in use by any thread
 
@@ -1052,6 +1166,15 @@ bool close_cached_tables(THD *thd, TABLE_LIST *tables, bool have_lock,
   /* Wait until all threads have closed all the tables we are flushing. */
   DBUG_PRINT("info", ("Waiting for other threads to close their open tables"));
 
+#ifdef DISABLED_UNTIL_GRL_IS_MADE_PART_OF
+  /*
+    @todo We need to do this for fast refresh as well, otherwise
+    deadlocks are possible.
+  */
+  if (flush_mdl_cache(thd, tables))
+    goto err_with_reopen;
+#endif // DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
+
   while (found && ! thd->killed)
   {
     found= FALSE;
@@ -2361,20 +2484,6 @@ bool check_if_table_exists(THD *thd, TABLE_LIST *table, bool *exists)
 }
 
 
-/**
-   @brief Helper function used by MDL subsystem for releasing TABLE_SHARE
-          objects in cases when it no longer wants to cache reference to it.
-*/
-
-void table_share_release_hook(void *share)
-{
-  mysql_mutex_lock(&LOCK_open);
-  release_table_share((TABLE_SHARE*) share);
-  broadcast_refresh();
-  mysql_mutex_unlock(&LOCK_open);
-}
-
-
 /**
   An error handler which converts, if possible, ER_LOCK_DEADLOCK error
   that can occur when we are trying to acquire a metadata lock to
@@ -2869,8 +2978,8 @@ bool open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
   mysql_mutex_lock(&LOCK_open);
 #ifdef DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
   if (!(share= (TABLE_SHARE *) mdl_ticket->get_cached_object()))
-#endif
   {
+#endif // DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
     if (!(share= get_table_share_with_create(thd, table_list, key,
                                              key_length, OPEN_VIEW,
                                              &error,
@@ -2927,14 +3036,15 @@ bool open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
 
 #ifdef DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
     /*
-      We are going to to store extra reference to the share in MDL-subsystem
-      so we need to increase reference counter;
+      We are going to to store extra reference to the share
+      in MDL-subsystem so we need to increase reference counter.
     */
-    reference_table_share(share);
-    mdl_ticket->set_cached_object(share, table_share_release_hook);
-#endif
+    if (! share->needs_reopen())
+    {
+      mdl_ticket->set_cached_object(share);
+      tdc_reference_table_share(share);
+    }
   }
-#ifdef DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
   else
   {
     if (table_list->view)
@@ -2943,19 +3053,19 @@ bool open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
       check_and_update_table_version(thd, table_list, share);
       /* Always an error. */
       DBUG_ASSERT(thd->is_error());
-      goto err_unlock;
+      goto err_unlock2;
     }
     /* When we have cached TABLE_SHARE we know that is not a view. */
     if (table_list->i_s_requested_object &  OPEN_VIEW_ONLY)
-      goto err_unlock;
+      goto err_unlock2;
 
     /*
       We are going to use this share for construction of new TABLE object
       so reference counter should be increased.
     */
-    reference_table_share(share);
+    tdc_reference_table_share(share);
   }
-#endif
+#endif // DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
 
 
   /*
@@ -8755,6 +8865,13 @@ void tdc_remove_table(THD *thd, enum_tdc_remove_table_type remove_type,
         automatically deleted once it is no longer referenced.
       */
       share->version= 0;
+#ifdef DISABLED_UNTIL_GRL_IS_MADE_PART_OF_MDL
+      /*
+        If lock type is not EXCLUSIVE, we must call
+        MDL_ticket::release_cached_object() here to make sure there
+        is no self-reference left on the share in MDL_lock.
+      */
+#endif
       while ((table= it++))
         free_cache_entry(table);
     }

sql/sql_yacc.yy

@@ -11186,10 +11186,10 @@ flush_options:
             Lex->type|= REFRESH_TABLES;
             /*
               Set type of metadata and table locks for
-              FLUSH TABLES table_list WITH READ LOCK.
+              FLUSH TABLES table_list [WITH READ LOCK].
             */
             YYPS->m_lock_type= TL_READ_NO_INSERT;
-            YYPS->m_mdl_type= MDL_EXCLUSIVE;
+            YYPS->m_mdl_type= MDL_SHARED_HIGH_PRIO;
           }
           opt_table_list {}
           opt_with_read_lock {}
@@ -11199,7 +11199,13 @@ flush_options:
 opt_with_read_lock:
           /* empty */ {}
         | WITH READ_SYM LOCK_SYM
-          { Lex->type|= REFRESH_READ_LOCK; }
+          {
+            TABLE_LIST *tables= Lex->query_tables;
+            Lex->type|= REFRESH_READ_LOCK;
+            /* We acquire an X lock currently and then downgrade. */
+            for (; tables; tables= tables->next_global)
+              tables->mdl_request.set_type(MDL_EXCLUSIVE);
+          }
         ;
 
 flush_options_list: