Re: Add client connection check during the execution of the query

On Thu, Jul 18, 2019 at 5:04 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> writes:
> >> Yeah, the timer logic is wrong. I didn't have time to look into it
> >> but with truss/strace for some reason I see 3 setitimer() syscalls for
> >> every query, but I think this doesn't even need to set the timer for
> >> every query.
>
> > Hum. I see 2 settimer(), instead of 3.
>
> src/backend/utils/misc/timeout.c is not really designed for there
> to be timeouts that persist across multiple queries. It can probably
> be made better, but this patch doesn't appear to have touched any of
> that logic.
>
> To point to just one obvious problem, the error recovery path
> (sigsetjmp block) in postgres.c does
>
> disable_all_timeouts(false);
>
> which cancels *all* timeouts. Probably don't want that behavior
> anymore.
>
> I think the issue you're complaining about may have to do with
> the fact that if there's no statement timeout active, both
> enable_statement_timeout and disable_statement_timeout will
> call "disable_timeout(STATEMENT_TIMEOUT, false);". That does
> nothing, as desired, if there are no other active timeouts ...
> but if there is one, ie the client_connection timeout, we'll
> end up calling schedule_alarm which will call setitimer even
> if the desired time-of-nearest-timeout hasn't changed.
> That was OK behavior for the set of timeouts that the code
> was designed to handle, but we're going to need to be smarter
> now.

Ok, I think we like this feature proposal and I think we have a pretty
good handle on what needs to be done next, but without a patch author
that's not going to happen. I've therefore marked it "Returned with
feedback". If Sergey or someone else is interested in picking this up
and doing the work in time for CF2, please feel free to change that to
'moved to next'.

I wonder if it'd be possible, along the way, to make it so that
statement_timeout doesn't require calling itimer() for every
statement, instead letting the existing timer run if there is a
reasonable amount left to run on it, and then resetting it if needed.
I haven't looked into whether that's hard/impossible due to some race
condition I haven't spent the time to think about, but if Ishii-san
sees 5% slowdown from a couple of itimer calls(), I guess using
statement_timeout might currently cause a 2.5% slowdown.

--
Thomas Munro
https://enterprisedb.com