A temporary workaround for bug #56405 "Deadlock in the

Dmitry Lenev · Dmitry Lenev · commit d8b313a01de0 · 2010-09-06T21:29:02.000+04:00
MDL deadlock detector".

Deadlock could have occurred when workload containing mix
of DML, DDL and FLUSH TABLES statements affecting same
set of tables was executed in heavily concurrent environment.

This deadlock occurred when several connections tried to
perform deadlock detection in metadata locking subsystem.
The first connection started traversing wait-for graph,
encountered sub-graph representing wait for flush, acquired
LOCK_open and dived into sub-graph inspection. When it has
encounterd sub-graph corresponding to wait for metadata lock
and blocked while trying to acquire rd-lock on
MDL_lock::m_rwlock (*) protecting this subgraph, since some
other thread had wr-lock on it. When this wr-lock was released
it could have happened (if there was other pending wr-lock
against this rwlock) that rd-lock from the first connection
was left unsatisfied but at the same time new rd-lock request
from the second connection sneaked in and was satisfied (for
this to be possible second rd- request should come exactly
after wr-lock is released but before pending wr-lock manages
to grab rwlock, which is possible both on Linux and in our
own rwlock implementation). If this second connection
continued traversing wait-for graph and encountered sub-graph
representing wait for flush it tried to acquire LOCK_open
and thus deadlock was created.

This patch tries to workaround this problem but not allowing
deadlock detector to lock LOCK_open mutex if some other thread
doing deadlock detection already owns it and current search
depth is greater than 0. Instead deadlock is reported.

Other possible solutions are either known to have negative
effects on performance or require much more time for proper
implementation and testing.

No test case is provided as this bug is very hard to repeat
in MTR environment but is repeatable with the help of RQG
tests.
--BZR--
revision-id: dlenev@mysql.com-20100906172902-p0grd8x6thgqcnjd
property-branch-nick: mysql-5.5.6-m3-release
property-file-info: ld7:file_id40:mdl.cc-20080523121737-j62pi0m62eaw1hq6-17:message261:Moved Deadlock_detection_visitor::m_current_search_depth to
property-file-info: parent class to make it available in
property-file-info: TABLE_SHARE::visit_subgraph().
property-file-info: Added MDL_wait_for_graph_visitor::abort_traversal() method
property-file-info: which allows to abort traversal of a wait-for graph and
property-file-info: report a deadlock.4:path10:sql/mdl.cced7:file_id39:mdl.h-20080523121748-o4y2wcq3maotb9do-17:message261:Moved Deadlock_detection_visitor::m_current_search_depth to
property-file-info: parent class to make it available in
property-file-info: TABLE_SHARE::visit_subgraph().
property-file-info: Added MDL_wait_for_graph_visitor::abort_traversal() method
property-file-info: which allows to abort traversal of a wait-for graph and
property-file-info: report a deadlock.4:path9:sql/mdl.hed7:file_id64:sp1f-sql_base.cc-19700101030959-w7tul2gb2n4jzayjwlslj3ybmf3uhk6a7:message151:Added dd_owns_lock_open counter and mutex protecting it to
property-file-info: track number of connections which do deadlock detection and
property-file-info: own or try to acquire LOCK_open.4:path15:sql/sql_base.cced7:file_id45:sql_base.h-20100331135644-cgcb6oowzqyx7fi3-127:message151:Added dd_owns_lock_open counter and mutex protecting it to
property-file-info: track number of connections which do deadlock detection and
property-file-info: own or try to acquire LOCK_open.4:path14:sql/sql_base.hed7:file_id61:sp1f-table.cc-19700101030959-nsxtem2adyqzwe6nz4cgrpcmts3o54v77:message215:Workaround bug #56405 but not allowing MDL deadlock detector
property-file-info: to lock LOCK_open mutex if some other thread doing deadlock
property-file-info: detection already owns it and current search depth is greater
property-file-info: than 0. Instead report deadlock.4:path12:sql/table.ccee
testament3-sha1: e192dbc29ae016554c132a6d1c1c9138f1ba46c7
diff --git a/sql/mdl.cc b/sql/mdl.cc
@@ -124,7 +124,6 @@ class Deadlock_detection_visitor: public MDL_wait_for_graph_visitor
   Deadlock_detection_visitor(MDL_context *start_node_arg)
     : m_start_node(start_node_arg),
       m_victim(NULL),
-      m_current_search_depth(0),
       m_found_deadlock(FALSE)
   {}
   virtual bool enter_node(MDL_context *node);
@@ -133,6 +132,8 @@ class Deadlock_detection_visitor: public MDL_wait_for_graph_visitor
   virtual bool inspect_edge(MDL_context *dest);
 
   MDL_context *get_victim() const { return m_victim; }
+
+  void abort_traversal(MDL_context *node);
 private:
   /**
     Change the deadlock victim to a new one if it has lower deadlock
@@ -147,13 +148,6 @@ class Deadlock_detection_visitor: public MDL_wait_for_graph_visitor
   MDL_context *m_start_node;
   /** If a deadlock is found, the context that identifies the victim. */
   MDL_context *m_victim;
-  /** Set to the 0 at start. Increased whenever
-    we descend into another MDL context (aka traverse to the next
-    wait-for graph node). When MAX_SEARCH_DEPTH is reached, we
-    assume that a deadlock is found, even if we have not found a
-    loop.
-  */
-  uint m_current_search_depth;
   /** TRUE if we found a deadlock. */
   bool m_found_deadlock;
   /**
@@ -187,7 +181,7 @@ class Deadlock_detection_visitor: public MDL_wait_for_graph_visitor
 
 bool Deadlock_detection_visitor::enter_node(MDL_context *node)
 {
-  m_found_deadlock= ++m_current_search_depth >= MAX_SEARCH_DEPTH;
+  m_found_deadlock= m_current_search_depth >= MAX_SEARCH_DEPTH;
   if (m_found_deadlock)
   {
     DBUG_ASSERT(! m_victim);
@@ -207,7 +201,6 @@ bool Deadlock_detection_visitor::enter_node(MDL_context *node)
 
 void Deadlock_detection_visitor::leave_node(MDL_context *node)
 {
-  --m_current_search_depth;
   if (m_found_deadlock)
     opt_change_victim_to(node);
 }
@@ -251,6 +244,21 @@ Deadlock_detection_visitor::opt_change_victim_to(MDL_context *new_victim)
 }
 
 
+/**
+  Abort traversal of a wait-for graph and report a deadlock.
+
+  @param node Node which we were about to visit when abort
+              was initiated.
+*/
+
+void Deadlock_detection_visitor::abort_traversal(MDL_context *node)
+{
+  DBUG_ASSERT(! m_victim);
+  m_found_deadlock= TRUE;
+  opt_change_victim_to(node);
+}
+
+
 /**
   Get a bit corresponding to enum_mdl_type value in a granted/waiting bitmaps
   and compatibility matrices.
@@ -2056,8 +2064,13 @@ bool MDL_lock::visit_subgraph(MDL_ticket *waiting_ticket,
     are visiting it but this is OK: in the worst case we might do some
     extra work and one more context might be chosen as a victim.
   */
+  ++gvisitor->m_current_search_depth;
+
   if (gvisitor->enter_node(src_ctx))
+  {
+    --gvisitor->m_current_search_depth;
     goto end;
+  }
 
   /*
     We do a breadth-first search first -- that is, inspect all
@@ -2114,6 +2127,7 @@ bool MDL_lock::visit_subgraph(MDL_ticket *waiting_ticket,
 
 end_leave_node:
   gvisitor->leave_node(src_ctx);
+  --gvisitor->m_current_search_depth;
 
 end:
   mysql_prlock_unlock(&m_rwlock);
diff --git a/sql/mdl.h b/sql/mdl.h
@@ -385,7 +385,10 @@ class MDL_wait_for_graph_visitor
 
   virtual bool inspect_edge(MDL_context *dest) = 0;
   virtual ~MDL_wait_for_graph_visitor();
-  MDL_wait_for_graph_visitor() :m_lock_open_count(0) {}
+  MDL_wait_for_graph_visitor() :m_lock_open_count(0),
+                                m_current_search_depth(0)
+  { }
+  virtual void abort_traversal(MDL_context *node) = 0;
 public:
   /**
    XXX, hack: During deadlock search, we may need to
@@ -396,6 +399,17 @@ class MDL_wait_for_graph_visitor
    LOCK_open since it has significant performance impacts.
   */
   uint m_lock_open_count;
+  /**
+    Set to the 0 at start. Increased whenever
+    we descend into another MDL context (aka traverse to the next
+    wait-for graph node). When MAX_SEARCH_DEPTH is reached, we
+    assume that a deadlock is found, even if we have not found a
+    loop.
+
+    XXX: This member belongs to this class only temporarily until
+         bug #56405 is fixed.
+  */
+  uint m_current_search_depth;
 };
 
 /**
diff --git a/sql/sql_base.cc b/sql/sql_base.cc
@@ -100,6 +100,8 @@ bool No_such_table_error_handler::safely_trapped_errors()
   TABLE_SHAREs, refresh_version and the table id counter.
 */
 mysql_mutex_t LOCK_open;
+mysql_mutex_t LOCK_dd_owns_lock_open;
+uint dd_owns_lock_open= 0;
 
 #ifdef HAVE_PSI_INTERFACE
 static PSI_mutex_key key_LOCK_open;
@@ -298,6 +300,7 @@ bool table_def_init(void)
   init_tdc_psi_keys();
 #endif
   mysql_mutex_init(key_LOCK_open, &LOCK_open, MY_MUTEX_INIT_FAST);
+  mysql_mutex_init(NULL, &LOCK_dd_owns_lock_open, MY_MUTEX_INIT_FAST);
   oldest_unused_share= &end_of_unused_share;
   end_of_unused_share.prev= &oldest_unused_share;
 
@@ -341,6 +344,7 @@ void table_def_free(void)
     table_def_inited= 0;
     /* Free table definitions. */
     my_hash_free(&table_def_cache);
+    mysql_mutex_destroy(&LOCK_dd_owns_lock_open);
     mysql_mutex_destroy(&LOCK_open);
   }
   DBUG_VOID_RETURN;
diff --git a/sql/sql_base.h b/sql/sql_base.h
@@ -71,6 +71,8 @@ enum enum_tdc_remove_table_type {TDC_RT_REMOVE_ALL, TDC_RT_REMOVE_NOT_OWN,
 
 bool check_dup(const char *db, const char *name, TABLE_LIST *tables);
 extern mysql_mutex_t LOCK_open;
+extern mysql_mutex_t LOCK_dd_owns_lock_open;
+extern uint dd_owns_lock_open;
 bool table_cache_init(void);
 void table_cache_free(void);
 bool table_def_init(void);
diff --git a/sql/table.cc b/sql/table.cc
@@ -3085,7 +3085,30 @@ bool TABLE_SHARE::visit_subgraph(Wait_for_flush *wait_for_flush,
     holding a write-lock on MDL_lock::m_rwlock.
   */
   if (gvisitor->m_lock_open_count++ == 0)
+  {
+    /*
+      To circumvent bug #56405 "Deadlock in the MDL deadlock detector"
+      we don't try to lock LOCK_open mutex if some thread doing
+      deadlock detection already owns it and current search depth is
+      greater than 0. Instead we report a deadlock.
+
+      TODO/FIXME: The proper fix for this bug is to use rwlocks for
+                  protection of table shares/instead of LOCK_open.
+                  Unfortunately it requires more effort/has significant
+                  performance effect.
+    */
+    mysql_mutex_lock(&LOCK_dd_owns_lock_open);
+    if (gvisitor->m_current_search_depth > 0 && dd_owns_lock_open > 0)
+    {
+      mysql_mutex_unlock(&LOCK_dd_owns_lock_open);
+      --gvisitor->m_lock_open_count;
+      gvisitor->abort_traversal(src_ctx);
+      return TRUE;
+    }
+    ++dd_owns_lock_open;
+    mysql_mutex_unlock(&LOCK_dd_owns_lock_open);
     mysql_mutex_lock(&LOCK_open);
+  }
 
   I_P_List_iterator <TABLE, TABLE_share> tables_it(used_tables);
 
@@ -3100,8 +3123,12 @@ bool TABLE_SHARE::visit_subgraph(Wait_for_flush *wait_for_flush,
     goto end;
   }
 
+  ++gvisitor->m_current_search_depth;
   if (gvisitor->enter_node(src_ctx))
+  {
+    --gvisitor->m_current_search_depth;
     goto end;
+  }
 
   while ((table= tables_it++))
   {
@@ -3124,10 +3151,16 @@ bool TABLE_SHARE::visit_subgraph(Wait_for_flush *wait_for_flush,
 
 end_leave_node:
   gvisitor->leave_node(src_ctx);
+  --gvisitor->m_current_search_depth;
 
 end:
   if (gvisitor->m_lock_open_count-- == 1)
+  {
     mysql_mutex_unlock(&LOCK_open);
+    mysql_mutex_lock(&LOCK_dd_owns_lock_open);
+    --dd_owns_lock_open;
+    mysql_mutex_unlock(&LOCK_dd_owns_lock_open);
+  }
 
   return result;
 }
