Re: [HACKERS] Walsender timeouts and large transactions

On 02/08/17 19:35, Yura Sokolov wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       not tested
> Spec compliant:           not tested
> Documentation:            not tested
>
> There is no check for (last_reply_timestamp <= 0 || wal_sender_timeout <= 0) as in other places
> (in WalSndKeepaliveIfNecessary for example).
>
> I don't think moving update of 'now' down to end of loop body is correct:
> there are calls to ProcessConfigFile with SyncRepInitConfig, ProcessRepliesIfAny that can
> last non-negligible time. It could lead to over sleeping due to larger computed sleeptime.
> Though I could be mistaken.
>
> I'm not sure about moving `if (!pg_is_send_pending())` in a body loop after WalSndKeepaliveIfNecessary.
> Is it necessary? But it looks harmless at least.
>

We also need to do actual timeout handing so that the timeout is not
deferred to the end of the transaction (Which is why I moved `if
(!pg_is_send_pending())` under WalSndCheckTimeOut() and
WalSndKeepaliveIfNecessary() calls).

> Could patch be reduced to check after first `if (!pg_is_sendpending())` ? like:
>
>       if (!pq_is_send_pending())
> -             return;
> +     {
> +             if (last_reply_timestamp <= 0 || wal_sender_timeout <= 0)
> +             {
> +                     CHECK_FOR_INTERRUPTS();
> +                     return;
> +             }
> +             if (now <= TimestampTzPlusMilliseconds(last_reply_timestamp, wal_sender_timeout / 2))
> +                     return;
> +     }
>
> If not, what problem prevents?

We should do CHECK_FOR_INTERRUPTS() independently of pq_is_send_pending
so that it's possible to stop walsender while it's processing large
transaction.