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

For v4-0002-some-fixups.patch :

+       if (client_connection_check_interval > 0)
+           enable_timeout_after(CLIENT_CONNECTION_CHECK_TIMEOUT,

+   /* Start timeout for checking if the client has gone away if necessary.
*/
+   if (client_connection_check_interval)

It would be better if the second if condition is aligned with that of the
first (> 0).

For v4-0001-Detect-dropped-connections-while-running-queries.patch :

+        Sets a time interval, in milliseconds, between periodic

I wonder if the interval should be expressed in seconds. Since the
description says: while running very long queries.

Cheers

On Fri, Mar 5, 2021 at 8:07 PM Thomas Munro <thomas.munro@gmail.com> wrote:

> On Mon, Mar 1, 2021 at 6:18 PM Thomas Munro <thomas.munro@gmail.com>
> wrote:
> > I've done a quick rebase of this the patch and added it to the
> > commitfest.  No other changes.  Several things were mentioned earlier
> > that still need to be tidied up.
>
> Rebased again due to bitrot.  This time I did some actual work:
>
> 1.  I didn't like the way it was rearming the timer *in the timer
> handler*; I think it should be done in the CFI(), and only if it
> determines that you're still running a query (otherwise you'll get
> periodic wakeups while you're idle between quieries, which is bad for
> the arctic ice cap; we already handle client going away efficiently
> between queries with WaitEventSet socket readiness).
> 2.  The timer handler surely has to set the latch to close a race (cf.
> other similar handlers; between the CFI() and the beginning of the
> sleep, you could handle the signal, set the flag, and then go to sleep
> for 100 years).
> 3.  The test might as well use pg_sleep() instead of doing a plpgsql
> busy loop of SELECT queries.
> 4.  I prefer the name CLIENT_CONNECTION_CHECK_TIMEOUT instead of
> SKIP_CLIENT_CHECK_TIMEOUT; let's make up only one new name for a
> concept instead of two.
> 5.  Miniscule doc change.
>
> I put these into a separate patch for ease of review.  I don't claim
> this is ready -- still needs more testing etc -- but it seems to be
> generating the right system calls at the right times now.
>