Introduce timeout capability for ConditionVariableSleep

Hi Shawn,

On Wed, Mar 13, 2019 at 12:25 PM Shawn Debnath <sdn@amazon.com> wrote:

Postgres today doesn't support waiting for a condition variable with a
timeout, although the framework it relies upon, does. This change wraps
the existing ConditionVariableSleep functionality and introduces a new
API, ConditionVariableTimedSleep, to allow callers to specify a timeout
value.

Seems reasonable, I think, and should be familiar to anyone used to
well known multithreading libraries.

+/*
+ * Wait for the given condition variable to be signaled or till timeout.
+ * This should be called in a predicate loop that tests for a specific exit
+ * condition and otherwise sleeps, like so:
+ *
+ *     ConditionVariablePrepareToSleep(cv);  // optional
+ *     while (condition for which we are waiting is not true)
+ *         ConditionVariableSleep(cv, wait_event_info);
+ *     ConditionVariableCancelSleep();
+ *
+ * wait_event_info should be a value from one of the WaitEventXXX enums
+ * defined in pgstat.h.  This controls the contents of pg_stat_activity's
+ * wait_event_type and wait_event columns while waiting.
+ *
+ * Returns 0 or -1 if timed out.
+ */
+int
+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
+                            uint32 wait_event_info)

Can we just refer to the other function's documentation for this? I
don't want two copies of this blurb (and this copy-paste already
failed to include "Timed" in the example function name).

One difference compared to pthread_cond_timedwait() is that pthread
uses an absolute time and here you use a relative time (as we do in
WaitEventSet API). The first question is which makes a better API,
and the second is what the semantics of a relative timeout should be:
start again every time we get a spurious wake-up, or track time
already waited? Let's see...

-        (void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,
+        ret = WaitEventSetWait(cv_wait_event_set, timeout, &event, 1,
                                 wait_event_info);

Here you're restarting the timeout clock every time through the loop
without adjustment, and I think that's not a good choice: spurious
wake-ups cause bonus waiting.

--
Thomas Munro
https://enterprisedb.com

Hi Thomas,

Thanks for reviewing!

On Wed, Mar 13, 2019 at 12:40:57PM +1300, Thomas Munro wrote:

Can we just refer to the other function's documentation for this? I
don't want two copies of this blurb (and this copy-paste already
failed to include "Timed" in the example function name).

Hah - point well taken. Fixed.

One difference compared to pthread_cond_timedwait() is that pthread
uses an absolute time and here you use a relative time (as we do in
WaitEventSet API). The first question is which makes a better API,
and the second is what the semantics of a relative timeout should be:
start again every time we get a spurious wake-up, or track time
already waited? Let's see...

-        (void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,
+        ret = WaitEventSetWait(cv_wait_event_set, timeout, &event, 1,
wait_event_info);

Here you're restarting the timeout clock every time through the loop
without adjustment, and I think that's not a good choice: spurious
wake-ups cause bonus waiting.

Agree. In my testing WaitEventSetWait did the work for calculating the
right timeout remaining. It's a bummer that we have to repeat the same
pattern at the ConditionVariableTimedSleep() but I guess anyone who
loops in such cases will have to adjust their values accordingly.

BTW, I am curious why Andres in 98a64d0bd71 didn't just create an
artificial event with WL_TIMEOUT and return that from
WaitEventSetWait(). Would have made it cleaner than checking checking
return values for -1.

Updated v2 patch attached.

--
Shawn Debnath
Amazon Web Services (AWS)

Hello.

At Tue, 12 Mar 2019 17:53:43 -0700, Shawn Debnath <sdn@amazon.com> wrote in <20190313005342.GA8301@f01898859afd.ant.amazon.com>

Hi Thomas,

Thanks for reviewing!

On Wed, Mar 13, 2019 at 12:40:57PM +1300, Thomas Munro wrote:

Can we just refer to the other function's documentation for this? I
don't want two copies of this blurb (and this copy-paste already
failed to include "Timed" in the example function name).

Hah - point well taken. Fixed.

One difference compared to pthread_cond_timedwait() is that pthread
uses an absolute time and here you use a relative time (as we do in
WaitEventSet API). The first question is which makes a better API,
and the second is what the semantics of a relative timeout should be:
start again every time we get a spurious wake-up, or track time
already waited? Let's see...

-        (void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,
+        ret = WaitEventSetWait(cv_wait_event_set, timeout, &event, 1,
wait_event_info);

Here you're restarting the timeout clock every time through the loop
without adjustment, and I think that's not a good choice: spurious
wake-ups cause bonus waiting.

Agree. In my testing WaitEventSetWait did the work for calculating the
right timeout remaining. It's a bummer that we have to repeat the same
pattern at the ConditionVariableTimedSleep() but I guess anyone who
loops in such cases will have to adjust their values accordingly.

I think so, too. And actually the duplicate timeout calculation
doesn't seem good. We could eliminate the duplicate by allowing
WaitEventSetWait to exit by unwanted events, something like the
attached.

BTW, I am curious why Andres in 98a64d0bd71 didn't just create an
artificial event with WL_TIMEOUT and return that from
WaitEventSetWait(). Would have made it cleaner than checking checking
return values for -1.

Maybe because it is not a kind of WaitEvent, so it naturally
cannot be a part of occurred_events.

# By the way, you can obtain a short hash of a commit by git
# rev-parse --short <full hash>.

Updated v2 patch attached.

I'd like to comment on the patch.

+ * In the event of a timeout, we simply return and the caller
+ * calls ConditionVariableCancelSleep to remove themselves from the
+ * wait queue. See ConditionVariableSleep() for notes on how to correctly check
+ * for the exit condition.
+ *
+ * Returns 0, or -1 if timed out.

Maybe this could be more simpler, that like:

* ConditionVariableTimedSleep - allows us to specify timeout
*
* If timeout = =1, block until the condition is satisfied.
*
* Returns -1 when timeout expires, otherwise returns 0.
*
* See ConditionVariableSleep() for general behavior and usage.

+int
+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,

Counldn't the two-state return value be a boolean?

+ int ret = 0;

As a general coding convention, we are not to give such a generic
name for a variable with such a long life if avoidable. In the
case the 'ret' could be 'timeout_fired' or something and it would
be far verbose.

+ if (rc == 0 && timeout >= 0)

WaitEventSetWait returns 0 only in the case of timeout
expiration, so the second term is useless. Just setting ret to
-1 and break seems to me almost the same with "goto". The reason
why the existing ConditionVariableSleep uses do {} while(done) is
that it is straightforwad. Timeout added everal exit point in the
loop so it's make the loop rather complex by going around with
the variable. Whole the loop could be in the following more flat
shape.

while (true)
{
CHECK_FOR_INTERRUPTS();
rc = WaitEventSetWait();
ResetLatch();

/* timeout expired, return */
if (rc == 0) return -1;
SpinLockAcquire();
if (!proclist...)
{
done = true;
}
SpinLockRelease();

/* condition satisfied, return */
if (done) return 0;

/* if we're here, we should wait for the remaining time */
INSTR_TIME_SET_CURRENT()
...
}

+ Assert(ret == 0);

I don't see a point in the assertion so much.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Thank you reviewing. Comments inline.

On Wed, Mar 13, 2019 at 05:24:15PM +0900, Kyotaro HORIGUCHI wrote:

Agree. In my testing WaitEventSetWait did the work for calculating
the right timeout remaining. It's a bummer that we have to repeat
the same pattern at the ConditionVariableTimedSleep() but I guess
anyone who loops in such cases will have to adjust their values
accordingly.

I think so, too. And actually the duplicate timeout calculation
doesn't seem good. We could eliminate the duplicate by allowing
WaitEventSetWait to exit by unwanted events, something like the
attached.

After thinking about this more, I see WaitEventSetWait()'s contract as
wait for an event or timeout if no events are received in that time
frame. Although ConditionVariableTimedSleep() is also using the same
word, I believe the semantics are different. The timeout period in
ConditionVariableTimedSleep() is intended to limit the time we wait
until removal from the wait queue. Whereas, in the case of
WaitEventSetWait, the timeout period is intended to limit the time the
caller waits till the first event.

Hence, I believe the code is correct as is and we shouldn't change the
contract for WaitEventSetWait.

BTW, I am curious why Andres in 98a64d0bd71 didn't just create an
artificial event with WL_TIMEOUT and return that from
WaitEventSetWait(). Would have made it cleaner than checking checking
return values for -1.

Maybe because it is not a kind of WaitEvent, so it naturally
cannot be a part of occurred_events.

Hmm, I don't agree with that completely. One could argue that the
backend is waiting for any event in order to be woken up, including a
timeout event.

# By the way, you can obtain a short hash of a commit by git
# rev-parse --short <full hash>.

Good to know! :-) Luckily git is smart enough to still match it to the
correct commit.

Updated v2 patch attached.

I'd like to comment on the patch.

+ * In the event of a timeout, we simply return and the caller
+ * calls ConditionVariableCancelSleep to remove themselves from the
+ * wait queue. See ConditionVariableSleep() for notes on how to correctly check
+ * for the exit condition.
+ *
+ * Returns 0, or -1 if timed out.

Maybe this could be more simpler, that like:

* ConditionVariableTimedSleep - allows us to specify timeout
*
* If timeout = =1, block until the condition is satisfied.
*
* Returns -1 when timeout expires, otherwise returns 0.
*
* See ConditionVariableSleep() for general behavior and usage.

Agree. Changed to:

* Wait for the given condition variable to be signaled or till timeout.
*
* Returns -1 when timeout expires, otherwise returns 0.
*
* See ConditionVariableSleep() for general usage.

+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,

Counldn't the two-state return value be a boolean?

I wanted to leave the option open to use the positive integers for other
purposes but you are correct, bool suffices for now. If needed, we can
change it in the future.

+ int ret = 0;

As a general coding convention, we are not to give such a generic
name for a variable with such a long life if avoidable. In the
case the 'ret' could be 'timeout_fired' or something and it would
be far verbose.

+ if (rc == 0 && timeout >= 0)

WaitEventSetWait returns 0 only in the case of timeout
expiration, so the second term is useless. Just setting ret to
-1 and break seems to me almost the same with "goto". The reason
why the existing ConditionVariableSleep uses do {} while(done) is
that it is straightforwad. Timeout added everal exit point in the
loop so it's make the loop rather complex by going around with
the variable. Whole the loop could be in the following more flat
shape.

while (true)
{
CHECK_FOR_INTERRUPTS();
rc = WaitEventSetWait();
ResetLatch();

/* timeout expired, return */
if (rc == 0) return -1;
SpinLockAcquire();
if (!proclist...)
{
done = true;
}
SpinLockRelease();

/* condition satisfied, return */
if (done) return 0;

/* if we're here, we should wait for the remaining time */
INSTR_TIME_SET_CURRENT()
...
}

Agree. The timeout did complicate the logic for a single variable to
track the exit condition. Adopted the approach above.

+ Assert(ret == 0);

I don't see a point in the assertion so much.

Being overly verbose. Removed.

--
Shawn Debnath
Amazon Web Services (AWS)

Hello.

At Thu, 14 Mar 2019 17:26:11 -0700, Shawn Debnath <sdn@amazon.com> wrote in <20190315002611.GA1119@f01898859afd.ant.amazon.com>

Thank you reviewing. Comments inline.

On Wed, Mar 13, 2019 at 05:24:15PM +0900, Kyotaro HORIGUCHI wrote:

Agree. In my testing WaitEventSetWait did the work for calculating
the right timeout remaining. It's a bummer that we have to repeat
the same pattern at the ConditionVariableTimedSleep() but I guess
anyone who loops in such cases will have to adjust their values
accordingly.

I think so, too. And actually the duplicate timeout calculation
doesn't seem good. We could eliminate the duplicate by allowing
WaitEventSetWait to exit by unwanted events, something like the
attached.

After thinking about this more, I see WaitEventSetWait()'s contract as
wait for an event or timeout if no events are received in that time

Sure.

frame. Although ConditionVariableTimedSleep() is also using the same
word, I believe the semantics are different. The timeout period in
ConditionVariableTimedSleep() is intended to limit the time we wait
until removal from the wait queue. Whereas, in the case of
WaitEventSetWait, the timeout period is intended to limit the time the
caller waits till the first event.

Mmm. The two look the same to me.. Timeout means for both that
"Wait for one of these events or timeout expiration to
occur". Removal from waiting queue is just a subtask of exiting
from waiting state.

The "don't exit until timeout expires unless any expected events
occur" part is to be done in the uppermost layer so it is enough
that the lower layer does just "exit when something
happened". This is the behavior of WaitEventSetWaitBlock for
WaitEventSetWait. My proposal is giving callers capabliy to tell
WaitEventSetWait not to perform useless timeout contination.

Hence, I believe the code is correct as is and we shouldn't change the
contract for WaitEventSetWait.

BTW, I am curious why Andres in 98a64d0bd71 didn't just create an
artificial event with WL_TIMEOUT and return that from
WaitEventSetWait(). Would have made it cleaner than checking checking
return values for -1.

Maybe because it is not a kind of WaitEvent, so it naturally
cannot be a part of occurred_events.

Hmm, I don't agree with that completely. One could argue that the
backend is waiting for any event in order to be woken up, including a
timeout event.

Right, I understand that. I didn't mean that it is the right
design for everyone. Just meant that it is in that shape. (And I
rather like it.)

latch.h:127
#define WL_TIMEOUT (1 << 3) /* not for WaitEventSetWait() */

We can make it one of the events for WaitEventSetWait, but I
don't see such a big point on that, and also that doesn't this
patch better in any means.

# By the way, you can obtain a short hash of a commit by git
# rev-parse --short <full hash>.

Good to know! :-) Luckily git is smart enough to still match it to the
correct commit.

And too complex so that infrequent usage easily get out from my
brain:(

Updated v2 patch attached.

Thank you . It looks fine execpt the above point. But still I
have some questions on it. (the reason for they not being
comments is that they are about wordings..).

+     * Track the current time so that we can calculate the remaining timeout
+     * if we are woken up spuriously.

I think tha "track" means chasing a moving objects. So it might
be bettter that it is record or something?

* Wait for the given condition variable to be signaled or till timeout.
*
* Returns -1 when timeout expires, otherwise returns 0.
*
* See ConditionVariableSleep() for general usage.

+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,

Counldn't the two-state return value be a boolean?

I wanted to leave the option open to use the positive integers for other
purposes but you are correct, bool suffices for now. If needed, we can
change it in the future.

Yes, we can do that after we found it needed.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Mar 15, 2019 at 02:15:17PM +0900, Kyotaro HORIGUCHI wrote:

After thinking about this more, I see WaitEventSetWait()'s contract
as wait for an event or timeout if no events are received in that
time

Sure.

frame. Although ConditionVariableTimedSleep() is also using the same
word, I believe the semantics are different. The timeout period in
ConditionVariableTimedSleep() is intended to limit the time we wait
until removal from the wait queue. Whereas, in the case of
WaitEventSetWait, the timeout period is intended to limit the time the
caller waits till the first event.

Mmm. The two look the same to me.. Timeout means for both that
"Wait for one of these events or timeout expiration to
occur". Removal from waiting queue is just a subtask of exiting
from waiting state.

The "don't exit until timeout expires unless any expected events
occur" part is to be done in the uppermost layer so it is enough
that the lower layer does just "exit when something
happened".

Agree with the fact that lower layers should return and let the upper
layer determine and filter events as needed.

This is the behavior of WaitEventSetWaitBlock for
WaitEventSetWait. My proposal is giving callers capabliy to tell
WaitEventSetWait not to perform useless timeout contination.

This is where I disagree. WaitEventSetWait needs its own loop and
timeout calculation because WaitEventSetWaitBlock can return when EINTR
is received. This gets filtered in WaitEventSetWait and doesn't bubble
up by design. Since it's involved in filtering events, it now also has
to manage the timeout value. ConditionVariableTimedSleep being at a
higher level, and waitng for certain events that the lower layers are
unaware of, also shares the timeout management reponsibility.

Do note that there is no performance impact of having multiple timeout
loops. The current design allows for each layer to filter events and
hence per layer timeout management seems fine. If one would want to
avoid this, perhaps we need to introduce a non-static version of
WaitEventSetWaitBlock and call that directly. But that of course is
beyond this patch.

Thank you . It looks fine execpt the above point. But still I
have some questions on it. (the reason for they not being
comments is that they are about wordings..).

+     * Track the current time so that we can calculate the remaining timeout
+     * if we are woken up spuriously.

I think tha "track" means chasing a moving objects. So it might
be bettter that it is record or something?

* Wait for the given condition variable to be signaled or till timeout.
*
* Returns -1 when timeout expires, otherwise returns 0.
*
* See ConditionVariableSleep() for general usage.

+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,

Counldn't the two-state return value be a boolean?

I will change it to Record in the next iteration of the patch.

--
Shawn Debnath
Amazon Web Services (AWS)

On Sat, Mar 16, 2019 at 03:27:17PM -0700, Shawn Debnath wrote:

+     * Track the current time so that we can calculate the 
remaining timeout
+     * if we are woken up spuriously.

I think tha "track" means chasing a moving objects. So it might
be bettter that it is record or something?

* Wait for the given condition variable to be signaled or till timeout.
*
* Returns -1 when timeout expires, otherwise returns 0.
*
* See ConditionVariableSleep() for general usage.

+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,

Counldn't the two-state return value be a boolean?

I will change it to Record in the next iteration of the patch.

Posting rebased and updated patch. Changed the word 'Track' to 'Record'
and also changed variable name rem_timeout to cur_timeout to match
naming in other use cases.

--
Shawn Debnath
Amazon Web Services (AWS)

On Fri, Mar 22, 2019 at 7:21 AM Shawn Debnath <sdn@amazon.com> wrote:

On Sat, Mar 16, 2019 at 03:27:17PM -0700, Shawn Debnath wrote:

+     * Track the current time so that we can calculate the
remaining timeout
+     * if we are woken up spuriously.

I think tha "track" means chasing a moving objects. So it might
be bettter that it is record or something?

* Wait for the given condition variable to be signaled or till timeout.
*
* Returns -1 when timeout expires, otherwise returns 0.
*
* See ConditionVariableSleep() for general usage.

+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,

Counldn't the two-state return value be a boolean?

I will change it to Record in the next iteration of the patch.

Posting rebased and updated patch. Changed the word 'Track' to 'Record'
and also changed variable name rem_timeout to cur_timeout to match
naming in other use cases.

Hi Shawn,

I think this is looking pretty good and I'm planning to commit it
soon. The convention for CHECK_FOR_INTERRUPTS() in latch wait loops
seems to be to put it after the ResetLatch(), so I've moved it in the
attached version (though I don't think it was wrong where it was).
Also pgindented and with my proposed commit message. I've also
attached a throw-away test module that gives you CALL poke() and
SELECT wait_for_poke(timeout) using a CV.

Observations that I'm not planning to do anything about:
1. I don't really like the data type "long", but it's already
established that we use that for latches so maybe it's too late for me
to complain about that.
2. I don't really like the fact that we have to do floating point
stuff in INSTR_TIME_GET_MILLISEC(). That's not really your patch's
fault and you've copied the timeout adjustment code from latch.c,
which seems reasonable.

--
Thomas Munro
https://enterprisedb.com

On Fri, Jul 5, 2019 at 1:40 PM Thomas Munro <thomas.munro@gmail.com> wrote:

I think this is looking pretty good and I'm planning to commit it
soon. The convention for CHECK_FOR_INTERRUPTS() in latch wait loops
seems to be to put it after the ResetLatch(), so I've moved it in the
attached version (though I don't think it was wrong where it was).
Also pgindented and with my proposed commit message. I've also
attached a throw-away test module that gives you CALL poke() and
SELECT wait_for_poke(timeout) using a CV.

I thought of one small problem with the current coding. Suppose there
are two processes A and B waiting on a CV, and another process C calls
ConditionVariableSignal() to signal one process. Around the same
time, A times out and exits via this code path:

+               /* Timed out */
+               if (rc == 0)
+                       return true;

Suppose ConditionVariableSignal() set A's latch immediately after
WaitEventSetWait() returned 0 in A. Now A won't report the CV signal
to the caller, and B is still waiting, so effectively nobody has
received the message and yet C thinks it has signalled a waiter if
there is one. My first thought is that we could simply remove the
above-quoted hunk and fall through to the second timeout-detecting
code. That'd mean that if we've been signalled AND timed out as of
that point in the code, we'll prefer to report the signal, and it also
reduces the complexity of the function to have only one "return true"
path.

That still leaves the danger that the CV can be signalled some time
after ConditionVariableTimedSleep() returns. So now I'm wondering if
ConditionVariableCancelSleep() should signal the CV if it discovers
that this process is not in the proclist, on the basis that that must
indicate that we've been signalled even though we're not interested in
the message anymore, and yet some other process else might be
interested, and that might have been the only signal that is ever
going to be delivered by ConditionVariableSignal().

--
Thomas Munro
https://enterprisedb.com

On Sun, Jul 7, 2019 at 3:09 PM Thomas Munro <thomas.munro@gmail.com> wrote:

+               /* Timed out */
+               if (rc == 0)
+                       return true;

Here's a version without that bit, because I don't think we need it.

That still leaves the danger that the CV can be signalled some time
after ConditionVariableTimedSleep() returns. So now I'm wondering if
ConditionVariableCancelSleep() should signal the CV if it discovers
that this process is not in the proclist, on the basis that that must
indicate that we've been signalled even though we're not interested in
the message anymore, and yet some other process else might be
interested, and that might have been the only signal that is ever
going to be delivered by ConditionVariableSignal().

And a separate patch to do that. Thoughts?

--
Thomas Munro
https://enterprisedb.com

On Tue, Jul 09, 2019 at 11:03:18PM +1200, Thomas Munro wrote:

On Sun, Jul 7, 2019 at 3:09 PM Thomas Munro <thomas.munro@gmail.com> wrote:

+               /* Timed out */
+               if (rc == 0)
+                       return true;

Here's a version without that bit, because I don't think we need it.

This works. Agree that letting it fall through covers the first gap.

That still leaves the danger that the CV can be signalled some time
after ConditionVariableTimedSleep() returns. So now I'm wondering if
ConditionVariableCancelSleep() should signal the CV if it discovers
that this process is not in the proclist, on the basis that that must
indicate that we've been signalled even though we're not interested in
the message anymore, and yet some other process else might be
interested, and that might have been the only signal that is ever
going to be delivered by ConditionVariableSignal().

And a separate patch to do that. Thoughts?

I like it. This covers the gap all the way till cancel is invoked and it
manipulates the list to remove itself or realizes that it needs to
forward the signal to some other process.

Thanks Thomas!

--
Shawn Debnath
Amazon Web Services (AWS)

On Fri, Jul 12, 2019 at 6:08 PM Shawn Debnath <sdn@amazon.com> wrote:

On Tue, Jul 09, 2019 at 11:03:18PM +1200, Thomas Munro wrote:

On Sun, Jul 7, 2019 at 3:09 PM Thomas Munro <thomas.munro@gmail.com> wrote:

+               /* Timed out */
+               if (rc == 0)
+                       return true;

Here's a version without that bit, because I don't think we need it.

This works. Agree that letting it fall through covers the first gap.

Pushed, like that (with the now unused rc variable also removed).
Thanks for the patch!

That still leaves the danger that the CV can be signalled some time
after ConditionVariableTimedSleep() returns. So now I'm wondering if
ConditionVariableCancelSleep() should signal the CV if it discovers
that this process is not in the proclist, on the basis that that must
indicate that we've been signalled even though we're not interested in
the message anymore, and yet some other process else might be
interested, and that might have been the only signal that is ever
going to be delivered by ConditionVariableSignal().

And a separate patch to do that. Thoughts?

I like it. This covers the gap all the way till cancel is invoked and it
manipulates the list to remove itself or realizes that it needs to
forward the signal to some other process.

I pushed this too. It's a separate commit, because I think there is
at least a theoretical argument that it should be back-patched. I'm
not going to do that today though, because I doubt anyone is relying
on ConditionVariableSignal() working that reliably yet, and it's
really with timeouts that it becomes a likely problem.

I thought about this edge case because I have long wanted to propose a
pair of functions that provide a simplified payloadless blocking
alternative to NOTIFY, that would allow for just the right number of
waiting sessions to wake up to handle SKIP LOCKED-style job queues.
Otherwise you sometimes get thundering herds of wakeups fighting over
crumbs. That made me think about the case where a worker session
decides to time out and shut down due to being idle for too long, but
eats a wakeup on its way out. Another question that comes up in that
use case is whether CV wakeup queues should be LIFO or FIFO. I think
the answer is LIFO, to support class worker pool designs that
stabilise at the right size using a simple idle timeout rule. They're
currently FIFO (proclist_pop_head_node() to wake up, but
proclist_push_tail() to sleep). I understand why Robert didn't care
about that last time I mentioned it: all our uses of CVs today are
"broadcast" wakeups. But a productised version of the "poke" hack I
showed earlier that supports poking just one waiter would care about
the thing this patch fixed, and also the wakeup queue order.

--
Thomas Munro
https://enterprisedb.com

On Sat, Jul 13, 2019 at 03:02:25PM +1200, Thomas Munro wrote:

Pushed, like that (with the now unused rc variable also removed).
Thanks for the patch!

Awesome - thank you!

--
Shawn Debnath
Amazon Web Services (AWS)

On Fri, Jul 12, 2019 at 11:03 PM Thomas Munro <thomas.munro@gmail.com> wrote:

I pushed this too. It's a separate commit, because I think there is
at least a theoretical argument that it should be back-patched. I'm
not going to do that today though, because I doubt anyone is relying
on ConditionVariableSignal() working that reliably yet, and it's
really with timeouts that it becomes a likely problem.

To make it work reliably, you'd need to be sure that a process can't
ERROR or FATAL after getting signaled and before doing whatever the
associated work is (or that if it does, it will first pass on the
signal). Since that seems impossible, I'm not sure I see the point of
trying to do anything at all.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jul 16, 2019 at 1:11 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Jul 12, 2019 at 11:03 PM Thomas Munro <thomas.munro@gmail.com> wrote:

I pushed this too. It's a separate commit, because I think there is
at least a theoretical argument that it should be back-patched. I'm
not going to do that today though, because I doubt anyone is relying
on ConditionVariableSignal() working that reliably yet, and it's
really with timeouts that it becomes a likely problem.

To make it work reliably, you'd need to be sure that a process can't
ERROR or FATAL after getting signaled and before doing whatever the
associated work is (or that if it does, it will first pass on the
signal). Since that seems impossible, I'm not sure I see the point of
trying to do anything at all.

I agree that that on its own doesn't fix problems in <some
non-existent client of this facility>, but that doesn't mean we
shouldn't try to make this API as reliable as possible. Unlike
typical CV implementations, our wait primitive is not atomic. When we
invented two-step wait, we created a way for ConditionVariableSignal()
to have no effect due to bad timing. Surely that's a bug.

--
Thomas Munro
https://enterprisedb.com