Condition variables vs interrupts
Hi,
While testing something unrelated, Tomas reported[1]/messages/by-id/20191217232124.3dtrycatgfm6oxxb@development that he could
make a parallel worker ignore a SIGTERM and hang forever in
ConditionVariableSleep(). I looked into this and realised that it's
more likely in master. Commit 1321509f refactored the latch wait loop
to look a little bit more like other examples* by putting
CHECK_FOR_INTERRUPTS() after ResetLatch(), whereas previously it was
at the top of the loop. ConditionVariablePrepareToSleep() was
effectively relying on that order when it reset the latch without its
own CFI().
The bug goes back to the introduction of CVs however, because there
was no guarantee that you'd ever reach ConditionVariableSleep(). You
could call ConditionVariablePrepareToSleep(), test your condition,
decide you're done, then call ConditionVariableCancelSleep(), then
reach some other WaitLatch() with no intervening CFI(). It might be
hard to find a code path that actually does that without a
coincidental CFI() to save you, but at least in theory the problem
exists.
I think we should probably just remove the unusual ResetLatch() call,
rather than adding a CFI(). See attached. Thoughts?
*It can't quite be exactly like the two patterns shown in latch.h,
namely { Reset, Test, Wait } and { Test, Wait, Reset }, because the
real test is external to this function; we have the other possible
rotation { Wait, Reset, Test }, and this code is only run if the
client's test failed. Really it's a nested loop, with the outer loop
belonging to the caller, so we have { Test', { Wait, Reset, Test } }.
However, CHECK_FOR_INTERRUPTS() also counts as a test of work to do,
and AFAICS it always belongs between Reset and Wait, no matter how far
you rotate the loop. I wonder if latch.h should mention that.
[1]: /messages/by-id/20191217232124.3dtrycatgfm6oxxb@development
Attachments:
0001-Don-t-call-ResetLatch-in-ConditionVariablePrepareToS.patchapplication/octet-stream; name=0001-Don-t-call-ResetLatch-in-ConditionVariablePrepareToS.patchDownload
From 9e1308c8f208612dd0928449f558e5219a5b202b Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Fri, 20 Dec 2019 09:31:09 +1300
Subject: [PATCH] Don't call ResetLatch() in ConditionVariablePrepareToSleep().
It reset the latch without calling CHECK_FOR_INTERRUPTS(). Let's
just leave the latch set if it's set, so that the next wait loop
(mostly likely ConditionVariableSleep()) sees it and handles it.
One consequence of this bug was that a SIGTERM delivered in a very
narrow timing window could leave a parallel worker process waiting
forever for a condition variable that will never be signaled, after
an error was raised in other process.
Reported-by: Tomas Vondra
Discussion: https://postgr.es/m/20191217232124.3dtrycatgfm6oxxb%40development
---
src/backend/storage/lmgr/condition_variable.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index e08507f0cc..663db95aa8 100644
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -93,12 +93,6 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
/* Record the condition variable on which we will sleep. */
cv_sleep_target = cv;
- /*
- * Reset my latch before adding myself to the queue, to ensure that we
- * don't miss a wakeup that occurs immediately.
- */
- ResetLatch(MyLatch);
-
/* Add myself to the wait queue. */
SpinLockAcquire(&cv->mutex);
proclist_push_tail(&cv->wakeup, pgprocno, cvWaitLink);
--
2.23.0
On Fri, Dec 20, 2019 at 12:05:34PM +1300, Thomas Munro wrote:
I think we should probably just remove the unusual ResetLatch() call,
rather than adding a CFI(). See attached. Thoughts?
I agree: removing the ResetLatch() and having the wait event code deal
with it is a better way to go. I wonder why the reset was done in the
first place?
--
Shawn Debnath
Amazon Web Services (AWS)
On Sat, Dec 21, 2019 at 2:10 PM Shawn Debnath <sdn@amazon.com> wrote:
On Fri, Dec 20, 2019 at 12:05:34PM +1300, Thomas Munro wrote:
I think we should probably just remove the unusual ResetLatch() call,
rather than adding a CFI(). See attached. Thoughts?I agree: removing the ResetLatch() and having the wait event code deal
with it is a better way to go. I wonder why the reset was done in the
first place?
Thanks for the review. I was preparing to commit this today, but I
think I've spotted another latch protocol problem in the stable
branches only and I'd to get some more eyeballs on it. I bet it's
really hard to hit, but ConditionVariableSleep()'s return path (ie
when the CV has been signalled) forgets that the the latch is
multiplexed and latches are merged: just because it was set by
ConditionVariableSignal() doesn't mean it wasn't *also* set by die()
or some other interrupt, and yet at the point we return, we've reset
the latch and not run CFI(), and there's nothing to say the caller
won't then immediately wait on the latch in some other code path, and
sleep like a log despite the earlier delivery of (say) SIGTERM. If
I'm right about that, I think the solution is to move that CFI down in
the stable branches (which you already did for master in commit
1321509f).
Attachments:
0001-Don-t-reset-latch-in-ConditionVariablePrepare-master.patchapplication/octet-stream; name=0001-Don-t-reset-latch-in-ConditionVariablePrepare-master.patchDownload
From 8171af9697470138b936ab0870f3e5dacd306a00 Mon Sep 17 00:00:00 2001
From: Thomas Munro <tmunro@postgresql.org>
Date: Tue, 24 Dec 2019 14:36:16 +1300
Subject: [PATCH] Don't reset latch in ConditionVariablePrepareToSleep().
It's not OK to do that without calling CHECK_FOR_INTERRUPTS(), and
it's probably better to make all latch interaction code follow the
standard pattern.
Let's leave the latch set if it's set, so that the next wait loop
(mostly likely ConditionVariableSleep()) sees it and handles it, if
we don't reach some other CHECK_FOR_INTERRUPTS() first.
One consequence of this bug was that a SIGTERM delivered in a very
narrow timing window could leave a parallel worker process waiting
forever for a condition variable that will never be signaled, after
an error was raised in other process.
Back-patch to 10, where condition variables were introduced. The bug
was probably not easy to hit before commit 1321509f in master, but
it was still a violation of the latch/interrupt protocol, so repair.
In the back-branches only, also move the CHECK_FOR_INTERRUPTS() to
after the ResetLatch() call, to close a related race: a SIGTERM that
arrives around that same time as a condition variable signal could
be merged, and the ConditionVariableSleep() could return having
reset the latch and noticed the CV signal, and then the caller could
wait on a latch.
Author: Thomas Munro
Reviewed-by: Shawn Debnath
Reported-by: Tomas Vondra
Discussion: https://postgr.es/m/CA%2BhUKGJOm8zZHjVA8svoNT3tHY0XdqmaC_kHitmgXDQM49m1dA%40mail.gmail.com
---
src/backend/storage/lmgr/condition_variable.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index e08507f0cc..663db95aa8 100644
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -93,12 +93,6 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
/* Record the condition variable on which we will sleep. */
cv_sleep_target = cv;
- /*
- * Reset my latch before adding myself to the queue, to ensure that we
- * don't miss a wakeup that occurs immediately.
- */
- ResetLatch(MyLatch);
-
/* Add myself to the wait queue. */
SpinLockAcquire(&cv->mutex);
proclist_push_tail(&cv->wakeup, pgprocno, cvWaitLink);
--
2.23.0
0001-Don-t-reset-latch-in-ConditionVariableP-backbranches.patchapplication/octet-stream; name=0001-Don-t-reset-latch-in-ConditionVariableP-backbranches.patchDownload
From 3c53488da4557972e5f62b0a3566c2783427c0c5 Mon Sep 17 00:00:00 2001
From: Thomas Munro <tmunro@postgresql.org>
Date: Tue, 24 Dec 2019 14:36:16 +1300
Subject: [PATCH] Don't reset latch in ConditionVariablePrepareToSleep().
It's not OK to do that without calling CHECK_FOR_INTERRUPTS(), and
it's probably better to make all latch interaction code follow the
standard pattern.
Let's leave the latch set if it's set, so that the next wait loop
(mostly likely ConditionVariableSleep()) sees it and handles it, if
we don't reach some other CHECK_FOR_INTERRUPTS() first.
One consequence of this bug was that a SIGTERM delivered in a very
narrow timing window could leave a parallel worker process waiting
forever for a condition variable that will never be signaled, after
an error was raised in other process.
Back-patch to 10, where condition variables were introduced. The bug
was probably not easy to hit before commit 1321509f in master, but
it was still a violation of the latch/interrupt protocol, so repair.
In the back-branches only, also move the CHECK_FOR_INTERRUPTS() to
after the ResetLatch() call, to close a related race: a SIGTERM that
arrives around that same time as a condition variable signal could
be merged, and the ConditionVariableSleep() could return having
reset the latch and noticed the CV signal, and then the caller could
wait on a latch.
Author: Thomas Munro
Reviewed-by: Shawn Debnath
Reported-by: Tomas Vondra
Discussion: https://postgr.es/m/CA%2BhUKGJOm8zZHjVA8svoNT3tHY0XdqmaC_kHitmgXDQM49m1dA%40mail.gmail.com
---
src/backend/storage/lmgr/condition_variable.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index 58b7b51472..6c4dbaec1c 100644
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -92,12 +92,6 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
/* Record the condition variable on which we will sleep. */
cv_sleep_target = cv;
- /*
- * Reset my latch before adding myself to the queue, to ensure that we
- * don't miss a wakeup that occurs immediately.
- */
- ResetLatch(MyLatch);
-
/* Add myself to the wait queue. */
SpinLockAcquire(&cv->mutex);
proclist_push_tail(&cv->wakeup, pgprocno, cvWaitLink);
@@ -148,8 +142,6 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
do
{
- CHECK_FOR_INTERRUPTS();
-
/*
* Wait for latch to be set. (If we're awakened for some other
* reason, the code below will cope anyway.)
@@ -160,6 +152,8 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
/* Reset latch before examining the state of the wait list. */
ResetLatch(MyLatch);
+ CHECK_FOR_INTERRUPTS();
+
/*
* If this process has been taken out of the wait list, then we know
* that it has been signaled by ConditionVariableSignal (or
--
2.23.0
On Tue, Dec 24, 2019 at 3:10 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Sat, Dec 21, 2019 at 2:10 PM Shawn Debnath <sdn@amazon.com> wrote:
On Fri, Dec 20, 2019 at 12:05:34PM +1300, Thomas Munro wrote:
I think we should probably just remove the unusual ResetLatch() call,
rather than adding a CFI(). See attached. Thoughts?I agree: removing the ResetLatch() and having the wait event code deal
with it is a better way to go. I wonder why the reset was done in the
first place?
I have pushed this on master only.
Thanks for the review. I was preparing to commit this today, but I
think I've spotted another latch protocol problem in the stable
branches only and I'd to get some more eyeballs on it. I bet it's
really hard to hit, but ConditionVariableSleep()'s return path (ie
when the CV has been signalled) forgets that the the latch is
multiplexed and latches are merged: just because it was set by
ConditionVariableSignal() doesn't mean it wasn't *also* set by die()
or some other interrupt, and yet at the point we return, we've reset
the latch and not run CFI(), and there's nothing to say the caller
won't then immediately wait on the latch in some other code path, and
sleep like a log despite the earlier delivery of (say) SIGTERM. If
I'm right about that, I think the solution is to move that CFI down in
the stable branches (which you already did for master in commit
1321509f).
I'm not going to back-patch this (ie the back-branches version from my
previous email) without more discussion, but I still think it's subtly
broken.