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? > 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.