Remove unneeded PGDATABASE setting from TAP tests

Started by Bharath Rupireddyabout 2 years ago4 messages

bharath.rupireddyforpostgres@gmail.com

about 2 years ago

1 attachment(s)

Hi,

Some of the TAP tests are explicitly setting PGDATABASE environment
variable to 'postgres', which isn't needed because the TAP test's perl
library PostgreSQL::Test::Cluster already sets it once and for all.
I've attached a patch that removes all such unneeded PGDATABASE
settings.

Thoughts?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Attachments:

v1-0001-Remove-unneeded-PGDATABASE-setting-from-TAP-tests.patchapplication/x-patch; name=v1-0001-Remove-unneeded-PGDATABASE-setting-from-TAP-tests.patchDownload

From d428cc8a9ded13e22fea8a472b6a218ae77184cc Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 28 Dec 2023 11:48:37 +0000
Subject: [PATCH v1] Remove unneeded PGDATABASE setting from TAP tests

Some of the TAP tests are explicitly setting PGDATABASE
environment variable to 'postgres', which isn't needed because the
TAP test's perl library PostgreSQL::Test::Cluster already sets it
once and for all. This commit removes all such unneeded PGDATABASE
settings.
---
 src/test/recovery/t/004_timeline_switch.pl | 2 --
 src/test/recovery/t/019_replslot_limit.pl  | 2 --
 2 files changed, 4 deletions(-)

diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index edaef91845..5a3dac972f 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -8,8 +8,6 @@ use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
 
-$ENV{PGDATABASE} = 'postgres';
-
 # Ensure that a cascading standby is able to follow a newly-promoted standby
 # on a new timeline.
 
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index 6c244a5550..29c0390697 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -12,8 +12,6 @@ use PostgreSQL::Test::Cluster;
 use Test::More;
 use Time::HiRes qw(usleep);
 
-$ENV{PGDATABASE} = 'postgres';
-
 # Initialize primary node, setting wal-segsize to 1MB
 my $node_primary = PostgreSQL::Test::Cluster->new('primary');
 $node_primary->init(allows_streaming => 1, extra => ['--wal-segsize=1']);
-- 
2.34.1

Michael Paquier

michael@paquier.xyz

about 2 years ago

In reply to: Bharath Rupireddy (#1)

Re: Remove unneeded PGDATABASE setting from TAP tests

On Sun, Dec 31, 2023 at 07:24:08AM +0530, Bharath Rupireddy wrote:

Some of the TAP tests are explicitly setting PGDATABASE environment
variable to 'postgres', which isn't needed because the TAP test's perl
library PostgreSQL::Test::Cluster already sets it once and for all.
I've attached a patch that removes all such unneeded PGDATABASE
settings.

Thoughts?

Hmm. I don't see any reason to not do that as these are not really
self-documenting either. How about 011_clusterdb_all.pl for
PGDATABASE?
--
Michael

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

about 2 years ago

In reply to: Michael Paquier (#2)

1 attachment(s)

Re: Remove unneeded PGDATABASE setting from TAP tests

On Sun, Dec 31, 2023 at 8:36 AM Michael Paquier <michael@paquier.xyz> wrote:

On Sun, Dec 31, 2023 at 07:24:08AM +0530, Bharath Rupireddy wrote:

Some of the TAP tests are explicitly setting PGDATABASE environment
variable to 'postgres', which isn't needed because the TAP test's perl
library PostgreSQL::Test::Cluster already sets it once and for all.
I've attached a patch that removes all such unneeded PGDATABASE
settings.

Thoughts?

Hmm. I don't see any reason to not do that as these are not really
self-documenting either. How about 011_clusterdb_all.pl for
PGDATABASE?

Oh, yeah. We can remove that too, after all, PGDATABASE is being set
to postgres the default database which PostgreSQL::Test::Cluster does
set for us. I was earlier swayed by the comment atop the PGDATABASE
setting in 011_clusterdb_all.pl. Attached is v2 patch with this
change.

We perhaps can re-word and retain the comment in 011_clusterdb_all.pl,
which I think unnecessary as the code around 'alldb' if-else condition
in clusterdb.c can tell that.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Attachments:

v2-0001-Remove-unneeded-PGDATABASE-setting-from-TAP-tests.patchapplication/octet-stream; name=v2-0001-Remove-unneeded-PGDATABASE-setting-from-TAP-tests.patchDownload

From 8598da24b4ddee025c22dc682443199e516611a1 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 1 Jan 2024 07:42:01 +0000
Subject: [PATCH v2] Remove unneeded PGDATABASE setting from TAP tests

Some of the TAP tests are explicitly setting PGDATABASE
environment variable to 'postgres', which isn't needed because the
TAP test's perl library PostgreSQL::Test::Cluster already sets it
once and for all. This commit removes all such unneeded PGDATABASE
settings.
---
 src/bin/scripts/t/011_clusterdb_all.pl     | 4 ----
 src/test/recovery/t/004_timeline_switch.pl | 2 --
 src/test/recovery/t/019_replslot_limit.pl  | 2 --
 3 files changed, 8 deletions(-)

diff --git a/src/bin/scripts/t/011_clusterdb_all.pl b/src/bin/scripts/t/011_clusterdb_all.pl
index 26315d0a51..90e0b0aa31 100644
--- a/src/bin/scripts/t/011_clusterdb_all.pl
+++ b/src/bin/scripts/t/011_clusterdb_all.pl
@@ -12,10 +12,6 @@ my $node = PostgreSQL::Test::Cluster->new('main');
 $node->init;
 $node->start;
 
-# clusterdb -a is not compatible with -d, hence enforce environment variable
-# correctly.
-$ENV{PGDATABASE} = 'postgres';
-
 $node->issues_sql_like(
 	[ 'clusterdb', '-a' ],
 	qr/statement: CLUSTER.*statement: CLUSTER/s,
diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index 2500201b99..58d1f2ffc1 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -8,8 +8,6 @@ use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
 
-$ENV{PGDATABASE} = 'postgres';
-
 # Ensure that a cascading standby is able to follow a newly-promoted standby
 # on a new timeline.
 
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index 22ca8ecb5c..8a727c9bda 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -12,8 +12,6 @@ use PostgreSQL::Test::Cluster;
 use Test::More;
 use Time::HiRes qw(usleep);
 
-$ENV{PGDATABASE} = 'postgres';
-
 # Initialize primary node, setting wal-segsize to 1MB
 my $node_primary = PostgreSQL::Test::Cluster->new('primary');
 $node_primary->init(allows_streaming => 1, extra => ['--wal-segsize=1']);
-- 
2.34.1

Michael Paquier

michael@paquier.xyz

about 2 years ago

In reply to: Bharath Rupireddy (#3)

Re: Remove unneeded PGDATABASE setting from TAP tests

On Mon, Jan 01, 2024 at 01:52:10PM +0530, Bharath Rupireddy wrote:

Oh, yeah. We can remove that too, after all, PGDATABASE is being set
to postgres the default database which PostgreSQL::Test::Cluster does
set for us. I was earlier swayed by the comment atop the PGDATABASE
setting in 011_clusterdb_all.pl. Attached is v2 patch with this
change.

Thanks. Applied after keeping the comment in 011_clusterdb_all.pl,
rewording it a bit.
--
Michael