src/bin/pg_upgrade/t/004_subscription.pl test comment fix
Hi,
PSA a small fix for a misleading comment found in the pg_upgrade test code.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Fix-test-comment.patchapplication/octet-stream; name=v1-0001-Fix-test-comment.patchDownload
From 2942d560d25c74164a26a9b356e98f7a98623c5e Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Wed, 31 Jan 2024 15:47:57 +1100
Subject: [PATCH v1] Fix test comment
---
src/bin/pg_upgrade/t/004_subscription.pl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_upgrade/t/004_subscription.pl b/src/bin/pg_upgrade/t/004_subscription.pl
index 792f221..a7655b0 100644
--- a/src/bin/pg_upgrade/t/004_subscription.pl
+++ b/src/bin/pg_upgrade/t/004_subscription.pl
@@ -138,8 +138,8 @@ $publisher->safe_psql(
$new_sub->start;
# The subscription's running status and failover option should be preserved.
-# Old subscription regress_sub1 should have enabled and failover as true while
-# old subscription regress_sub2 should have enabled and failover as false.
+# # Upgraded regress_sub1 should still have enabled and failover as true.
+# # Upgraded regress_sub2 should still have enabled and failover as false.
$result =
$new_sub->safe_psql('postgres',
"SELECT subname, subenabled, subfailover FROM pg_subscription ORDER BY subname");
--
1.8.3.1
On Wed, 31 Jan 2024 at 10:27, Peter Smith <smithpb2250@gmail.com> wrote:
Hi,
PSA a small fix for a misleading comment found in the pg_upgrade test code.
Is the double # intentional here, as I did not see this style of
commenting used elsewhere:
+# # Upgraded regress_sub1 should still have enabled and failover as true.
+# # Upgraded regress_sub2 should still have enabled and failover as false.Regards,
Vignesh
On Wed, Jan 31, 2024 at 4:51 PM vignesh C <vignesh21@gmail.com> wrote:
On Wed, 31 Jan 2024 at 10:27, Peter Smith <smithpb2250@gmail.com> wrote:
Hi,
PSA a small fix for a misleading comment found in the pg_upgrade test code.
Is the double # intentional here, as I did not see this style of commenting used elsewhere: +# # Upgraded regress_sub1 should still have enabled and failover as true. +# # Upgraded regress_sub2 should still have enabled and failover as false.
Unintentional caused by copy/paste. Thanks for reporting. I will post
a fixed patch tomorrow.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
How about rewording it more extensively? It doesn't read great IMO.
I would use something like
# In the upgraded instance, the running status and failover option of the
# subscription with the failover option should have been preserved; the other
# should not.
# So regress_sub1 should still have subenabled,subfailover set to true,
# while regress_sub2 should have both set to false.
I think the symmetry between the two lines confuses more than helps.
It's not a huge thing but since we're editing anyway, why not?
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"The saddest aspect of life right now is that science gathers knowledge faster
than society gathers wisdom." (Isaac Asimov)
On Wed, Jan 31, 2024 at 7:48 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
How about rewording it more extensively? It doesn't read great IMO.
I would use something like# In the upgraded instance, the running status and failover option of the
# subscription with the failover option should have been preserved; the other
# should not.
# So regress_sub1 should still have subenabled,subfailover set to true,
# while regress_sub2 should have both set to false.
IIUC this suggested comment is implying that the running status is
*only* preserved when the failover option is true. But AFAIK that is
not correct. e.g. I hacked the test to keep subscription regress_sub2
as ENABLED but the result after the upgrade was subenabled/subfailover
= t/f, not f/f.
I think the symmetry between the two lines confuses more than helps.
It's not a huge thing but since we're editing anyway, why not?
OK. Now using your suggested 2nd sentence:
+# The subscription's running status and failover option should be preserved
+# in the upgraded instance. So regress_sub1 should still have
subenabled,subfailover
+# set to true, while regress_sub2 should have both set to false.~
I also tweaked some other nearby comments/messages which did not
mention the 'failover' preservation.
PSA v2.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v2-0001-Fix-pg_upgrade-test-comment.patchapplication/octet-stream; name=v2-0001-Fix-pg_upgrade-test-comment.patchDownload
From da15176d0d22a8b76f22722587941a9b76a48b8d Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 1 Feb 2024 11:11:37 +1100
Subject: [PATCH v2] Fix pg_upgrade test comment
---
src/bin/pg_upgrade/t/004_subscription.pl | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_upgrade/t/004_subscription.pl b/src/bin/pg_upgrade/t/004_subscription.pl
index 792f221..2d54313 100644
--- a/src/bin/pg_upgrade/t/004_subscription.pl
+++ b/src/bin/pg_upgrade/t/004_subscription.pl
@@ -109,7 +109,7 @@ $old_sub->stop;
# Check that pg_upgrade is successful when all tables are in ready or in
# init state (tab_upgraded1 table is in ready state and tab_upgraded2 table is
# in init state) along with retaining the replication origin's remote lsn
-# and subscription's running status.
+# and subscription's running status and failover option.
# ------------------------------------------------------
command_ok(
[
@@ -137,15 +137,15 @@ $publisher->safe_psql(
$new_sub->start;
-# The subscription's running status and failover option should be preserved.
-# Old subscription regress_sub1 should have enabled and failover as true while
-# old subscription regress_sub2 should have enabled and failover as false.
+# The subscription's running status and failover option should be preserved
+# in the upgraded instance. So regress_sub1 should still have subenabled,subfailover
+# set to true, while regress_sub2 should have both set to false.
$result =
$new_sub->safe_psql('postgres',
"SELECT subname, subenabled, subfailover FROM pg_subscription ORDER BY subname");
is( $result, qq(regress_sub1|t|t
regress_sub2|f|f),
- "check that the subscription's running status are preserved");
+ "check that the subscription's running status and failover are preserved");
my $sub_oid = $new_sub->safe_psql('postgres',
"SELECT oid FROM pg_subscription WHERE subname = 'regress_sub2'");
--
1.8.3.1
On Thu, Feb 1, 2024 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote:
OK. Now using your suggested 2nd sentence:
+# The subscription's running status and failover option should be preserved +# in the upgraded instance. So regress_sub1 should still have subenabled,subfailover +# set to true, while regress_sub2 should have both set to false.~
I also tweaked some other nearby comments/messages which did not
mention the 'failover' preservation.
Looks mostly good to me. One minor nitpick:
*
along with retaining the replication origin's remote lsn
-# and subscription's running status.
+# and subscription's running status and failover option.I don't think we need to use 'and' twice in the above sentence. We
should use ',' between different properties. I can change this on
Monday and push it unless you think otherwise.
--
With Regards,
Amit Kapila.
On Sat, Feb 3, 2024 at 5:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Feb 1, 2024 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote:
OK. Now using your suggested 2nd sentence:
+# The subscription's running status and failover option should be preserved +# in the upgraded instance. So regress_sub1 should still have subenabled,subfailover +# set to true, while regress_sub2 should have both set to false.~
I also tweaked some other nearby comments/messages which did not
mention the 'failover' preservation.Looks mostly good to me. One minor nitpick: * along with retaining the replication origin's remote lsn -# and subscription's running status. +# and subscription's running status and failover option.I don't think we need to use 'and' twice in the above sentence. We
should use ',' between different properties. I can change this on
Monday and push it unless you think otherwise.
+1
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Feb 5, 2024 at 2:42 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Sat, Feb 3, 2024 at 5:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Feb 1, 2024 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote:
OK. Now using your suggested 2nd sentence:
+# The subscription's running status and failover option should be preserved +# in the upgraded instance. So regress_sub1 should still have subenabled,subfailover +# set to true, while regress_sub2 should have both set to false.~
I also tweaked some other nearby comments/messages which did not
mention the 'failover' preservation.Looks mostly good to me. One minor nitpick: * along with retaining the replication origin's remote lsn -# and subscription's running status. +# and subscription's running status and failover option.I don't think we need to use 'and' twice in the above sentence. We
should use ',' between different properties. I can change this on
Monday and push it unless you think otherwise.+1
Pushed!
--
With Regards,
Amit Kapila.