Horton

Thread

Commits

  1. Forward received condition variable signals on cancel.

  2. Introduce timed waits for condition variables.

  1. Introduce timeout capability for ConditionVariableSleep

    Shawn Debnath <sdn@amazon.com> — 2019-03-12T23:24:54Z

    Hello,

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.

A scenario that highlights this use case is a backend is waiting on 
status update from multiple workers but needs to time out if that signal 
doesn't arrive within a certain period. There was a workaround prior 
to aced5a92, but with that change, the semantics are now different.

I chose to go with -1 instead of 0 for the return from 
ConditionVariableTimedSleep to indicate timeout error  as it seems 
cleaner for this API. WaitEventSetWaitBlock returns -1 for timeout but 
WaitEventSetWait treats timeout as 0 (to represent 0 events indicating 
timeout).

If there's an alternative, cleaner way to achieve this outcome, I am all 
ears.

Thanks.

-- 
Shawn Debnath
Amazon Web Services (AWS)

  2. Re: Introduce timeout capability for ConditionVariableSleep

    Thomas Munro <thomas.munro@gmail.com> — 2019-03-12T23:40:57Z

    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

  3. Re: Introduce timeout capability for ConditionVariableSleep

    Shawn Debnath <sdn@amazon.com> — 2019-03-13T00:53:43Z

    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)

  4. Re: Introduce timeout capability for ConditionVariableSleep

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-03-13T08:24:15Z

    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

  5. Re: Introduce timeout capability for ConditionVariableSleep

    Shawn Debnath <sdn@amazon.com> — 2019-03-15T00:26:11Z

    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)

  6. Re: Introduce timeout capability for ConditionVariableSleep

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-03-15T05:15:17Z

    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

  7. Re: Introduce timeout capability for ConditionVariableSleep

    Shawn Debnath <sdn@amazon.com> — 2019-03-16T22:27:17Z

    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)

  8. Re: Introduce timeout capability for ConditionVariableSleep

    Shawn Debnath <sdn@amazon.com> — 2019-03-21T18:20:50Z

    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)

  9. Re: Introduce timeout capability for ConditionVariableSleep

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-05T01:40:02Z

    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

  10. Re: Introduce timeout capability for ConditionVariableSleep

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-07T03:09:39Z

    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

  11. Re: Introduce timeout capability for ConditionVariableSleep

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-09T11:03:18Z

    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

  12. Re: Introduce timeout capability for ConditionVariableSleep

    Shawn Debnath <sdn@amazon.com> — 2019-07-12T06:08:13Z

    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)

  13. Re: Introduce timeout capability for ConditionVariableSleep

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-13T03:02:25Z

    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

  14. Re: Introduce timeout capability for ConditionVariableSleep

    Shawn Debnath <sdn@amazon.com> — 2019-07-13T04:56:10Z

    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)

  15. Re: Introduce timeout capability for ConditionVariableSleep

    Robert Haas <robertmhaas@gmail.com> — 2019-07-15T13:11:25Z

    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

  16. Re: Introduce timeout capability for ConditionVariableSleep

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-23T04:50:40Z

    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