Re: Walsender timeouts and large transactions

On 2017-05-25 17:52 Petr Jelinek wrote:
> Hi,
>
> We have had issue with walsender timeout when used with logical
> decoding and the transaction is taking long time to be decoded
> (because it contains many changes)
>
> I was looking today at the walsender code and realized that it's
> because if the network and downstream are fast enough, we'll always
> take fast path in WalSndWriteData which does not do reply or
> keepalive processing and is only reached once the transaction has
> finished by other code. So paradoxically we die of timeout because
> everything was fast enough to never fall back to slow code path.
>
> I propose we only use fast path if the last processed reply is not
> older than half of walsender timeout, if it is then we'll force the
> slow code path to process the replies again. This is similar logic
> that we use to determine if to send keepalive message. I also added
> CHECK_INTERRUPRS call to fast code path because otherwise walsender
> might ignore them for too long on large transactions.
>
> Thoughts?
>

On 2017-08-10 14:20 Sokolov Yura wrote:
> On 2017-08-09 16:23, Petr Jelinek wrote:
> > 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).
> >
>
> If standby really stalled, then it will not read from socket, and then
> `pg_is_sendpending` eventually will return false, and timeout will be
> checked.
> If standby doesn't stall, then `last_reply_timestamp` will be updated
> in `ProcessRepliesIfAny`, and so timeout will not be triggered.
> Do I understand correctly, or I missed something?
>
> >> 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.
>
> In this case CHECK_FOR_INTERRUPTS will be called after
> wal_sender_timeout/2.
> (This diff is for first appearance of `pq_is_send_pending` in a
> function).
>
> If it should be called more often, then patch could be simplier:
>
> if (!pq_is_send_pending())
> - return;
> + {
> + CHECK_FOR_INTERRUPTS();
> + if (last_reply_timestamp <= 0 || wal_sender_timeout
> <= 0 ||
> + now <=
> TimestampTzPlusMilliseconds(last_reply_timestamp,
> wal_sender_timeout / 2))
> + return;
> + }
>
> (Still, I could be mistaken, it is just suggestion).
>
> Is it hard to add test for case this patch fixes?
>
> With regards,

Tom, Robert,

I believe this bug have to be fixed in Pg10, and I don't wonna
be that guy who prevents it from being fixed.
What should/could I do?
( https://commitfest.postgresql.org/14/1151/ )

--
With regards,
Sokolov Yura aka funny_falcon
Postgres Professional: https://postgrespro.ru
The Russian Postgres Company