Re: Logical replication keepalive flood

On Thu, Sep 30, 2021 at 3:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> I am not able to understand some parts of that patch.
>
> + If the sleep is shorter
> + * than KEEPALIVE_TIMEOUT milliseconds, we skip sending a keepalive to
> + * prevent it from getting too-frequent.
> + */
> + if (MyWalSnd->flush < sentPtr &&
> + MyWalSnd->write < sentPtr &&
> + !waiting_for_ping_response)
> + {
> + if (sleeptime > KEEPALIVE_TIMEOUT)
> + {
> + int r;
> +
> + r = WalSndWait(wakeEvents, KEEPALIVE_TIMEOUT,
> + WAIT_EVENT_WAL_SENDER_WAIT_WAL);
> +
> + if (r != 0)
> + continue;
> +
> + sleeptime -= KEEPALIVE_TIMEOUT;
> + }
> +
> + WalSndKeepalive(false);
>
> It claims to skip sending keepalive if the sleep time is shorter than
> KEEPALIVE_TIMEOUT (a new threshold) but the above code seems to
> suggest it sends in both cases. What am I missing?
>

The comment does seem to be wrong.
The way I see it, if the calculated sleeptime is greater than
KEEPALIVE_TIMEOUT, then it first sleeps for up to KEEPALIVE_TIMEOUT
milliseconds and skips sending a keepalive if something happens (i.e.
the socket becomes readable/writeable) during that time (WalSendWait
will return non-zero in that case). Otherwise, it sends a keepalive
and sleeps for (sleeptime - KEEPALIVE_TIMEOUT), then loops around
again ...

> Also, more to the point this special keep_alive seems to be sent for
> synchronous replication and walsender shutdown as they can expect
> updated locations. You haven't given any reason (theory) why those two
> won't be impacted due to this change? I am aware that for synchronous
> replication, we wake waiters while ProcessStandbyReplyMessage but I am
> not sure how it helps with wal sender shutdown? I think we need to
> know the reasons for this message and then need to see if the change
> has any impact on the same.
>

Yes, I'm not sure about the possible impacts, still looking at it.

Regards,
Greg Nancarrow
Fujitsu Australia