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

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


Best Regards,
    Takamichi Osumi