RE: logical replication restrictions

On Tuesday, August 9, 2022 6:47 AM Euler Taveira <euler(at)eulerto(dot)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