On Tuesday, August 9, 2022 6:47 AM Euler Taveira <euler@eulerto.com> wrote:
> I attached a v6.
Hi, thank you for posting the updated patch.
Minor review comments for v6.
(1) commit message
"If the subscriber sets min_apply_delay parameter, ..."
I suggest we use subscription rather than subscriber, because
this parameter refers to and is used for one subscription.
My suggestion is
"If one subscription sets min_apply_delay parameter, ..."
In case if you agree, there are other places to apply this change.
(2) commit message
It might be better to write a note for committer
like "Bump catalog version" at the bottom of the commit message.
(3) unit alignment between recovery_min_apply_delay and min_apply_delay
The former interprets input number as milliseconds in case of no units,
while the latter takes it as seconds without units.
I feel it would be better to make them aligned.
(4) catalogs.sgml
+ Delay the application of changes by a specified amount of time. The
+ unit is in milliseconds.
As a column explanation, it'd be better to use a noun
in the first sentence to make this description aligned with
other places. My suggestion is
"Application delay of changes by ....".
(5) pg_subscription.c
There is one missing blank line before writing if statement.
It's written in the AlterSubscription for other cases.
@@ -1100,6 +1130,12 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
replaces[Anum_pg_subscription_subdisableonerr - 1]
= true;
}
+ if (IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY))
(6) tab-complete.c
The order of tab-complete parameters listed in the COMPLETE_WITH
should follow alphabetical order. "min_apply_delay" can come before "origin".
We can refer to d547f7c commit.
(7) 032_apply_delay.pl
There are missing whitespaces after comma in the mod functions.
UPDATE test_tab SET b = md5(b) WHERE mod(a,2) = 0;
DELETE FROM test_tab WHERE mod(a,3) = 0;
Best Regards,
Takamichi Osumi
