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

On Tuesday, January 3, 2023 4:01 PM vignesh C <vignesh21(at)gmail(dot)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