From 13db3b936359eebf02a768db3a1959af880b6cc6 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 9 Jan 2018 11:39:10 -0500 Subject: [PATCH] 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 --- src/backend/storage/lmgr/condition_variable.c | 26 ++++++++++--------- src/include/storage/condition_variable.h | 4 +-- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c index 98a67965cd..25c5cd7b45 100644 --- 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 7dac477d25..32e645c02a 100644 --- 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);