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

On Wednesday, October 5, 2022 6:42 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> Hi Euler, a long time ago you ask me a few questions about my previous review
> [1].
>
> Here are my replies, plus a few other review comments for patch v7-0001.
Hi, thank you for your comments.

> ======
>
> 1. doc/src/sgml/catalogs.sgml
>
> + <para>
> + Application delay of changes by a specified amount of time. The
> + unit is in milliseconds.
> + </para></entry>
>
> The wording sounds a bit strange still. How about below
>
> SUGGESTION
> The length of time (ms) to delay the application of changes.
Fixed.

> =======
>
> 2. Other documentation?
>
> Maybe should say something on the Logical Replication Subscription page
> about this? (31.2 Subscription)
Added.

> =======
>
> 3. doc/src/sgml/ref/create_subscription.sgml
>
> + synchronized, this may lead to apply changes earlier than expected.
> + This is not a major issue because a typical setting of this parameter
> + are much larger than typical time deviations between servers.
>
> Wording?
>
> SUGGESTION
> ... than expected, but this is not a major issue because this parameter is
> typically much larger than the time deviations between servers.
Fixed.

> ~~~
>
> 4. Q/A
>
> From [2] you asked:
>
> > Should there also be a big warning box about the impact if using
> > synchronous_commit (like the other streaming replication page has this
> > warning)?
> Impact? Could you elaborate?
>
> ~
>
> I noticed the streaming replication docs for recovery_min_apply_delay has a big
> red warning box saying that setting this GUC may block the synchronous
> commits. So I was saying won’t a similar big red warning be needed also for
> this min_apply_delay parameter if the delay is used in conjunction with a
> publisher wanting synchronous commit because it might block everything?
I agree with you. Fixed.

> ~~~
>
> 4. Example
>
> +<programlisting>
> +CREATE SUBSCRIPTION foo
> + CONNECTION 'host=192.168.1.50 port=5432 user=foo
> dbname=foodb'
> + PUBLICATION baz
> + WITH (copy_data = false, min_apply_delay = '4h');
> +</programlisting></para>
>
> If the example named the subscription/publication as ‘mysub’ and ‘mypub’ I
> think it would be more consistent with the existing examples.
Fixed.

> ======
>
> 5. src/backend/commands/subscriptioncmds.c - SubOpts
>
> @@ -89,6 +91,7 @@ typedef struct SubOpts
> bool disableonerr;
> char *origin;
> XLogRecPtr lsn;
> + int64 min_apply_delay;
> } SubOpts;
>
> I feel it would be better to be explicit about the storage units. So call this
> member ‘min_apply_delay_ms’. E.g. then other code in
> parse_subscription_options will be more natural when you are converting using
> and assigning them to this member.
I don't think we use such names including units explicitly.
Could you please tell me a similar example for this ?

> ~~~
>
> 6. - parse_subscription_options
>
> + /*
> + * If there is no unit, interval_in takes second as unit. This
> + * parameter expects millisecond as unit so add a unit (ms) if
> + * there isn't one.
> + */
>
> The comment feels awkward. How about below
>
> SUGGESTION
> If no unit was specified, then explicitly add 'ms' otherwise the interval_in
> function would assume 'seconds'
Fixed.

> ~~~
>
> 7. - parse_subscription_options
>
> (This is a repeat of [1] review comment #12)
>
> + if (opts->min_apply_delay < 0 && IsSet(supported_opts,
> SUBOPT_MIN_APPLY_DELAY))
> + ereport(ERROR,
> + errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> + errmsg("option \"%s\" must not be negative", "min_apply_delay"));
>
> Why is this code here instead of inside the previous code block where the
> min_apply_delay was assigned in the first place?
Changed.

> ======
>
> 8. src/backend/replication/logical/worker.c - apply_delay
>
> + * When min_apply_delay parameter is set on subscriber, we wait long
> + enough to
> + * make sure a transaction is applied at least that interval behind the
> + * publisher.
>
> "on subscriber" -> "on the subscription"
Fixed.

> ~~~
>
> 9.
>
> + * Apply delay only after all tablesync workers have reached READY
> + state. A
> + * tablesync worker are kept until it reaches READY state. If we allow
> + the
>
>
> Wording ??
>
> "A tablesync worker are kept until it reaches READY state." ??
I removed the sentence.

> ~~~
>
> 10.
>
> 10a.
> + /* nothing to do if no delay set */
>
> Uppercase comment
> /* Nothing to do if no delay set */
>
> ~
>
> 10b.
> + /* set apply delay */
>
> Uppercase comment
> /* Set apply delay */
Both are fixed.

> ~~~
>
> 11. - apply_handle_stream_prepare / apply_handle_stream_commit
>
> The previous concern about incompatibility with the "Parallel Apply"
> work (see [1] review comments #17, #18) is still a pending issue, isn't it?
Yes, I think so.
Kindly have a look at [1].

> ======
>
> 12. src/backend/utils/adt/timestamp.c interval_to_ms
>
> +/*
> + * Given an Interval returns the number of milliseconds.
> + */
> +int64
> +interval_to_ms(const Interval *interval)
>
> SUGGESTION
> Returns the number of milliseconds in the specified Interval.
Fixed.

> ~~~
>
> 13.
>
>
> + /* adds portion time (in ms) to the previous result. */
>
> Uppercase comment
> /* Adds portion time (in ms) to the previous result. *
Fixed.

> ======
>
> 14. src/bin/pg_dump/pg_dump.c - getSubscriptions
>
> + {
> + appendPQExpBufferStr(query, " s.suborigin,\n");
> + appendPQExpBufferStr(query, " s.subapplydelay\n"); }
>
> This could be done using just a single appendPQExpBufferStr if you want to
> have 1 call instead of 2.
Made them together.

> ======
>
> 15. src/bin/psql/describe.c - describeSubscriptions
>
> + /* origin and min_apply_delay are only supported in v16 and higher */
>
> Uppercase comment
> /* Origin and min_apply_delay are only supported in v16 and higher */
Fixed.

> ======
>
> 16. src/include/catalog/pg_subscription.h
>
> + int64 subapplydelay; /* Replication apply delay */
> +
>
> Consider renaming this as 'subapplydelayms' to make the units perfectly clear.
Similar to the 5th comments, I can't find any examples for this.
I'd like to keep it general, which makes me feel it is more aligned with
existing codes.

> ======
>
> 17. src/test/regress/sql/subscription.sql
>
> Is [1] review comment 21 (There are some test cases for CREATE
> SUBSCRIPTION but there are no test cases for ALTER SUBSCRIPTION
> changing this new parameter.) still a pending item?
Added one test case for alter subscription.

Also, I removed the function of logicalrep_worker_wakeup()
that was trigged by AlterSubscription only when disabling the subscription.
This is achieved and replaced by another patch proposed in [2] in a general manner.

There are still some pending comments for this patch,
but I'll share the current patch once.

Lastly, thank you so much, Kuroda-san for giving me many advice and
suggestion for some modification of this patch.

[1] - https://www.postgresql.org/message-id/CAA4eK1JJFpgqE0ehAb7C9YFkJ-Xe-W1ZUPZptEfYjNJM4G-sLA%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/20221122004119.GA132961%40nathanxps13

Best Regards,
Takamichi Osumi