Fix a test case in 035_standby_logical_decoding.pl
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. The test currently passes because it
only checks the return value of psql. It might be more appropriate to check the
error message. Please see the attached patch.
```
$node_primary->safe_psql('postgres', 'CREATE DATABASE otherdb');
is( $node_primary->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');
```
The regress log:
psql:<stdin>:1: ERROR: replication slot "behaves_ok_activeslot" does not exist
[11:23:21.859](0.086s) ok 8 - replaying logical slot from another database fails
Regards,
Shi Yu
Attachments:
v1-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patchapplication/octet-stream; name=v1-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patchDownload
From c27953c59fdfb0a4a4933843d5ec233a84b0ff19 Mon Sep 17 00:00:00 2001
From: Shi Yu <shiy.fnst@fujitsu.com>
Date: Thu, 27 Apr 2023 11:05:40 +0800
Subject: [PATCH v1] 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.
---
src/test/recovery/t/035_standby_logical_decoding.pl | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl
index b8f5311fe9..f559cde914 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -358,12 +358,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");
##################################################
# Recovery conflict: Invalidate conflicting slots, including in-use slots
--
2.31.1
Hi,
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.
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.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Thu, Apr 27, 2023 4:47 PM Drouvot, Bertrand <bertranddrouvot.pg@gmail.com> wrote:
Hi,
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.
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.
Thanks for your reply. I agree with you and I removed it in the attached patch.
Regards,
Shi Yu
Attachments:
v2-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patchapplication/octet-stream; name=v2-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patchDownload
From 40fd8053f73d29a13e2ddc5b77b15dcb686a6915 Mon Sep 17 00:00:00 2001
From: Shi Yu <shiy.fnst@fujitsu.com>
Date: Thu, 27 Apr 2023 11:05:40 +0800
Subject: [PATCH v2] 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..930e755712 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -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');
+ "SELECT lsn FROM pg_logical_slot_peek_changes('behaves_ok_activeslot', NULL, NULL);"
+ );
+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
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.
Hi,
On 4/27/23 11:53 AM, Amit Kapila wrote:
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?
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.
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.
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.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Thu, Apr 27, 2023 at 4:07 PM Drouvot, Bertrand
<bertranddrouvot.pg@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.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?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.
On Thu, Apr 27, 2023 9:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Apr 27, 2023 at 4:07 PM Drouvot, Bertrand
<bertranddrouvot.pg@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.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 checkthe
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 FROMpg_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 thisdatabase/,
+ "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:
v3-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patchapplication/octet-stream; name=v3-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patchDownload
From a9d850131dc375b69f69f319d189ed4ba6430f9b Mon Sep 17 00:00:00 2001
From: Shi Yu <shiy.fnst@fujitsu.com>
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