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

On Wed, Jan 11, 2023 at 3:27 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Jan 10, 2023 at 7:42 PM Takamichi Osumi (Fujitsu)
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > On Tuesday, January 3, 2023 4:01 PM vignesh C <vignesh21@gmail.com> wrote:
> > Hi, thanks for your review !
> >
> >
> > > 1) This global variable can be removed as it is used only in send_feedback which
> > > is called from maybe_delay_apply so we could pass it as a function argument:
> > > + * delay, avoid having positions of the flushed and apply LSN
> > > +overwritten by
> > > + * the latest LSN.
> > > + */
> > > +static bool in_delaying_apply = false;
> > > +static XLogRecPtr last_received = InvalidXLogRecPtr;
> > > +
> > I have removed the first variable and make it one of the arguments for send_feedback().
> >
> > > 2) -1 gets converted to -1000
> > >
> > > +int64
> > > +interval2ms(const Interval *interval)
> > > +{
> > > +       int64           days;
> > > +       int64           ms;
> > > +       int64           result;
> > > +
> > > +       days = interval->month * INT64CONST(30);
> > > +       days += interval->day;
> > > +
> > > +       /* Detect whether the value of interval can cause an overflow. */
> > > +       if (pg_mul_s64_overflow(days, MSECS_PER_DAY, &result))
> > > +               ereport(ERROR,
> > > +
> > > (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> > > +                                errmsg("bigint out of range")));
> > > +
> > > +       /* Adds portion time (in ms) to the previous result. */
> > > +       ms = interval->time / INT64CONST(1000);
> > > +       if (pg_add_s64_overflow(result, ms, &result))
> > > +               ereport(ERROR,
> > > +
> > > (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> > > +                                errmsg("bigint out of range")));
> > >
> > > create subscription sub7 connection 'dbname=regression host=localhost
> > > port=5432' publication pub1 with (min_apply_delay = '-1');
> > > ERROR:  -1000 ms is outside the valid range for parameter "min_apply_delay"
> > Good catch! Fixed in order to make input '-1' interpretted as -1 ms.
> >
> > > 3) This can be slightly reworded:
> > > +      <para>
> > > +       The length of time (ms) to delay the application of changes.
> > > +      </para></entry>
> > > to:
> > > Delay applying the changes by a specified amount of time(ms).
> > This has been suggested in [1] by Peter Smith. So, I'd like to keep the current patch's description.
> > Then, I didn't change this.
> >
> > > 4)  maybe_delay_apply can be moved from apply_handle_stream_prepare to
> > > apply_spooled_messages so that it is consistent with
> > > maybe_start_skipping_changes:
> > > @@ -1120,6 +1240,19 @@ apply_handle_stream_prepare(StringInfo s)
> > >
> > >         elog(DEBUG1, "received prepare for streamed transaction %u",
> > > prepare_data.xid);
> > >
> > > +       /*
> > > +        * Should we delay the current prepared transaction?
> > > +        *
> > > +        * Although the delay is applied in BEGIN PREPARE messages,
> > > streamed
> > > +        * prepared transactions apply the delay in a STREAM PREPARE
> > > message.
> > > +        * That's ok because no changes have been applied yet
> > > +        * (apply_spooled_messages() will do it). The STREAM START message
> > > does
> > > +        * not contain a prepare time (it will be available when the in-progress
> > > +        * prepared transaction finishes), hence, it was not possible to apply a
> > > +        * delay at that time.
> > > +        */
> > > +       maybe_delay_apply(prepare_data.prepare_time);
> > >
> > >
> > > That way the call from apply_handle_stream_commit can also be removed.
> > Sounds good. I moved the call of maybe_delay_apply() to the apply_spooled_messages().
> > Now it's aligned with maybe_start_skipping_changes().
> >
> > > 5) typo transfering should be transferring
> > > +          publisher and the current time on the subscriber. Time
> > > spent in logical
> > > +          decoding and in transfering the transaction may reduce the
> > > actual wait
> > > +          time. If the system clocks on publisher and subscriber are
> > > + not
> > Fixed.
> >
> > > 6) feedbacks can be changed to feedback messages
> > > + * it's necessary to keep sending feedbacks during the delay from the
> > > + worker
> > > + * process. Meanwhile, the feature delays the apply before starting the
> > Fixed.
> >
> > > 7)
> > > + /*
> > > + * Suppress overwrites of flushed and writtten positions by the lastest
> > > + * LSN in send_feedback().
> > > + */
> > >
> > > 7a) typo writtten should be written
> > >
> > > 7b) lastest should latest
> > I have removed this sentence. So, those typos are removed.
> >
> > Please have a look at the updated patch.
> >
> > [1] - https://www.postgresql.org/message-id/CAHut%2BPttQdFMNM2c6WqKt2c9G6r3ZKYRGHm04RR-4p4fyA4WRg%40mail.gmail.com
> >
> >
>
> Hi,
>
> 1.
> + errmsg("min_apply_delay must not be set when streaming = parallel")));
> we give the same error msg for  both the cases:
> a. when subscription is created with streaming=parallel  but we are
> trying to alter subscription to set min_apply_delay >0
> b. when subscription is created with some min_apply_delay and we are
> trying to alter subscription to make it streaming=parallel.
> For case a, error msg looks fine but for case b, I think error msg
> should be changed slightly.
> ALTER SUBSCRIPTION regress_testsub SET (streaming = parallel);
> ERROR:  min_apply_delay must not be set when streaming = parallel
> This gives the feeling that we are trying to modify min_apply_delay
> but we are not. Maybe we can change it to:
> "subscription with min_apply_delay must not be allowed to stream
> parallel" (or something better)
>
> thanks
> Shveta

Sorry for multiple emails. One suggestion:

2.
I think users can set ' wal_receiver_status_interval ' to 0 or more
than 'wal_sender_timeout'. But is this a frequent use-case scenario or
do we see DBAs setting these in such a way by mistake? If so, then I
think, it is better to give Warning message in such a case when a user
tries to create or alter a subscription with a large 'min_apply_delay'
(>=  'wal_sender_timeout') , rather than leaving it to the user's
understanding that WalSender may repeatedly timeout in such a case.
Parse_subscription_options and AlterSubscription can be modified to
log a warning. Any thoughts?

thanks
Shveta