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

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