Remove return values of ConditionVariableSignal/Broadcast.

In the wake of commit aced5a92b, the semantics of these results are
a bit squishy: we can tell whether we signaled some other process(es),
but we do not know which ones were real waiters versus mere sentinels
for ConditionVariableBroadcast operations.  It does not help much that
ConditionVariableBroadcast will attempt to pass on the signal to the
next real waiter, because (a) there might not be one, and (b) that will
only happen awhile later, anyway.  So these results could overstate how
much effect the calls really had.

However, no existing caller of either function pays any attention to its
result value, so it seems reasonable to just define that as a required
property of a correct algorithm.  To encourage correctness and save some
tiny number of cycles, change both functions to return void.

Patch by me, per an observation by Thomas Munro.  No back-patch, since
if any third parties happen to be using these functions, they might not
appreciate an API break in a minor release.

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 cac3d36b6a3331f9c974564c005b3217090af429..60234db4cd0884eb1ac69e44bb79f74e658b4fda 100644 (file)
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -186,11 +186,14 @@ ConditionVariableCancelSleep(void)
 }
 
 /*
- * Wake up one sleeping process, assuming there is at least one.
+ * Wake up the oldest process sleeping on the CV, if there is any.
  *
- * The return value indicates whether or not we woke somebody up.
+ * Note: it's difficult to tell whether this has any real effect: we know
+ * whether we took an entry off the list, but the entry might only be a
+ * sentinel.  Hence, think twice before proposing that this should return
+ * a flag telling whether it woke somebody.
  */
-bool
+void
 ConditionVariableSignal(ConditionVariable *cv)
 {
        PGPROC     *proc = NULL;
@@ -203,24 +206,19 @@ ConditionVariableSignal(ConditionVariable *cv)
 
        /* If we found someone sleeping, set their latch to wake them up. */
        if (proc != NULL)
-       {
                SetLatch(&proc->procLatch);
-               return true;
-       }
-
-       /* No sleeping processes. */
-       return false;
 }
 
 /*
- * Wake up all sleeping processes.
+ * Wake up all processes sleeping on the given CV.
  *
- * The return value indicates the number of processes we woke.
+ * This guarantees to wake all processes that were sleeping on the CV
+ * at time of call, but processes that add themselves to the list mid-call
+ * will typically not get awakened.
  */
-int
+void
 ConditionVariableBroadcast(ConditionVariable *cv)
 {
-       int                     nwoken = 0;
        int                     pgprocno = MyProc->pgprocno;
        PGPROC     *proc = NULL;
        bool            have_sentinel = false;
@@ -270,10 +268,7 @@ ConditionVariableBroadcast(ConditionVariable *cv)
 
        /* Awaken first waiter, if there was one. */
        if (proc != NULL)
-       {
                SetLatch(&proc->procLatch);
-               ++nwoken;
-       }
 
        while (have_sentinel)
        {
@@ -297,11 +292,6 @@ ConditionVariableBroadcast(ConditionVariable *cv)
                SpinLockRelease(&cv->mutex);
 
                if (proc != NULL && proc != MyProc)
-               {
                        SetLatch(&proc->procLatch);
-                       ++nwoken;
-               }
        }
-
-       return nwoken;
 }

diff --git a/src/include/storage/condition_variable.h b/src/include/storage/condition_variable.h
index f9f93e0d4a265bc76cc31f6ca98a6183ec2be79d..c7afbbca429d287be16fd036615549552a785df8 100644 (file)
--- a/src/include/storage/condition_variable.h
+++ b/src/include/storage/condition_variable.h
@@ -53,7 +53,7 @@ extern void ConditionVariableCancelSleep(void);
 extern void ConditionVariablePrepareToSleep(ConditionVariable *);
 
 /* Wake up a single waiter (via signal) or all waiters (via broadcast). */
-extern bool ConditionVariableSignal(ConditionVariable *);
-extern int     ConditionVariableBroadcast(ConditionVariable *);
+extern void ConditionVariableSignal(ConditionVariable *cv);
+extern void ConditionVariableBroadcast(ConditionVariable *cv);
 
 #endif                                                 /* CONDITION_VARIABLE_H */