Skip to content

Commit

Permalink
De-pessimize ConditionVariableCancelSleep().
Browse files Browse the repository at this point in the history
Commit b91dd9d was concerned with a theoretical problem with our
non-atomic condition variable operations.  If you stop sleeping, and
then cancel the sleep in a separate step, you might be signaled in
between, and that could be lost.  That doesn't matter for callers of
ConditionVariableBroadcast(), but callers of ConditionVariableSignal()
might be upset if a signal went missing like this.

Commit bc971f4 interacted badly with that logic, because it doesn't
use ConditionVariableSleep(), which would normally put us back in the
wait list.  ConditionVariableCancelSleep() would be confused and think
we'd received an extra signal, and try to forward it to another backend,
resulting in wakeup storms.

New idea: ConditionVariableCancelSleep() can just return true if we've
been signaled.  Hypothetical users of ConditionVariableSignal() would
then still have a way to deal with rare lost signals if they are
concerned about that problem.

Back-patch to 16, where bc971f4 arrived.

Reported-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/2840876b-4cfe-240f-0a7e-29ffd66711e7%40enterprisedb.com
  • Loading branch information
macdice committed Aug 14, 2023
1 parent 82a4eda commit 5ffb7c7
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 11 deletions.
16 changes: 6 additions & 10 deletions src/backend/storage/lmgr/condition_variable.c
Original file line number Diff line number Diff line change
Expand Up @@ -223,15 +223,17 @@ ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
*
* Do nothing if nothing is pending; this allows this function to be called
* during transaction abort to clean up any unfinished CV sleep.
*
* Return true if we've been signaled.
*/
void
bool
ConditionVariableCancelSleep(void)
{
ConditionVariable *cv = cv_sleep_target;
bool signaled = false;

if (cv == NULL)
return;
return false;

SpinLockAcquire(&cv->mutex);
if (proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink))
Expand All @@ -240,15 +242,9 @@ ConditionVariableCancelSleep(void)
signaled = true;
SpinLockRelease(&cv->mutex);

/*
* If we've received a signal, pass it on to another waiting process, if
* there is one. Otherwise a call to ConditionVariableSignal() might get
* lost, despite there being another process ready to handle it.
*/
if (signaled)
ConditionVariableSignal(cv);

cv_sleep_target = NULL;

return signaled;
}

/*
Expand Down
2 changes: 1 addition & 1 deletion src/include/storage/condition_variable.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ extern void ConditionVariableInit(ConditionVariable *cv);
extern void ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info);
extern bool ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
uint32 wait_event_info);
extern void ConditionVariableCancelSleep(void);
extern bool ConditionVariableCancelSleep(void);

/*
* Optionally, ConditionVariablePrepareToSleep can be called before entering
Expand Down

0 comments on commit 5ffb7c7

Please sign in to comment.