Re: Introduce timeout capability for ConditionVariableSleep

From: Shawn Debnath <sdn(at)amazon(dot)com>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Introduce timeout capability for ConditionVariableSleep
Date: 2019-03-13 00:53:43
Message-ID: 20190313005342.GA8301@f01898859afd.ant.amazon.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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)

Attachment Content-Type Size
0001-Introduce-timeout-capability-for-ConditionVariableSleep-v2.patch text/plain 4.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Shawn Debnath 2019-03-13 01:00:07 Re: Refactoring the checkpointer's fsync request queue
Previous Message Tatsuo Ishii 2019-03-13 00:45:27 Re: Early WIP/PoC for inlining CTEs