Re: Introduce timeout capability for ConditionVariableSleep

On Fri, Mar 22, 2019 at 7:21 AM Shawn Debnath <sdn(at)amazon(dot)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