RE: Time delayed LR (WAS Re: logical replication restrictions)

Hi,

On Tuesday, February 7, 2023 2:26 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Feb 7, 2023 at 10:13 AM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
> wrote:
> >
> > At Mon, 6 Feb 2023 13:10:01 +0000, "Takamichi Osumi (Fujitsu)"
> > <osumi(dot)takamichi(at)fujitsu(dot)com> wrote in
> > > The attached patch v29 has included your changes.
> >
> > catalogs.sgml
> >
> > + <para>
> > + The minimum delay (ms) for applying changes.
> > + </para></entry>
> >
> > I think we don't use unit symbols that way. Namely I think we would
> > write it as "The minimum delay for applying changes in milliseconds"
> >
>
> Okay, if we prefer to use milliseconds, then how about: "The minimum delay, in
> milliseconds, for applying changes"?
This looks good to me. Adopted.

>
> >
> > alter_subscription.sgml
> >
> > are <literal>slot_name</literal>,
> > <literal>synchronous_commit</literal>,
> > <literal>binary</literal>, <literal>streaming</literal>,
> > - <literal>disable_on_error</literal>, and
> > - <literal>origin</literal>.
> > + <literal>disable_on_error</literal>,
> > + <literal>origin</literal>, and
> > + <literal>min_apply_delay</literal>.
> > </para>
> >
> > By the way, is there any rule for the order among the words?
> >
>
> Currently, it is in the order in which the corresponding features are added.
Yes. So, I keep it as it is.

>
> > They
> > don't seem in alphabetical order nor in the same order to the
> > create_sbuscription page.
> >
>
> In create_subscription page also, it appears to be in the order in which those
> are added with a difference that they are divided into two categories
> (parameters that control what happens during subscription creation and
> parameters that control the subscription's replication behavior after it has been
> created)
Same as here. The current order should be fine.

>
> > (I seems like in the order of SUBOPT_* symbols, but I'm not sure it's
> > a good idea..)
> >
> >
> > subscriptioncmds.c
> >
> > + if (opts.streaming ==
> LOGICALREP_STREAM_PARALLEL &&
> > +
> > + !IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) &&
> > + sub->minapplydelay > 0)
> > ..
> > + if (opts.min_apply_delay > 0 &&
> > +
> > + !IsSet(opts.specified_opts, SUBOPT_STREAMING) && sub->stream ==
> > + LOGICALREP_STREAM_PARALLEL)
> >
> > Don't we wrap the lines?
> >
> >
> > worker.c
> >
> > + if (wal_receiver_status_interval > 0 &&
> > + diffms > wal_receiver_status_interval * 1000L)
> > + {
> > + WaitLatch(MyLatch,
> > + WL_LATCH_SET |
> WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> > + wal_receiver_status_interval *
> 1000L,
> > +
> WAIT_EVENT_RECOVERY_APPLY_DELAY);
> > + send_feedback(last_received, true, false, true);
> > + }
> > + else
> > + WaitLatch(MyLatch,
> > + WL_LATCH_SET |
> WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> > + diffms,
> > +
> > + WAIT_EVENT_RECOVERY_APPLY_DELAY);
> >
> > send_feedback always handles the case where
> > wal_receiver_status_interval == 0.
> >
>
> It only handles when force is false but here we are using that as true. So, not
> sure, if what you said would be an improvement.
Agreed. So, I keep it as it is.

>
> > thus we can simply wait for
> > min(wal_receiver_status_interval, diffms) then call send_feedback()
> > unconditionally.
> >
> >
> > -start_apply(XLogRecPtr origin_startpos)
> > +start_apply(void)
> >
> > -LogicalRepApplyLoop(XLogRecPtr last_received)
> > +LogicalRepApplyLoop(void)
> >
> > Does this patch requires this change?
> >
>
> I think this is because the scope of last_received has been changed so that it
> can be used to pass in send_feedback() during the delay.
Yes, that's our intention.

Kindly have a look at the latest patch v31 shared in [1].

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

Best Regards,
Takamichi Osumi