Date: Sat, 16 Mar 2019 15:27:17 -0700
From: Shawn Debnath <sdn@amazon.com>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
CC: <thomas.munro@gmail.com>, <pgsql-hackers@postgresql.org>
Subject: Re: Introduce timeout capability for ConditionVariableSleep
Message-ID: <20190316222717.GA2139@f01898859afd.ant.amazon.com>
References: <20190313005342.GA8301@f01898859afd.ant.amazon.com>
 <20190313.172415.156658364.horiguchi.kyotaro@lab.ntt.co.jp>
 <20190315002611.GA1119@f01898859afd.ant.amazon.com>
 <20190315.141517.229371570.horiguchi.kyotaro@lab.ntt.co.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20190315.141517.229371570.horiguchi.kyotaro@lab.ntt.co.jp>
User-Agent: Mutt/1.11.3 (2019-02-01)
Precedence: Bulk
Precedence: bulk

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)