A better fix for bug #56405 "Deadlock in the MDL deadlock

detector" that doesn't introduce bug #56715 "Concurrent
transactions + FLUSH result in sporadical unwarranted
deadlock errors".

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

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

The previous patch tried to workaround this problem by not
allowing the 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
was reported. As a result it has introduced bug #56715.

This patch solves this problem in a different way.
It introduces a new rw_pr_lock_t implementation to be used
by MDL subsystem instead of one based on Linux rwlocks or
our own rwlock implementation. This new implementation
never allows situation in which an rwlock is rd-locked and
there is a blocked pending rd-lock. Thus the situation which
has caused this bug becomes impossible with this implementation.

Due to fact that this implementation is optimized for
wr-lock/unlock scenario which is most common in the MDL
subsystem it doesn't introduce noticeable performance
regressions in sysbench tests. Moreover it significantly
improves situation for POINT_SELECT test when many
connections are used.

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.
This patch also doesn't include a test for bug #56715
"Concurrent transactions + FLUSH result in sporadical
unwarranted deadlock errors" as it takes too much time to
be run as part of normal test-suite runs.

config.h.cmake:
  We no longer need to check for presence of
  pthread_rwlockattr_setkind_np as we no longer
  use Linux-specific implementation of rw_pr_lock_t
  which uses this function.
configure.cmake:
  We no longer need to check for presence of
  pthread_rwlockattr_setkind_np as we no longer
  use Linux-specific implementation of rw_pr_lock_t
  which uses this function.
configure.in:
  We no longer need to check for presence of
  pthread_rwlockattr_setkind_np as we no longer
  use Linux-specific implementation of rw_pr_lock_t
  which uses this function.
include/my_pthread.h:
  Introduced new implementation of rw_pr_lock_t.
  Since it never allows situation in which rwlock is rd-locked
  and there is a blocked pending rd-lock it is not affected by
  bug #56405 "Deadlock in the MDL deadlock detector".
  This implementation is also optimized for wr-lock/unlock
  scenario which is most common in MDL subsystem. So it doesn't
  introduce noticiable performance regressions in sysbench tests
  (compared to old Linux-specific implementation). Moreover it
  significantly improves situation for POINT_SELECT test when
  many connections are used.
  As part of this change removed try-lock part of API for
  this type of lock. It is not used in our code and it would
  be hard to implement correctly within constraints of new
  implementation.
  Finally, removed support of preferring readers from
  my_rw_lock_t implementation as the only user of this
  feature was old rw_pr_lock_t implementation.
include/mysql/psi/mysql_thread.h:
  Removed try-lock part of prlock API.
  It is not used in our code and it would be hard
  to implement correctly within constraints of new
  prlock implementation.
mysys/thr_rwlock.c:
  Introduced new implementation of rw_pr_lock_t.
  Since it never allows situation in which rwlock is rd-locked
  and there is a blocked pending rd-lock it is not affected by
  bug #56405 "Deadlock in the MDL deadlock detector".
  This implementation is also optimized for wr-lock/unlock
  scenario which is most common in MDL subsystem. So it doesn't
  introduce noticiable performance regressions in sysbench tests
  (compared to old Linux-specific implementation). Moreover it
  significantly improves situation for POINT_SELECT test when
  many connections are used.
  Also removed support of preferring readers from
  my_rw_lock_t implementation as the only user of this
  feature was old rw_pr_lock_t implementation.

1 files changed, 115 insertions, 22 deletions

diff --git a/mysys/thr_rwlock.c b/mysys/thr_rwlock.c
index ecd12849822..218cfb251c8 100644
--- a/mysys/thr_rwlock.c
+++ b/mysys/thr_rwlock.c
@@ -59,7 +59,7 @@
 *  Mountain View, California  94043
 */
 
-int my_rw_init(my_rw_lock_t *rwp, my_bool *prefer_readers_attr)
+int my_rw_init(my_rw_lock_t *rwp)
 {
   pthread_condattr_t	cond_attr;
 
@@ -74,8 +74,6 @@ int my_rw_init(my_rw_lock_t *rwp, my_bool *prefer_readers_attr)
 #ifdef SAFE_MUTEX
   rwp->write_thread   = 0;
 #endif
-  /* If attribute argument is NULL use default value - prefer writers. */
-  rwp->prefer_readers= prefer_readers_attr ? *prefer_readers_attr : FALSE;
 
   return(0);
 }
@@ -96,8 +94,7 @@ int my_rw_rdlock(my_rw_lock_t *rwp)
   pthread_mutex_lock(&rwp->lock);
 
   /* active or queued writers */
-  while (( rwp->state < 0 ) ||
-         (rwp->waiters && ! rwp->prefer_readers))
+  while (( rwp->state < 0 ) || rwp->waiters)
     pthread_cond_wait( &rwp->readers, &rwp->lock);
 
   rwp->state++;
@@ -109,8 +106,7 @@ int my_rw_tryrdlock(my_rw_lock_t *rwp)
 {
   int res;
   pthread_mutex_lock(&rwp->lock);
-  if ((rwp->state < 0 ) ||
-      (rwp->waiters && ! rwp->prefer_readers))
+  if ((rwp->state < 0 ) || rwp->waiters)
     res= EBUSY;					/* Can't get lock */
   else
   {
@@ -192,30 +188,127 @@ int my_rw_unlock(my_rw_lock_t *rwp)
   return(0);
 }
 
+#endif /* defined(NEED_MY_RW_LOCK) */
+
 
-int rw_pr_init(struct st_my_rw_lock_t *rwlock)
+int rw_pr_init(rw_pr_lock_t *rwlock)
 {
-  my_bool prefer_readers_attr= TRUE;
-  return my_rw_init(rwlock, &prefer_readers_attr);
+  pthread_mutex_init(&rwlock->lock, NULL);
+  pthread_cond_init(&rwlock->no_active_readers, NULL);
+  rwlock->active_readers= 0;
+  rwlock->writers_waiting_readers= 0;
+  rwlock->active_writer= FALSE;
+#ifdef SAFE_MUTEX
+  rwlock->writer_thread= 0;
+#endif
+  return 0;
 }
 
-#else
 
-/*
-  We are on system which has native read/write locks which support
-  preferring of readers.
-*/
+int rw_pr_destroy(rw_pr_lock_t *rwlock)
+{
+  pthread_cond_destroy(&rwlock->no_active_readers);
+  pthread_mutex_destroy(&rwlock->lock);
+  return 0;
+}
 
-int rw_pr_init(rw_pr_lock_t *rwlock)
+
+int rw_pr_rdlock(rw_pr_lock_t *rwlock)
+{
+  pthread_mutex_lock(&rwlock->lock);
+  /*
+    The fact that we were able to acquire 'lock' mutex means
+    that there are no active writers and we can acquire rd-lock.
+    Increment active readers counter to prevent requests for
+    wr-lock from succeeding and unlock mutex.
+  */
+  rwlock->active_readers++;
+  pthread_mutex_unlock(&rwlock->lock);
+  return 0;
+}
+
+
+int rw_pr_wrlock(rw_pr_lock_t *rwlock)
 {
-  pthread_rwlockattr_t rwlock_attr;
+  pthread_mutex_lock(&rwlock->lock);
+
+  if (rwlock->active_readers != 0)
+  {
+    /* There are active readers. We have to wait until they are gone. */
+    rwlock->writers_waiting_readers++;
 
-  pthread_rwlockattr_init(&rwlock_attr);
-  pthread_rwlockattr_setkind_np(&rwlock_attr, PTHREAD_RWLOCK_PREFER_READER_NP);
-  pthread_rwlock_init(rwlock, NULL);
-  pthread_rwlockattr_destroy(&rwlock_attr);
+    while (rwlock->active_readers != 0)
+      pthread_cond_wait(&rwlock->no_active_readers, &rwlock->lock);
+
+    rwlock->writers_waiting_readers--;
+  }
+
+  /*
+    We own 'lock' mutex so there is no active writers.
+    Also there are no active readers.
+    This means that we can grant wr-lock.
+    Not releasing 'lock' mutex until unlock will block
+    both requests for rd and wr-locks.
+    Set 'active_writer' flag to simplify unlock.
+
+    Thanks to the fact wr-lock/unlock in the absence of
+    contention from readers is essentially mutex lock/unlock
+    with a few simple checks make this rwlock implementation
+    wr-lock optimized.
+  */
+  rwlock->active_writer= TRUE;
+#ifdef SAFE_MUTEX
+  rwlock->writer_thread= pthread_self();
+#endif
   return 0;
 }
 
-#endif /* defined(NEED_MY_RW_LOCK) */
+
+int rw_pr_unlock(rw_pr_lock_t *rwlock)
+{
+  if (rwlock->active_writer)
+  {
+    /* We are unlocking wr-lock. */
+#ifdef SAFE_MUTEX
+    rwlock->writer_thread= 0;
+#endif
+    rwlock->active_writer= FALSE;
+    if (rwlock->writers_waiting_readers)
+    {
+      /*
+        Avoid expensive cond signal in case when there is no contention
+        or it is wr-only.
+
+        Note that from view point of performance it would be better to
+        signal on the condition variable after unlocking mutex (as it
+        reduces number of contex switches).
+
+        Unfortunately this would mean that such rwlock can't be safely
+        used by MDL subsystem, which relies on the fact that it is OK
+        to destroy rwlock once it is in unlocked state.
+      */
+      pthread_cond_signal(&rwlock->no_active_readers);
+    }
+    pthread_mutex_unlock(&rwlock->lock);
+  }
+  else
+  {
+    /* We are unlocking rd-lock. */
+    pthread_mutex_lock(&rwlock->lock);
+    rwlock->active_readers--;
+    if (rwlock->active_readers == 0 &&
+        rwlock->writers_waiting_readers)
+    {
+      /*
+        If we are last reader and there are waiting
+        writers wake them up.
+      */
+      pthread_cond_signal(&rwlock->no_active_readers);
+    }
+    pthread_mutex_unlock(&rwlock->lock);
+  }
+  return 0;
+}
+
+
 #endif /* defined(THREAD) */