Dear Amit,
> I was trying to think if there is any better way to implement the
> newly added callback (WalSndDelay()) but couldn't find any. For
> example, one idea I tried to evaluate is whether we can merge it with
> the existing callback WalSndUpdateProgress() or maybe extract the part
> other than progress tracking from that function into a new callback
> and then try to reuse it here as well. Though there is some common
> functionality like checking for timeout and processing replies still
> they are different enough that they seem to need separate callbacks.
> The prime purpose of a callback for the patch being discussed here is
> to delay the xact before sending the commit/prepare whereas the
> existing callback (WalSndUpdateProgress()) or what we are discussing
> at [1] allows sending the keepalive message in some special cases
> where there is no communication between walsender and walreceiver.
> Now, the WalSndDelay() also tries to check for timeout and send
> keepalive if necessary but there is also dependency on the delay
> parameter, so don't think it is a good idea of trying to combine those
> functionalities into one API.
>
> Thoughts?
>
> [1] -
> https://www.postgresql.org/message-id/20230210210423.r26ndnfmuifie4f6%40
> awork3.anarazel.de
Thank you for confirming. My understanding was that we should keep the current design.
I agree with your posting.
In the current callback and modified version in [1], sending keepalives is done
via ProcessPendingWrites(). It is called by many functions and should not be changed,
like adding end_time only for us. Moreover, the name is not suitable because
time-delayed logical replication does not wait until the send buffer becomes empty.
If we reconstruct WalSndUpdateProgress() and change mechanisms around that,
codes will become dirty. As Amit said, in one path, the lag will be tracked and
the walsender will wait until the buffer is empty.
In another path, the lag calculation will be ignored, and the walsender will wait
until the process spends time till a given period. Such a function is painful to read later.
I think callbacks that have different purposes should not be mixed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED