Thread: 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
Attachment
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. (Ihave to admit that using @_ in the body of the function was a little bit confusing to me at first. Did you do that in orderto 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
dothat 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
stripthe "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
Attachment
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: https://www.postgresql.org/message-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: https://www.postgresql.org/message-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
Attachment
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
Attachment
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: > https://www.postgresql.org/message-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: > https://www.postgresql.org/message-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. 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
Attachment
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