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