On Thu, Apr 27, 2023 at 4:07 PM Drouvot, Bertrand
<bertranddrouvot...@gmail.com> wrote:
>
> On 4/27/23 11:53 AM, Amit Kapila wrote:
> > On Thu, Apr 27, 2023 at 2:16 PM Drouvot, Bertrand
> > <bertranddrouvot...@gmail.com> wrote:
> >>
> >> On 4/27/23 10:11 AM, Yu Shi (Fujitsu) wrote:
> >>> Hi hackers,
> >>>
> >>> In 035_standby_logical_decoding.pl, I think that the check in the
> >>> following test
> >>> case should be performed on the standby node, instead of the primary
> >>> node, as
> >>> the slot is created on the standby node.
> >>
> >> Oh right, the current test is not done on the right node, thanks!
> >>
> >>> The test currently passes because it
> >>> only checks the return value of psql. It might be more appropriate to
> >>> check the
> >>> error message.
> >>
> >> Agree, and it's consistent with what is being done in
> >> 006_logical_decoding.pl.
> >>
> >>> Please see the attached patch.
> >>>
> >>
> >> +
> >> +($result, $stdout, $stderr) = $node_standby->psql(
> >> 'otherdb',
> >> "SELECT lsn FROM
> >> pg_logical_slot_peek_changes('behaves_ok_activeslot', NULL, NULL) ORDER BY
> >> lsn DESC LIMIT 1;"
> >> - ),
> >> - 3,
> >> - 'replaying logical slot from another database fails');
> >> + );
> >> +ok( $stderr =~
> >> + m/replication slot "behaves_ok_activeslot" was not created in
> >> this database/,
> >> + "replaying logical slot from another database fails");
> >>
> >>
> >> That does look good to me.
> >>
> >
> > I agree that that the check should be done on standby but how does it
> > make a difference to check the error message or return value? Won't it
> > be the same for both primary and standby?
> >
>
> Yes that would be the same. I think the original idea from Shi-san (please
> correct me If I'm wrong)
> was "while at it" let's also make this test on the right node even better.
>
Fair enough. Let''s do it that way then.
> >> Nit: I wonder if while at it (as it was already there) we could not remove
> >> the " ORDER BY lsn DESC LIMIT 1" part of it.
> >> It does not change anything regarding the test but looks more appropriate
> >> to me.
> >>
> >
> > It may not make a difference as this is anyway an error case but it
> > looks more logical to LIMIT by 1 as you are fetching a single LSN
> > value and it will be consistent with other tests in this file and the
> > test case in the file 006_logical_decoding.pl.
> >
>
> yeah I think it all depends how one see this test (sort of test a failure or
> try to get
> a result and see if it fails). That was a Nit so I don't have a strong
> opinion on this one.
>
I feel let's be consistent here and keep it as it is.
> > BTW, in the same test, I see it uses wait_for_catchup() in one place
> > and wait_for_replay_catchup() in another place after Insert. Isn't it
> > better to use wait_for_replay_catchup in both places?
> >
>
> They are both using the 'replay' mode so both are perfectly correct but for
> consistency (and as
> they don't use the same "target_lsn" ('write' vs 'flush')) I think that using
> wait_for_replay_catchup()
> in both places (which is using the 'flush' target lsn) is a good idea.
>
Yeah, let's use wait_for_replay_catchup() at both places.
--
With Regards,
Amit Kapila.
