On Thu, Apr 27, 2023 at 2:16 PM Drouvot, Bertrand
<bertranddrouvot.pg@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?
> 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.
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?
--
With Regards,
Amit Kapila.
