On Sat, Feb 4, 2023 at 5:04 PM Takamichi Osumi (Fujitsu)
<osumi.takamichi@fujitsu.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