public inbox for [email protected]
help / color / mirror / Atom feedFrom: Yu Shi (Fujitsu) <[email protected]>
To: Amit Kapila <[email protected]>
To: Drouvot, Bertrand <[email protected]>
Cc: [email protected] <[email protected]>
Subject: RE: Fix a test case in 035_standby_logical_decoding.pl
Date: Fri, 28 Apr 2023 02:05:39 +0000
Message-ID: <OSZPR01MB6310C83F99B56DFB43F4186EFD6B9@OSZPR01MB6310.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAA4eK1LfeYqYJjfOMXN+5R+J+cHtw-eMkauju=oa-vp8a5QBUA@mail.gmail.com>
References: <OSZPR01MB6310B0A507A0F2A2D379F38CFD6A9@OSZPR01MB6310.jpnprd01.prod.outlook.com>
<[email protected]>
<CAA4eK1KCrw5nOVWwfj_vfA=nhPnEpjKGhvHmANXpAhizQV6-Sg@mail.gmail.com>
<[email protected]>
<CAA4eK1LfeYqYJjfOMXN+5R+J+cHtw-eMkauju=oa-vp8a5QBUA@mail.gmail.com>
On Thu, Apr 27, 2023 9:53 PM Amit Kapila <[email protected]> wrote:
>
> On Thu, Apr 27, 2023 at 4:07 PM Drouvot, Bertrand
> <[email protected]> wrote:
> >
> > On 4/27/23 11:53 AM, Amit Kapila wrote:
> > > On Thu, Apr 27, 2023 at 2:16 PM Drouvot, Bertrand
> > > <[email protected]> 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.
>
Thanks for your reply. Your suggestions make sense to me.
Please see the attached patch.
Regards,
Shi Yu
Attachments:
[application/octet-stream] v3-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patch (2.0K, ../OSZPR01MB6310C83F99B56DFB43F4186EFD6B9@OSZPR01MB6310.jpnprd01.prod.outlook.com/2-v3-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patch)
download | inline diff:
From a9d850131dc375b69f69f319d189ed4ba6430f9b Mon Sep 17 00:00:00 2001
From: Shi Yu <[email protected]>
Date: Thu, 27 Apr 2023 11:05:40 +0800
Subject: [PATCH v3] Fix a test case in 035_standby_logical_decoding.pl
For the "replaying logical slot from another database fails" check, perform it
on the standby node instead of the primary node.
---
.../recovery/t/035_standby_logical_decoding.pl | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl
index f6d6447412..66d264f230 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -361,7 +361,7 @@ $node_primary->safe_psql('testdb',
qq[INSERT INTO decoding_test(x,y) SELECT s, s::text FROM generate_series(5,50) s;]
);
-$node_primary->wait_for_catchup($node_standby);
+$node_primary->wait_for_replay_catchup($node_standby);
my $stdout_recv = $node_standby->pg_recvlogical_upto(
'testdb', 'behaves_ok_activeslot', $endpos, $default_timeout,
@@ -384,12 +384,17 @@ is($stdout_recv, '', 'pg_recvlogical acknowledged changes');
$node_primary->safe_psql('postgres', 'CREATE DATABASE otherdb');
-is( $node_primary->psql(
+# Wait for catchup to ensure that the new database is visible to other sessions
+# on the standby.
+$node_primary->wait_for_replay_catchup($node_standby);
+
+($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");
##################################################
# Test that we can subscribe on the standby with the publication
--
2.30.0.windows.2
view thread (8+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: RE: Fix a test case in 035_standby_logical_decoding.pl
In-Reply-To: <OSZPR01MB6310C83F99B56DFB43F4186EFD6B9@OSZPR01MB6310.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox