Allow ConditionVariable[PrepareTo]Sleep to auto-switch between CVs.

The original coding here insisted that callers manually cancel any prepared
sleep for one condition variable before starting a sleep on another one.
While that's not a huge burden today, it seems like a gotcha that will bite
us in future if the use of condition variables increases; anything we can
do to make the use of this API simpler and more robust is attractive.
Hence, allow these functions to automatically switch their attention to
a different CV when required.  This is safe for the same reason it was OK
for commit aced5a92b to let a broadcast operation cancel any prepared CV
sleep: whenever we return to the other test-and-sleep loop, we will
automatically re-prepare that CV, paying at most an extra test of that
loop's exit condition.

Back-patch to v10 where condition variables were introduced.  Ordinarily
we would probably not back-patch a change like this, but since it does not
invalidate any coding pattern that was legal before, it seems safe enough.
Furthermore, there's an open bug in replorigin_drop() for which the
simplest fix requires this.  Even if we chose to fix that in some more
complicated way, the hazard would remain that we might back-patch some
other bug fix that requires this behavior.

Patch by me, reviewed by Thomas Munro.

Discussion: https://postgr.es/m/2437.1515368316@sss.pgh.pa.us

diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index 98a67965cde37aefb8bfd04dc9ea546cec2ab87a..25c5cd7b45b02cbae571daee3c15c50e0f687853 100644 (file)
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -55,10 +55,6 @@ ConditionVariableInit(ConditionVariable *cv)
  * condition between calling ConditionVariablePrepareToSleep and calling
  * ConditionVariableSleep.  If that is inconvenient, omit calling
  * ConditionVariablePrepareToSleep.
- *
- * Only one condition variable can be used at a time, ie,
- * ConditionVariableCancelSleep must be called before any attempt is made
- * to sleep on a different condition variable.
  */
 void
 ConditionVariablePrepareToSleep(ConditionVariable *cv)
@@ -81,10 +77,15 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
        }
 
        /*
-        * It's not legal to prepare a sleep until the previous sleep has been
-        * completed or canceled.
+        * If some other sleep is already prepared, cancel it; this is necessary
+        * because we have just one static variable tracking the prepared sleep,
+        * and also only one cvWaitLink in our PGPROC.  It's okay to do this
+        * because whenever control does return to the other test-and-sleep loop,
+        * its ConditionVariableSleep call will just re-establish that sleep as
+        * the prepared one.
         */
-       Assert(cv_sleep_target == NULL);
+       if (cv_sleep_target != NULL)
+               ConditionVariableCancelSleep();
 
        /* Record the condition variable on which we will sleep. */
        cv_sleep_target = cv;
@@ -133,16 +134,16 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
         * is recommended because it avoids manipulations of the wait list, or not
         * met initially, in which case preparing first is better because it
         * avoids one extra test of the exit condition.
+        *
+        * If we are currently prepared to sleep on some other CV, we just cancel
+        * that and prepare this one; see ConditionVariablePrepareToSleep.
         */
-       if (cv_sleep_target == NULL)
+       if (cv_sleep_target != cv)
        {
                ConditionVariablePrepareToSleep(cv);
                return;
        }
 
-       /* Any earlier condition variable sleep must have been canceled. */
-       Assert(cv_sleep_target == cv);
-
        do
        {
                CHECK_FOR_INTERRUPTS();
@@ -265,7 +266,8 @@ ConditionVariableBroadcast(ConditionVariable *cv)
         * prepared CV sleep.  The next call to ConditionVariableSleep will take
         * care of re-establishing the lost state.
         */
-       ConditionVariableCancelSleep();
+       if (cv_sleep_target != NULL)
+               ConditionVariableCancelSleep();
 
        /*
         * Inspect the state of the queue.  If it's empty, we have nothing to do.

diff --git a/src/include/storage/condition_variable.h b/src/include/storage/condition_variable.h
index 7dac477d259ad1b81e65970edb344b9b9e874b04..32e645c02a1d10347b9642494094526e1233c758 100644 (file)
--- a/src/include/storage/condition_variable.h
+++ b/src/include/storage/condition_variable.h
@@ -40,9 +40,7 @@ extern void ConditionVariableInit(ConditionVariable *cv);
  * ConditionVariableSleep.  Spurious wakeups are possible, but should be
  * infrequent.  After exiting the loop, ConditionVariableCancelSleep must
  * be called to ensure that the process is no longer in the wait list for
- * the condition variable.  Only one condition variable can be used at a
- * time, ie, ConditionVariableCancelSleep must be called before any attempt
- * is made to sleep on a different condition variable.
+ * the condition variable.
  */
 extern void ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info);
 extern void ConditionVariableCancelSleep(void);