RE: Logical replication timeout problem

On Wed, Mar 9, 2022 at 2:45 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
Thanks for your comments.

> On Wed, Mar 9, 2022 at 10:26 AM I wrote:
> > On Tue, Mar 8, 2022 at 3:52 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
> wrote:
> > > I've looked at the patch and have a question:
> > Thanks for your review and comments.
> >
> > > +void
> > > +SendKeepaliveIfNecessary(LogicalDecodingContext *ctx, bool skipped) {
> > > + static int skipped_changes_count = 0;
> > > +
> > > + /*
> > > + * skipped_changes_count is reset when processing changes that do
> not
> > > + * need to be skipped.
> > > + */
> > > + if (!skipped)
> > > + {
> > > + skipped_changes_count = 0;
> > > + return;
> > > + }
> > > +
> > > + /*
> > > + * After continuously skipping SKIPPED_CHANGES_THRESHOLD
> > > changes, try to send a
> > > + * keepalive message.
> > > + */
> > > + #define SKIPPED_CHANGES_THRESHOLD 10000
> > > +
> > > + if (++skipped_changes_count >= SKIPPED_CHANGES_THRESHOLD)
> > > + {
> > > + /* Try to send a keepalive message. */
> > > + OutputPluginUpdateProgress(ctx, true);
> > > +
> > > + /* After trying to send a keepalive message, reset the flag. */
> > > + skipped_changes_count = 0;
> > > + }
> > > +}
> > >
> > > Since we send a keepalive after continuously skipping 10000 changes, the
> > > originally reported issue can still occur if skipping 10000 changes took more
> than
> > > the timeout and the walsender didn't send any change while that, is that
> right?
> > Yes, theoretically so.
> > But after testing, I think this value should be conservative enough not to
> reproduce
> > this bug.
>
> But it really depends on the workload, the server condition, and the
> timeout value, right? The logical decoding might involve disk I/O much
> to spill/load intermediate data and the system might be under the
> high-load condition. Why don't we check both the count and the time?
> That is, I think we can send a keep-alive either if we skipped 10000
> changes or if we didn't sent anything for wal_sender_timeout / 2.
Yes, you are right.
Do you mean that when skipping every change, check if it has been more than
(wal_sender_timeout / 2) without sending anything?
IIUC, I tried to send keep-alive messages based on time before[1], but after
testing, I found that it will brings slight overhead. So I am not sure, in a
function(pgoutput_change) that is invoked frequently, should this kind of
overhead be introduced?

> Also, the patch changes the current behavior of wal senders; with the
> patch, we send keep-alive messages even when wal_sender_timeout = 0.
> But I'm not sure it's a good idea. The subscriber's
> wal_receiver_timeout might be lower than wal_sender_timeout. Instead,
> I think it's better to periodically check replies and send a reply to
> the keep-alive message sent from the subscriber if necessary, for
> example, every 10000 skipped changes.
Sorry, I could not follow what you said. I am not sure, do you mean the
following?
1. When we didn't sent anything for (wal_sender_timeout / 2) or we skipped
10000 changes continuously, we will invoke the function WalSndKeepalive in the
function WalSndUpdateProgress, and send a keepalive message to the subscriber
with requesting an immediate reply.
2. If after sending a keepalive message, and then 10000 changes are skipped
continuously again. In this case, we need to handle the reply from the
subscriber-side when processing the 10000th change. The handling approach is to
reply to the confirmation message from the subscriber.

[1] - https://www.postgresql.org/message-id/OS3PR01MB6275DFFDAC7A59FA148931529E209%40OS3PR01MB6275.jpnprd01.prod.outlook.com

Please let me know if I understand wrong.

Regards,
Wang wei