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

On Monday, February 6, 2023 12:03 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> 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.
Thanks for checking !

> ======
> 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"
Sounds good to me. Also,
this is an unique part for time-delayed logical replication.
So, we can update those as we want. Fixed.

> ~~~
>
> 2.
> +my $delay = 3;
>
> Might be better to name this variable as 'min_apply_delay'.
I named this variable by following the test of recovery_min_apply_delay
(src/test/recovery/005_replay_delay.pl). So, this is aligned
with the test and I'd like to keep it as it is.

> ~~~
>
> 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"
Your wording looks better than mine. Fixed.

> ~
>
> 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?
This is the style of 005_reply_delay.pl. Then, this is also aligned with it.
So, I'd like to keep the current way of times comparison as it is.

Even if we could omit wait_for_catchup(), there will be new codes
for synchronous replication and that would make the min_apply_delay tests
more different from the corresponding one. Note that if we use
the synchronous mode, we need to turn it off for the last
ALTER SUBSCRIPTION DISABLE test case whose min_apply_delay to 1 day 5 min
and execute one record insert after that. This will make the tests confusing.
> ~~~
>
> 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.
Fixed.

> ~~~
>
> 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
Fixed.

Best Regards,
Takamichi Osumi