Small TAP improvements
Here's a couple of small patches I came up with while doing some related
work on TAP tests.
The first makes the argument for $node->config_data() optional. If it's
not supplied, pg_config is called without an argument and the whole
result is returned. Currently, if you try that you get back a nasty and
cryptic error.
The second changes the new GUCs TAP test to check against the installed
postgresql.conf.sample rather than the one in the original source
location. There are probably arguments both ways, but if we ever decided
to postprocess the file before installation, this would do the right thing.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
0001-Allow-option-argument-for-config_data-method-to-be-m.patchtext/x-patch; charset=UTF-8; name=0001-Allow-option-argument-for-config_data-method-to-be-m.patchDownload
From 4a828045aa3f4d85c012deda9bd953025af1291d Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <andrew@dunslane.net>
Date: Mon, 13 Jun 2022 18:16:33 -0400
Subject: [PATCH 1/2] Allow option argument for config_data method to be
missing
If the argument is missing the current code gets a nasty and cryptic
error message from IPC::Run. Instead we allow the argument to be missing
and in this case all the config lines are returned to the caller.
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index c8c7bc5045..d6c485545b 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -348,18 +348,18 @@ sub pg_version
=item $node->config_data($option)
Return a string holding configuration data from pg_config, with $option
-being the option switch used with the pg_config command.
+(if supplied) being the option switch used with the pg_config command.
=cut
sub config_data
{
- my ($self, $option) = @_;
+ my $self = shift;
local %ENV = $self->_get_env();
my ($stdout, $stderr);
my $result =
- IPC::Run::run [ $self->installed_command('pg_config'), $option ],
+ IPC::Run::run [ $self->installed_command('pg_config'), @_ ],
'>', \$stdout, '2>', \$stderr
or die "could not execute pg_config";
chomp($stdout);
--
2.25.1
0002-Use-installed-postgresql.conf.sample-for-GUC-sanity-.patchtext/x-patch; charset=UTF-8; name=0002-Use-installed-postgresql.conf.sample-for-GUC-sanity-.patchDownload
From 4d145a96483c9cdda07b2c60da76c03a0524bc09 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <andrew@dunslane.net>
Date: Mon, 13 Jun 2022 18:19:39 -0400
Subject: [PATCH 2/2] Use installed postgresql.conf.sample for GUC sanity TAP
test
The current code looks for the sample file in the source directory, but
it seems better to test against the installed sample file.
---
src/test/modules/test_misc/t/003_check_guc.pl | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/test/modules/test_misc/t/003_check_guc.pl b/src/test/modules/test_misc/t/003_check_guc.pl
index 60459ef759..3ed905e7a0 100644
--- a/src/test/modules/test_misc/t/003_check_guc.pl
+++ b/src/test/modules/test_misc/t/003_check_guc.pl
@@ -33,10 +33,11 @@ my $not_in_sample = $node->safe_psql(
ORDER BY 1");
my @not_in_sample_array = split("\n", lc($not_in_sample));
-# TAP tests are executed in the directory of the test, in the source tree,
-# even for VPATH builds, so rely on that to find postgresql.conf.sample.
-my $rootdir = "../../../..";
-my $sample_file = "$rootdir/src/backend/utils/misc/postgresql.conf.sample";
+# use the sample file from the temp install
+my $share_dir = $node->config_data('--sharedir');
+chomp $share_dir;
+$share_dir =~ s/^SHAREDIR = //;
+my $sample_file = "$share_dir/postgresql.conf.sample";
# List of all the GUCs found in the sample file.
my @gucs_in_file;
--
2.25.1
Andrew Dunstan <andrew@dunslane.net> writes:
The first makes the argument for $node->config_data() optional. If it's
not supplied, pg_config is called without an argument and the whole
result is returned. Currently, if you try that you get back a nasty and
cryptic error.
No opinion about whether that's useful.
The second changes the new GUCs TAP test to check against the installed
postgresql.conf.sample rather than the one in the original source
location. There are probably arguments both ways, but if we ever decided
to postprocess the file before installation, this would do the right thing.
Seems like a good idea, especially since it also makes the test code
shorter and more robust(-looking).
Looking at the patch itself,
+my $share_dir = $node->config_data('--sharedir');
+chomp $share_dir;
+$share_dir =~ s/^SHAREDIR = //;
+my $sample_file = "$share_dir/postgresql.conf.sample";I kind of wonder why config_data() isn't doing the chomp itself;
what caller would not want that? Pulling off the variable name
might be helpful too, since it's hard to conceive of a use-case
where you don't also need that.
regards, tom lane
The comment atop config_data still mentions $option, but after the patch that's no longer a name used in the function. (I have to admit that using @_ in the body of the function was a little bit confusing to me at first. Did you do that in order to allow multiple options to be passed?)
Also: if you give an option to pg_config, the output is not prefixed with the variable name. So you don't need to strip the "SHAREDIR =" bit: there isn't any. This is true even if you give multiple options:
schmee: master 0$ pg_config --sharedir --includedir
/home/alvherre/Code/pgsql-install/REL9_6_STABLE/share
/home/alvherre/Code/pgsql-install/REL9_6_STABLE/include
On 2022-06-14 Tu 12:20, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
The first makes the argument for $node->config_data() optional. If it's
not supplied, pg_config is called without an argument and the whole
result is returned. Currently, if you try that you get back a nasty and
cryptic error.No opinion about whether that's useful.
The second changes the new GUCs TAP test to check against the installed
postgresql.conf.sample rather than the one in the original source
location. There are probably arguments both ways, but if we ever decided
to postprocess the file before installation, this would do the right thing.Seems like a good idea, especially since it also makes the test code
shorter and more robust(-looking).Looking at the patch itself,
+my $share_dir = $node->config_data('--sharedir'); +chomp $share_dir; +$share_dir =~ s/^SHAREDIR = //; +my $sample_file = "$share_dir/postgresql.conf.sample";I kind of wonder why config_data() isn't doing the chomp itself;
what caller would not want that? Pulling off the variable name
might be helpful too, since it's hard to conceive of a use-case
where you don't also need that.
It already chomps the output, and pg_config doesn't output "SETTING = "
if given an option argument, so we could just remove those two lines -
they are remnants of an earlier version. I'll do it that way.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 2022-06-14 Tu 12:44, Álvaro Herrera wrote:
The comment atop config_data still mentions $option, but after the patch that's no longer a name used in the function. (I have to admit that using @_ in the body of the function was a little bit confusing to me at first. Did you do that in order to allow multiple options to be passed?)
Also: if you give an option to pg_config, the output is not prefixed with the variable name. So you don't need to strip the "SHAREDIR =" bit: there isn't any. This is true even if you give multiple options:
schmee: master 0$ pg_config --sharedir --includedir
/home/alvherre/Code/pgsql-install/REL9_6_STABLE/share
/home/alvherre/Code/pgsql-install/REL9_6_STABLE/include
OK, here's a more principled couple of patches. For config_data, if you
give multiple options it gives you back the list of values. If you don't
specify any, in scalar context it just gives you back all of pg_config's
output, but in array context it gives you a map, so you should be able
to say things like:
my %node_config = $node->config_data;
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
v2-0001-Make-config_data-method-more-flexible.patchtext/x-patch; charset=UTF-8; name=v2-0001-Make-config_data-method-more-flexible.patchDownload
From 0cd2682561f37bfe9b316c5806db1d3526aa21c9 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <andrew@dunslane.net>
Date: Mon, 13 Jun 2022 18:16:33 -0400
Subject: [PATCH v2 1/2] Make config_data method more flexible.
If given a single option, return the value as now. If given multiple
options return the list of values. If given no options at all return the
output in a single string in scalar context, or a map in array context.
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 35 ++++++++++++++++++------
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index c8c7bc5045..2a142e97f5 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -345,27 +345,46 @@ sub pg_version
=pod
-=item $node->config_data($option)
+=item $node->config_data( option ...)
-Return a string holding configuration data from pg_config, with $option
-being the option switch used with the pg_config command.
+Return configuration data from pg_config, using options (if supplied).
+The options will be things like '--sharedir'.
+
+If no options are supplied, return a string in scalar context or a map in
+array context.
+
+If options are supplied, return the list of values.
=cut
sub config_data
{
- my ($self, $option) = @_;
+ my ($self, @options) = @_;
local %ENV = $self->_get_env();
my ($stdout, $stderr);
my $result =
- IPC::Run::run [ $self->installed_command('pg_config'), $option ],
+ IPC::Run::run [ $self->installed_command('pg_config'), @options ],
'>', \$stdout, '2>', \$stderr
or die "could not execute pg_config";
+ # standardize line endings
+ $stdout =~ s/\r(?=\n)//g;
+ # no options, scalar context: just hand back the output
+ return $stdout unless (wantarray || @options);
chomp($stdout);
- $stdout =~ s/\r$//;
-
- return $stdout;
+ # exactly one option: hand back the output (minus LF)
+ return $stdout if (@options == 1);
+ my @lines = split(/\n/, $stdout);
+ # more than one option: hand back the list of values;
+ return @lines if (@options);
+ # no options, array context: return a map
+ my @map;
+ foreach my $line (@lines)
+ {
+ my ($k,$v) = split (/ = /,$line,2);
+ push(@map, $k, $v);
+ }
+ return @map;
}
=pod
--
2.25.1
v2-0002-Use-installed-postgresql.conf.sample-for-GUC-sani.patchtext/x-patch; charset=UTF-8; name=v2-0002-Use-installed-postgresql.conf.sample-for-GUC-sani.patchDownload
From ef9dad29f31469fb3f4befd02258c97d32670ac7 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <andrew@dunslane.net>
Date: Mon, 13 Jun 2022 18:19:39 -0400
Subject: [PATCH v2 2/2] Use installed postgresql.conf.sample for GUC sanity
TAP test
The current code looks for the sample file in the source directory, but
it seems better to test against the installed sample file.
---
src/test/modules/test_misc/t/003_check_guc.pl | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/test/modules/test_misc/t/003_check_guc.pl b/src/test/modules/test_misc/t/003_check_guc.pl
index 60459ef759..1786cd1929 100644
--- a/src/test/modules/test_misc/t/003_check_guc.pl
+++ b/src/test/modules/test_misc/t/003_check_guc.pl
@@ -33,10 +33,9 @@ my $not_in_sample = $node->safe_psql(
ORDER BY 1");
my @not_in_sample_array = split("\n", lc($not_in_sample));
-# TAP tests are executed in the directory of the test, in the source tree,
-# even for VPATH builds, so rely on that to find postgresql.conf.sample.
-my $rootdir = "../../../..";
-my $sample_file = "$rootdir/src/backend/utils/misc/postgresql.conf.sample";
+# use the sample file from the temp install
+my $share_dir = $node->config_data('--sharedir');
+my $sample_file = "$share_dir/postgresql.conf.sample";
# List of all the GUCs found in the sample file.
my @gucs_in_file;
--
2.25.1
Andrew Dunstan <andrew@dunslane.net> writes:
OK, here's a more principled couple of patches. For config_data, if you
give multiple options it gives you back the list of values. If you don't
specify any, in scalar context it just gives you back all of pg_config's
output, but in array context it gives you a map, so you should be able
to say things like:
my %node_config = $node->config_data;
Might be overkill, but since you wrote it already, looks OK to me.
regards, tom lane
On Tue, Jun 14, 2022 at 12:20:56PM -0400, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
The second changes the new GUCs TAP test to check against the installed
postgresql.conf.sample rather than the one in the original source
location. There are probably arguments both ways, but if we ever decided
to postprocess the file before installation, this would do the right thing.Seems like a good idea, especially since it also makes the test code
shorter and more robust(-looking).
It seems to me that you did not look at the git history very closely.
The first version of 003_check_guc.pl did exactly what 0002 is
proposing to do, see b0a55f4. That's also why config_data() has been
introduced in the first place. This original logic has been reverted
once shortly after, as of 52377bb, per a complain by Christoph Berg
because this broke some of the assumptions the custom patches of
Debian relied on:
/messages/by-id/YgYw25OXV5men8Fj@msg.df7cb.de
And it was also pointed out that we'd better use the version in the
source tree rather than a logic that depends on finding the path from
the output of pg_config with an installation tree assumed to exist
(there should be one for installcheck anyway), as of:
/messages/by-id/2023925.1644591595@sss.pgh.pa.us
If the change of 0002 is applied, we will just loop back to the
original issue with Debian. So I am adding Christoph in CC, as he has
also mentioned that the patch applied to PG for Debian that
manipulates the installation paths has been removed, but I may be
wrong in assuming that it is the case.
--
Michael
On Tue, Jun 14, 2022 at 05:08:28PM -0400, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
OK, here's a more principled couple of patches. For config_data, if you
give multiple options it gives you back the list of values. If you don't
specify any, in scalar context it just gives you back all of pg_config's
output, but in array context it gives you a map, so you should be able
to say things like:
my %node_config = $node->config_data;Might be overkill, but since you wrote it already, looks OK to me.
+ # exactly one option: hand back the output (minus LF)
+ return $stdout if (@options == 1);
+ my @lines = split(/\n/, $stdout);
+ # more than one option: hand back the list of values;
+ return @lines if (@options);
+ # no options, array context: return a map
+ my @map;
+ foreach my $line (@lines)
+ {
+ my ($k,$v) = split (/ = /,$line,2);
+ push(@map, $k, $v);
+ }This patch is able to handle the case of no option and one option
specified by the caller of the routine. However, pg_config is able to
return a set of values when specifying multiple switches, respecting
the order of the switches, so wouldn't it be better to return a map
made of ($option, $line)? For example, on a command like `pg_config
--sysconfdir --`, we would get back:
(('--sysconfdir', sysconfdir_val), ('--localedir', localedir_val))
If this is not worth the trouble, I think that you'd better die() hard
if the caller specifies more than two option switches.
--
Michael
On 2022-06-14 Tu 19:24, Michael Paquier wrote:
On Tue, Jun 14, 2022 at 05:08:28PM -0400, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
OK, here's a more principled couple of patches. For config_data, if you
give multiple options it gives you back the list of values. If you don't
specify any, in scalar context it just gives you back all of pg_config's
output, but in array context it gives you a map, so you should be able
to say things like:
my %node_config = $node->config_data;Might be overkill, but since you wrote it already, looks OK to me.
+ # exactly one option: hand back the output (minus LF) + return $stdout if (@options == 1); + my @lines = split(/\n/, $stdout); + # more than one option: hand back the list of values; + return @lines if (@options); + # no options, array context: return a map + my @map; + foreach my $line (@lines) + { + my ($k,$v) = split (/ = /,$line,2); + push(@map, $k, $v); + }This patch is able to handle the case of no option and one option
specified by the caller of the routine. However, pg_config is able to
return a set of values when specifying multiple switches, respecting
the order of the switches, so wouldn't it be better to return a map
made of ($option, $line)? For example, on a command like `pg_config
--sysconfdir --`, we would get back:
(('--sysconfdir', sysconfdir_val), ('--localedir', localedir_val))If this is not worth the trouble, I think that you'd better die() hard
if the caller specifies more than two option switches.
My would we do that? If you want a map don't pass any switches. But as
written you could do:
my ($incdir, $localedir, $sharedir) = $node->config_data(qw(--includedir --localedir --sharedir));
No map needed to get what you want, in fact a map would get in the way.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 2022-06-14 Tu 19:13, Michael Paquier wrote:
On Tue, Jun 14, 2022 at 12:20:56PM -0400, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
The second changes the new GUCs TAP test to check against the installed
postgresql.conf.sample rather than the one in the original source
location. There are probably arguments both ways, but if we ever decided
to postprocess the file before installation, this would do the right thing.Seems like a good idea, especially since it also makes the test code
shorter and more robust(-looking).It seems to me that you did not look at the git history very closely.
The first version of 003_check_guc.pl did exactly what 0002 is
proposing to do, see b0a55f4. That's also why config_data() has been
introduced in the first place. This original logic has been reverted
once shortly after, as of 52377bb, per a complain by Christoph Berg
because this broke some of the assumptions the custom patches of
Debian relied on:
/messages/by-id/YgYw25OXV5men8Fj@msg.df7cb.de
Quite right, I missed that. Still, it now seems to be moot, given what
Christoph said at the bottom of the thread. If I'd seen the thread I
would probably have been inclined to say that is Debian can patch
pg_config they can also patch the test :-)
And it was also pointed out that we'd better use the version in the
source tree rather than a logic that depends on finding the path from
the output of pg_config with an installation tree assumed to exist
(there should be one for installcheck anyway), as of:
/messages/by-id/2023925.1644591595@sss.pgh.pa.usIf the change of 0002 is applied, we will just loop back to the
original issue with Debian. So I am adding Christoph in CC, as he has
also mentioned that the patch applied to PG for Debian that
manipulates the installation paths has been removed, but I may be
wrong in assuming that it is the case.
Honestly, I don't care all that much. I noticed these issues when
dealing with something for EDB that turned out not to be related to
these things. I can see arguments both ways on this one.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Wed, Jun 15, 2022 at 07:59:10AM -0400, Andrew Dunstan wrote:
My would we do that? If you want a map don't pass any switches. But as
written you could do:my ($incdir, $localedir, $sharedir) = $node->config_data(qw(--includedir --localedir --sharedir));
No map needed to get what you want, in fact a map would get in the
way.
Nice, I didn't know you could do that. That's a pattern worth
mentioning in the perldoc part as an example, in my opinion.
--
Michael
On 2022-Jun-14, Andrew Dunstan wrote:
OK, here's a more principled couple of patches. For config_data, if you
give multiple options it gives you back the list of values. If you don't
specify any, in scalar context it just gives you back all of pg_config's
output, but in array context it gives you a map, so you should be able
to say things like:my %node_config = $node->config_data;
Hi, it looks to me like these were forgotten?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On 2022-11-06 Su 08:51, Álvaro Herrera wrote:
On 2022-Jun-14, Andrew Dunstan wrote:
OK, here's a more principled couple of patches. For config_data, if you
give multiple options it gives you back the list of values. If you don't
specify any, in scalar context it just gives you back all of pg_config's
output, but in array context it gives you a map, so you should be able
to say things like:my %node_config = $node->config_data;
Hi, it looks to me like these were forgotten?
Yeah, will get to it this week.
Thanks for the reminder.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 2022-11-09 We 05:35, Andrew Dunstan wrote:
On 2022-11-06 Su 08:51, Álvaro Herrera wrote:
On 2022-Jun-14, Andrew Dunstan wrote:
OK, here's a more principled couple of patches. For config_data, if you
give multiple options it gives you back the list of values. If you don't
specify any, in scalar context it just gives you back all of pg_config's
output, but in array context it gives you a map, so you should be able
to say things like:my %node_config = $node->config_data;
Hi, it looks to me like these were forgotten?
Yeah, will get to it this week.
Thanks for the reminder.
Pushed now.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com