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

On Sat, Feb 4, 2023 at 5:04 PM Takamichi Osumi (Fujitsu)
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
...
>
> Kindly have a look at the attached v27.
>

Here are some review comments for patch v27-0001.

======
src/test/subscription/t/032_apply_delay.pl

1.
+# Confirm the time-delayed replication has been effective from the server log
+# message where the apply worker emits for applying delay. Moreover, verify
+# that the current worker's remaining wait time is sufficiently bigger than the
+# expected value, in order to check any update of the min_apply_delay.
+sub check_apply_delay_log

~

"has been effective from the server log" --> "worked, by inspecting
the server log"

~~~

2.
+my $delay = 3;

Might be better to name this variable as 'min_apply_delay'.

~~~

3.
+# Now wait for replay to complete on publisher. We're done waiting when the
+# subscriber has applyed up to the publisher LSN.
+$node_publisher->wait_for_catchup($appname);

3a.
Something seemed wrong with the comment.

Was it meant to say more like? "The publisher waits for the
replication to complete".

Typo: "applyed"

~

3b.
Instead of doing this wait_for_catchup stuff why don't you just use a
synchronous pub/sub and then the publication will just block
internally like you require but without you having to block using test
code?

~~~

4.
+# Run a query to make sure that the reload has taken effect.
+$node_publisher->safe_psql('postgres', q{SELECT 1});

SUGGESTION (for the comment)
# Running a dummy query causes the config to be reloaded.

~~~

5.
+# Confirm the record is not applied expectedly
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(a) FROM tab_int WHERE a = 0;");
+is($result, qq(0), "check the delayed transaction was not applied");

"expectedly" ??

SUGGESTION (for comment)
# Confirm the record was not applied

------
Kind Regards,
Peter Smith.
Fujitsu Australia