Rewrite ConditionVariableBroadcast() to avoid live-lock.

The original implementation of ConditionVariableBroadcast was, per its
self-description, "the dumbest way possible".  Thomas Munro found out
it was a bit too dumb.  An awakened process may immediately re-queue
itself, if the specific condition it's waiting for is not yet satisfied.
If this happens before ConditionVariableBroadcast is able to see the wait
queue as empty, then ConditionVariableBroadcast will re-awaken the same
process, repeating the cycle.  Given unlucky timing this back-and-forth
can repeat indefinitely; loops lasting thousands of seconds have been
seen in testing.

To fix, add our own process to the end of the wait queue to serve as a
sentinel, and exit the broadcast loop once our process is not there
anymore.  There are various special considerations described in the
comments, the principal disadvantage being that wakers can no longer
be sure whether they awakened a real waiter or just a sentinel.  But in
practice nobody pays attention to the result of ConditionVariableSignal
or ConditionVariableBroadcast anyway, so that problem seems hypothetical.

Back-patch to v10 where condition_variable.c was introduced.

Tom Lane and Thomas Munro

Discussion: https://postgr.es/m/CAEepm=0NWKehYw7NDoUSf8juuKOPRnCyY3vuaSvhrEWsOTAa3w@mail.gmail.com

diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index 41378d614a4d876c404e5cdaf2c4510027a4860f..55275cfafcbefbb161812f1c45fdca8f69c66992 100644 (file)
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -214,15 +214,87 @@ int
 ConditionVariableBroadcast(ConditionVariable *cv)
 {
        int                     nwoken = 0;
+       int                     pgprocno = MyProc->pgprocno;
+       PGPROC     *proc = NULL;
+       bool            have_sentinel = false;
+
+       /*
+        * In some use-cases, it is common for awakened processes to immediately
+        * re-queue themselves.  If we just naively try to reduce the wakeup list
+        * to empty, we'll get into a potentially-indefinite loop against such a
+        * process.  The semantics we really want are just to be sure that we have
+        * wakened all processes that were in the list at entry.  We can use our
+        * own cvWaitLink as a sentinel to detect when we've finished.
+        *
+        * A seeming flaw in this approach is that someone else might signal the
+        * CV and in doing so remove our sentinel entry.  But that's fine: since
+        * CV waiters are always added and removed in order, that must mean that
+        * every previous waiter has been wakened, so we're done.  We'll get an
+        * extra "set" on our latch from the someone else's signal, which is
+        * slightly inefficient but harmless.
+        *
+        * We can't insert our cvWaitLink as a sentinel if it's already in use in
+        * some other proclist.  While that's not expected to be true for typical
+        * uses of this function, we can deal with it by simply canceling any
+        * prepared CV sleep.  The next call to ConditionVariableSleep will take
+        * care of re-establishing the lost state.
+        */
+       ConditionVariableCancelSleep();
 
        /*
-        * Let's just do this the dumbest way possible.  We could try to dequeue
-        * all the sleepers at once to save spinlock cycles, but it's a bit hard
-        * to get that right in the face of possible sleep cancelations, and we
-        * don't want to loop holding the mutex.
+        * Inspect the state of the queue.  If it's empty, we have nothing to do.
+        * If there's exactly one entry, we need only remove and signal that
+        * entry.  Otherwise, remove the first entry and insert our sentinel.
         */
-       while (ConditionVariableSignal(cv))
+       SpinLockAcquire(&cv->mutex);
+       /* While we're here, let's assert we're not in the list. */
+       Assert(!proclist_contains(&cv->wakeup, pgprocno, cvWaitLink));
+
+       if (!proclist_is_empty(&cv->wakeup))
+       {
+               proc = proclist_pop_head_node(&cv->wakeup, cvWaitLink);
+               if (!proclist_is_empty(&cv->wakeup))
+               {
+                       proclist_push_tail(&cv->wakeup, pgprocno, cvWaitLink);
+                       have_sentinel = true;
+               }
+       }
+       SpinLockRelease(&cv->mutex);
+
+       /* Awaken first waiter, if there was one. */
+       if (proc != NULL)
+       {
+               SetLatch(&proc->procLatch);
                ++nwoken;
+       }
+
+       while (have_sentinel)
+       {
+               /*
+                * Each time through the loop, remove the first wakeup list entry, and
+                * signal it unless it's our sentinel.  Repeat as long as the sentinel
+                * remains in the list.
+                *
+                * Notice that if someone else removes our sentinel, we will waken one
+                * additional process before exiting.  That's intentional, because if
+                * someone else signals the CV, they may be intending to waken some
+                * third process that added itself to the list after we added the
+                * sentinel.  Better to give a spurious wakeup (which should be
+                * harmless beyond wasting some cycles) than to lose a wakeup.
+                */
+               proc = NULL;
+               SpinLockAcquire(&cv->mutex);
+               if (!proclist_is_empty(&cv->wakeup))
+                       proc = proclist_pop_head_node(&cv->wakeup, cvWaitLink);
+               have_sentinel = proclist_contains(&cv->wakeup, pgprocno, cvWaitLink);
+               SpinLockRelease(&cv->mutex);
+
+               if (proc != NULL && proc != MyProc)
+               {
+                       SetLatch(&proc->procLatch);
+                       ++nwoken;
+               }
+       }
 
        return nwoken;
 }