multi-install PostgresNode
I've been giving some thought to $subject. The initial impetus is the
promise I made to assist with testing of clients built with NSS against
servers built with openssl, and vice versa.
I've already generalized the process of saving binaries by the buildfarm
client. And we could proceed with a purely bespoke module for testing
the SSL components, as we already do for testing cross-version
pg_upgrade. But it struck me that it might be better to leverage our
existing investment in TAP tests. So I came up with the idea of creating
a child module of PostgresNode.pm, which would set the PATH and other
variables appropriately at the start of each method and restore them on
method exit. So then we could have things like:
$openssl_node->start;
my $connstr = $openssl_node->connstr;
$nss_node->psql($connstr, ...);
To use this a TAP test would need to know two (or more) install paths
for the various nodes, presumably set in environment variables much as
we do now for things like TESTDIR. So for the above example, the TAP
test could set things up with:
my
$openssl_node=PostgresNodePath::get_new_node($ENV{OPENSSL_POSTGRES_INSTALL_PATH},'openssl');
my
$nss_node=PostgresNodePath::get_new_node($ENV{NSS_POSTGRES_INSTALL_PATH},'nss');
Other possible uses would be things like cross-version testing of
pg_dump (How do we know we haven't broken anything in dumping very old
versions?).
The proposed module would look something like this:
package PostgresNodePath;
use strict;
use warnings;
use parent PostgresNode;
use Exporter qw(import);
our @EXPORT = qw(get_new_node);
sub get_new_node
{
my $installpath= shift;
my $node = PostgresNode::get_new_node(@_);
bless $node; # re-bless into current class
$node->{_installpath} = $installpath;
return $node;
}
and then for each class method in PostgresNode.pm we'd have an override
something like:
sub foo
{
my $node=shift;
my $inst = $node->{_installpath};
local %ENV = %ENV;
$ENV{PATH} = "$inst/bin:$ENV{PATH}";
$ENV{LD_LIBRARY_PATH} = "$inst/lib:$ENV{LD_LIBRARY_PATH}";
$node->SUPER::foo(@_);
}
There might be more elegant ways of doing this, but that's what I came
up with.
My main question is: do we want something like this in the core code
(presumably in src/test/perl), or is it not of sufficiently general
interest? If it's wanted I'll submit a patch, probably for the March CF,
but January if I manage to get my running shoes on. If not, I'll put it
in the buildfarm code, but then any TAP tests that want it will likewise
need to live there.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Thu, Dec 17, 2020 at 04:37:54PM -0500, Andrew Dunstan wrote:
The proposed module would look something like this:
[...]
use parent PostgresNode;
sub get_new_node
{
my $installpath= shift;
my $node = PostgresNode::get_new_node(@_);
bless $node; # re-bless into current class
$node->{_installpath} = $installpath;
return $node;
}
Passing down the installpath as argument and saving it within a
PostgresNode or child class looks like the correct way of doing things
to me. This would require an extra routine to be able to get the
install path from a node as _installpath would remain internal to the
module file, right? Shouldn't it be something that ought to be
directly part of PostgresNode actually, where we could enforce the lib
and bin paths to the output of pg_config if an _installpath is not
provided by the caller? In short, I am not sure that we need an extra
module here.
and then for each class method in PostgresNode.pm we'd have an override
something like:sub foo
{
my $node=shift;
my $inst = $node->{_installpath};
local %ENV = %ENV;
$ENV{PATH} = "$inst/bin:$ENV{PATH}";
$ENV{LD_LIBRARY_PATH} = "$inst/lib:$ENV{LD_LIBRARY_PATH}";
$node->SUPER::foo(@_);
}There might be more elegant ways of doing this, but that's what I came
up with.
As long as it does not become necessary to pass down _installpath to
all indidivual binary calls we have in PostgresNode or the extra
module, this gets a +1 from me. So, if I am getting that right, the
key point is the use of local %ENV here to make sure that PATH and
LD_LIBRARY_PATH are only enforced when it comes to calls within
PostgresNode.pm, right? That's an elegant solution. This is
something I have wanted for a long time for pg_upgrade to be able to
get rid of its test.sh.
My main question is: do we want something like this in the core code
(presumably in src/test/perl), or is it not of sufficiently general
interest? If it's wanted I'll submit a patch, probably for the March CF,
but January if I manage to get my running shoes on. If not, I'll put it
in the buildfarm code, but then any TAP tests that want it will likewise
need to live there.
This facility gives us the possibility to clean up the test code of
pg_upgrade and move it to a TAP test, so I'd say that it is worth
having in the core code in the long-term.
--
Michael
On 12/17/20 7:55 PM, Michael Paquier wrote:
On Thu, Dec 17, 2020 at 04:37:54PM -0500, Andrew Dunstan wrote:
The proposed module would look something like this:
[...]
use parent PostgresNode;
sub get_new_node
{
��� my $installpath= shift;
��� my $node = PostgresNode::get_new_node(@_);
��� bless $node; # re-bless into current class
��� $node->{_installpath} = $installpath;
��� return $node;
}Passing down the installpath as argument and saving it within a
PostgresNode or child class looks like the correct way of doing things
to me. This would require an extra routine to be able to get the
install path from a node as _installpath would remain internal to the
module file, right? Shouldn't it be something that ought to be
directly part of PostgresNode actually, where we could enforce the lib
and bin paths to the output of pg_config if an _installpath is not
provided by the caller? In short, I am not sure that we need an extra
module here.and then� for each class method in PostgresNode.pm we'd have an override
something like:sub foo
{
��� my $node=shift;
��� my $inst = $node->{_installpath};
��� local %ENV = %ENV;
��� $ENV{PATH} = "$inst/bin:$ENV{PATH}";
��� $ENV{LD_LIBRARY_PATH} = "$inst/lib:$ENV{LD_LIBRARY_PATH}";
��� $node->SUPER::foo(@_);
}There might be more elegant ways of doing this, but that's what I came
up with.As long as it does not become necessary to pass down _installpath to
all indidivual binary calls we have in PostgresNode or the extra
module, this gets a +1 from me. So, if I am getting that right, the
key point is the use of local %ENV here to make sure that PATH and
LD_LIBRARY_PATH are only enforced when it comes to calls within
PostgresNode.pm, right? That's an elegant solution. This is
something I have wanted for a long time for pg_upgrade to be able to
get rid of its test.sh.My main question is: do we want something like this in the core code
(presumably in src/test/perl), or is it not of sufficiently general
interest? If it's wanted I'll submit a patch, probably for the March CF,
but January if I manage to get my running shoes on. If not, I'll put it
in the buildfarm code, but then any TAP tests that want it will likewise
need to live there.This facility gives us the possibility to clean up the test code of
pg_upgrade and move it to a TAP test, so I'd say that it is worth
having in the core code in the long-term.
This turns out to be remarkably short, with the use of a little eval magic.
Give the attached, this test program works just fine:
#!/bin/perl
use PostgresNodePath;
$ENV{PG_REGRESS} =
'/home/andrew/pgl/vpath.12/src/test/regress/pg_regress';
my $node = get_new_node('/home/andrew/pgl/inst.12.5711','blurfl');
print $node->info;
print $node->connstr(),"\n";
$node->init();
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
On 12/19/20 11:19 AM, Andrew Dunstan wrote:
This turns out to be remarkably short, with the use of a little eval magic.
Give the attached, this test program works just fine:
#!/bin/perl
use PostgresNodePath;
$ENV{PG_REGRESS} =
'/home/andrew/pgl/vpath.12/src/test/regress/pg_regress';
my $node = get_new_node('/home/andrew/pgl/inst.12.5711','blurfl');
print $node->info;
print $node->connstr(),"\n";
$node->init();
This time with a typo removed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
On 2020-12-20 18:09, Andrew Dunstan wrote:
On 12/19/20 11:19 AM, Andrew Dunstan wrote:
This turns out to be remarkably short, with the use of a little eval magic.
Give the attached, this test program works just fine:
#!/bin/perl
use PostgresNodePath;
$ENV{PG_REGRESS} =
'/home/andrew/pgl/vpath.12/src/test/regress/pg_regress';
my $node = get_new_node('/home/andrew/pgl/inst.12.5711','blurfl');
print $node->info;
print $node->connstr(),"\n";
$node->init();This time with a typo removed.
What is proposed the destination of this file? Is it meant to be part
of a patch? Is it meant to be installed? Is it meant for the buildfarm
code?
On 17 Dec 2020, at 22:37, Andrew Dunstan <andrew@dunslane.net> wrote:
I've been giving some thought to $subject. The initial impetus is the
promise I made to assist with testing of clients built with NSS against
servers built with openssl, and vice versa.
Thanks for tackling!
My main question is: do we want something like this in the core code
(presumably in src/test/perl), or is it not of sufficiently general
interest?
To be able to implement pg_upgrade tests as TAP tests seems like enough of a
win to consider this for inclusion in core.
cheers ./daniel
On 1/11/21 9:34 AM, Peter Eisentraut wrote:
On 2020-12-20 18:09, Andrew Dunstan wrote:
On 12/19/20 11:19 AM, Andrew Dunstan wrote:
This turns out to be remarkably short, with the use of a little eval
magic.Give the attached, this test program works just fine:
���� #!/bin/perl
���� use PostgresNodePath;
���� $ENV{PG_REGRESS} =
���� '/home/andrew/pgl/vpath.12/src/test/regress/pg_regress';
���� my $node = get_new_node('/home/andrew/pgl/inst.12.5711','blurfl');
���� print $node->info;
���� print $node->connstr(),"\n";
���� $node->init();This time with a typo removed.
What is proposed the destination of this file?� Is it meant to be part
of a patch?� Is it meant to be installed?� Is it meant for the
buildfarm code?
Core code, ideally. I will submit a patch.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 1/13/21 7:56 AM, Andrew Dunstan wrote:
On 1/11/21 9:34 AM, Peter Eisentraut wrote:
On 2020-12-20 18:09, Andrew Dunstan wrote:
On 12/19/20 11:19 AM, Andrew Dunstan wrote:
This turns out to be remarkably short, with the use of a little eval
magic.Give the attached, this test program works just fine:
���� #!/bin/perl
���� use PostgresNodePath;
���� $ENV{PG_REGRESS} =
���� '/home/andrew/pgl/vpath.12/src/test/regress/pg_regress';
���� my $node = get_new_node('/home/andrew/pgl/inst.12.5711','blurfl');
���� print $node->info;
���� print $node->connstr(),"\n";
���� $node->init();This time with a typo removed.
What is proposed the destination of this file?� Is it meant to be part
of a patch?� Is it meant to be installed?� Is it meant for the
buildfarm code?Core code, ideally. I will submit a patch.
cheers
Here it is as a patch. I've added some docco in perl pod format, and
made it suitable for using on Windows and OSX as well as Linux/*BSD,
although I haven't tested on anything except Linux.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
PostgresNodePath-1.patchtext/x-patch; charset=UTF-8; name=PostgresNodePath-1.patchDownload
diff --git a/src/test/perl/PostgresNodePath.pm b/src/test/perl/PostgresNodePath.pm
new file mode 100644
index 0000000000..4787566dd7
--- /dev/null
+++ b/src/test/perl/PostgresNodePath.pm
@@ -0,0 +1,169 @@
+
+=pod
+
+=head1 NAME
+
+PostgresNodePath - subclass of PostgresNode using a given Postgres install
+
+=head1 SYNOPSIS
+
+ use lib '/path/to/postgres/src/test/perl';
+ use PostgresNodePath;
+
+ my $node = get_new_node('/path/to/binary/installation','my_node');
+
+ or
+
+ my $node = PostgresNodePath->get_new_node('/path/to/binary/installation',
+ 'my_node');
+
+ $node->init();
+ $node->start();
+
+ ...
+
+=head1 DESCRIPTION
+
+PostgresNodePath is a subclass of PostgresNode which runs commands in the
+context of a given PostgreSQL install path. The given path is
+is expected to have a standard install, with bin and lib
+subdirectories.
+
+The only API difference between this and PostgresNode is
+that get_new_node() takes an additional parameter in position
+1 that contains the install path. Everything else is either
+inherited from PostgresNode or overridden but with identical
+parameters.
+
+As with PostgresNode, the environment variable PG_REGRESS
+must point to a binary of pg_regress, in order to init() a
+node.
+
+=cut
+
+
+
+package PostgresNodePath;
+
+use strict;
+use warnings;
+
+use parent qw(PostgresNode);
+
+use Exporter qw(import);
+our @EXPORT = qw(get_new_node);
+
+use Config;
+use TestLib();
+
+sub get_new_node
+{
+ my $class = __PACKAGE__;
+ die 'no args' unless scalar(@_);
+ my $installpath = shift;
+ # check if we got a class name before the installpath
+ if ($installpath =~ /^[A-Za-z0-9_](::[A-Za-z0-9_])*/
+ && !-e "$installpath/bin")
+ {
+ $class = $installpath;
+ $installpath = shift;
+ }
+ my $node = PostgresNode->get_new_node(@_);
+ bless $node, $class; # re-bless
+ $node->{_installpath} = $installpath;
+ return $node;
+}
+
+
+# class methods we don't override:
+# new() # should probably be hidden
+# get_free_port()
+# can_bind()
+
+
+# instance methods we don't override because we don't need to:
+# port() host() basedir() name() logfile() connstr() group_access() data_dir()
+# archive_dir() backup_dir() dump_info() ? set_replication_conf() append_conf()
+# backup_fs_hot() backup_fs_cold() _backup_fs() init_from_backup()
+# rotate_logfile() enable_streaming() enable_restoring() set_recovery_mode()
+# set_standby_mode() enable_archiving() _update_pid teardown_node()
+# clean_node() lsn() wait_for_catchup() wait_for_slot_catchup() query_hash()
+# slot()
+
+# info is special - we add in the installpath spec
+# no need for environment override
+sub info
+{
+ my $node = shift;
+ my $inst = $node->{_installpath};
+ my $res = $node->SUPER::info();
+ $res .= "Install Path: $inst\n";
+ return $res;
+}
+
+BEGIN
+{
+
+ # putting this in a BEGIN block means it's run and checked by perl -c
+
+
+ # everything other than info and get_new_node that we need to override.
+ # they are all instance methods, so we can use the same template for all.
+ my @instance_overrides = qw(init backup start kill9 stop reload restart
+ promote logrotate safe_psql psql background_psql
+ interactive_psql poll_query_until command_ok
+ command_fails command_like command_checks_all
+ issues_sql_like run_log pg_recvlogical_upto
+ );
+
+ my $dylib_name =
+ $Config{osname} eq 'darwin' ? "DYLD_LIBRARY_PATH" : "LD_LIBRARY_PATH";
+
+ my $template = <<' EOSUB';
+
+ sub SUBNAME
+ {
+ my $node=shift;
+ my $inst = $node->{_installpath};
+ local %ENV = %ENV;
+ if ($TestLib::windows_os)
+ {
+ # Windows picks up DLLs from the PATH rather than *LD_LIBRARY_PATH
+ # choose the right path separator
+ if ($Config{osname} eq 'MSWin32')
+ {
+ $ENV{PATH} = "$inst/bin;$inst/lib;$ENV{PATH}";
+ }
+ else
+ {
+ $ENV{PATH} = "$inst/bin:$inst/lib:$ENV{PATH}";
+ }
+ }
+ else
+ {
+ $ENV{PATH} = "$inst/bin:$ENV{PATH}";
+ if (exists $ENV{DYLIB})
+ {
+ $ENV{DYLIB} = "$inst/lib:$ENV{DYLIB}";
+ }
+ else
+ {
+ $ENV{DYLIB} = "$inst/lib}";
+ }
+ }
+ $node->SUPER::SUBNAME(@_);
+ }
+
+ EOSUB
+
+ foreach my $subname (@instance_overrides)
+ {
+ my $code = $template;
+ $code =~ s/SUBNAME/$subname/g;
+ $code =~ s/DYLIB/$dylib_name/g;
+ ## no critic (ProhibitStringyEval)
+ eval($code);
+ }
+}
+
+1;
On 2021-Jan-28, Andrew Dunstan wrote:
... neat stuff, thanks.
+ # Windows picks up DLLs from the PATH rather than *LD_LIBRARY_PATH + # choose the right path separator + if ($Config{osname} eq 'MSWin32') + { + $ENV{PATH} = "$inst/bin;$inst/lib;$ENV{PATH}"; + } + else + { + $ENV{PATH} = "$inst/bin:$inst/lib:$ENV{PATH}"; + }
Hmm, if only Windows needs lib/ in PATH, then we do we add $inst/lib to
PATH even when not Windows?
+ if (exists $ENV{DYLIB}) + { + $ENV{DYLIB} = "$inst/lib:$ENV{DYLIB}"; + } + else + { + $ENV{DYLIB} = "$inst/lib}";
Note extra closing } in the string here.
--
�lvaro Herrera 39�49'30"S 73�17'W
On 1/28/21 9:24 AM, Alvaro Herrera wrote:
On 2021-Jan-28, Andrew Dunstan wrote:
... neat stuff, thanks.
+ # Windows picks up DLLs from the PATH rather than *LD_LIBRARY_PATH + # choose the right path separator + if ($Config{osname} eq 'MSWin32') + { + $ENV{PATH} = "$inst/bin;$inst/lib;$ENV{PATH}"; + } + else + { + $ENV{PATH} = "$inst/bin:$inst/lib:$ENV{PATH}"; + }Hmm, if only Windows needs lib/ in PATH, then we do we add $inst/lib to
PATH even when not Windows?
We could, but there's no point AFAICS. *nix dynamic loaders don't use
the PATH on any platform to my knowledge. This is mainly so that Windows
will find libpq.dll correctly.
+ if (exists $ENV{DYLIB}) + { + $ENV{DYLIB} = "$inst/lib:$ENV{DYLIB}"; + } + else + { + $ENV{DYLIB} = "$inst/lib}";Note extra closing } in the string here.
Oops. fixed, thanks
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
PostgresNodePath-2.patchtext/x-patch; charset=UTF-8; name=PostgresNodePath-2.patchDownload
diff --git a/src/test/perl/PostgresNodePath.pm b/src/test/perl/PostgresNodePath.pm
new file mode 100644
index 0000000000..83fd371192
--- /dev/null
+++ b/src/test/perl/PostgresNodePath.pm
@@ -0,0 +1,169 @@
+
+=pod
+
+=head1 NAME
+
+PostgresNodePath - subclass of PostgresNode using a given Postgres install
+
+=head1 SYNOPSIS
+
+ use lib '/path/to/postgres/src/test/perl';
+ use PostgresNodePath;
+
+ my $node = get_new_node('/path/to/binary/installation','my_node');
+
+ or
+
+ my $node = PostgresNodePath->get_new_node('/path/to/binary/installation',
+ 'my_node');
+
+ $node->init();
+ $node->start();
+
+ ...
+
+=head1 DESCRIPTION
+
+PostgresNodePath is a subclass of PostgresNode which runs commands in the
+context of a given PostgreSQL install path. The given path is
+is expected to have a standard install, with bin and lib
+subdirectories.
+
+The only API difference between this and PostgresNode is
+that get_new_node() takes an additional parameter in position
+1 that contains the install path. Everything else is either
+inherited from PostgresNode or overridden but with identical
+parameters.
+
+As with PostgresNode, the environment variable PG_REGRESS
+must point to a binary of pg_regress, in order to init() a
+node.
+
+=cut
+
+
+
+package PostgresNodePath;
+
+use strict;
+use warnings;
+
+use parent qw(PostgresNode);
+
+use Exporter qw(import);
+our @EXPORT = qw(get_new_node);
+
+use Config;
+use TestLib();
+
+sub get_new_node
+{
+ my $class = __PACKAGE__;
+ die 'no args' unless scalar(@_);
+ my $installpath = shift;
+ # check if we got a class name before the installpath
+ if ($installpath =~ /^[A-Za-z0-9_](::[A-Za-z0-9_])*/
+ && !-e "$installpath/bin")
+ {
+ $class = $installpath;
+ $installpath = shift;
+ }
+ my $node = PostgresNode->get_new_node(@_);
+ bless $node, $class; # re-bless
+ $node->{_installpath} = $installpath;
+ return $node;
+}
+
+
+# class methods we don't override:
+# new() # should probably be hidden
+# get_free_port()
+# can_bind()
+
+
+# instance methods we don't override because we don't need to:
+# port() host() basedir() name() logfile() connstr() group_access() data_dir()
+# archive_dir() backup_dir() dump_info() ? set_replication_conf() append_conf()
+# backup_fs_hot() backup_fs_cold() _backup_fs() init_from_backup()
+# rotate_logfile() enable_streaming() enable_restoring() set_recovery_mode()
+# set_standby_mode() enable_archiving() _update_pid teardown_node()
+# clean_node() lsn() wait_for_catchup() wait_for_slot_catchup() query_hash()
+# slot()
+
+# info is special - we add in the installpath spec
+# no need for environment override
+sub info
+{
+ my $node = shift;
+ my $inst = $node->{_installpath};
+ my $res = $node->SUPER::info();
+ $res .= "Install Path: $inst\n";
+ return $res;
+}
+
+BEGIN
+{
+
+ # putting this in a BEGIN block means it's run and checked by perl -c
+
+
+ # everything other than info and get_new_node that we need to override.
+ # they are all instance methods, so we can use the same template for all.
+ my @instance_overrides = qw(init backup start kill9 stop reload restart
+ promote logrotate safe_psql psql background_psql
+ interactive_psql poll_query_until command_ok
+ command_fails command_like command_checks_all
+ issues_sql_like run_log pg_recvlogical_upto
+ );
+
+ my $dylib_name =
+ $Config{osname} eq 'darwin' ? "DYLD_LIBRARY_PATH" : "LD_LIBRARY_PATH";
+
+ my $template = <<' EOSUB';
+
+ sub SUBNAME
+ {
+ my $node=shift;
+ my $inst = $node->{_installpath};
+ local %ENV = %ENV;
+ if ($TestLib::windows_os)
+ {
+ # Windows picks up DLLs from the PATH rather than *LD_LIBRARY_PATH
+ # choose the right path separator
+ if ($Config{osname} eq 'MSWin32')
+ {
+ $ENV{PATH} = "$inst/bin;$inst/lib;$ENV{PATH}";
+ }
+ else
+ {
+ $ENV{PATH} = "$inst/bin:$inst/lib:$ENV{PATH}";
+ }
+ }
+ else
+ {
+ $ENV{PATH} = "$inst/bin:$ENV{PATH}";
+ if (exists $ENV{DYLIB})
+ {
+ $ENV{DYLIB} = "$inst/lib:$ENV{DYLIB}";
+ }
+ else
+ {
+ $ENV{DYLIB} = "$inst/lib";
+ }
+ }
+ $node->SUPER::SUBNAME(@_);
+ }
+
+ EOSUB
+
+ foreach my $subname (@instance_overrides)
+ {
+ my $code = $template;
+ $code =~ s/SUBNAME/$subname/g;
+ $code =~ s/DYLIB/$dylib_name/g;
+ ## no critic (ProhibitStringyEval)
+ eval($code);
+ }
+}
+
+1;
On 1/13/21 7:25 AM, Daniel Gustafsson wrote:
On 17 Dec 2020, at 22:37, Andrew Dunstan <andrew@dunslane.net> wrote:
I've been giving some thought to $subject. The initial impetus is the
promise I made to assist with testing of clients built with NSS against
servers built with openssl, and vice versa.Thanks for tackling!
My main question is: do we want something like this in the core code
(presumably in src/test/perl), or is it not of sufficiently general
interest?To be able to implement pg_upgrade tests as TAP tests seems like enough of a
win to consider this for inclusion in core.
Daniel, did you have any further comments on this? If not, does anyone
object to my committing it?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Thu, Jan 28, 2021 at 10:19:57AM -0500, Andrew Dunstan wrote:
+BEGIN +{ + + # putting this in a BEGIN block means it's run and checked by perl -c + + + # everything other than info and get_new_node that we need to override. + # they are all instance methods, so we can use the same template for all. + my @instance_overrides = qw(init backup start kill9 stop reload restart + promote logrotate safe_psql psql background_psql + interactive_psql poll_query_until command_ok + command_fails command_like command_checks_all + issues_sql_like run_log pg_recvlogical_upto + );
No actual objections here, but it would be easy to miss the addition
of a new routine. Would an exclusion filter be more adapted, aka
override everything except get_new_node() and info()?
--
Michael
On 3/23/21 6:36 PM, Michael Paquier wrote:
On Thu, Jan 28, 2021 at 10:19:57AM -0500, Andrew Dunstan wrote:
+BEGIN +{ + + # putting this in a BEGIN block means it's run and checked by perl -c + + + # everything other than info and get_new_node that we need to override. + # they are all instance methods, so we can use the same template for all. + my @instance_overrides = qw(init backup start kill9 stop reload restart + promote logrotate safe_psql psql background_psql + interactive_psql poll_query_until command_ok + command_fails command_like command_checks_all + issues_sql_like run_log pg_recvlogical_upto + );No actual objections here, but it would be easy to miss the addition
of a new routine. Would an exclusion filter be more adapted, aka
override everything except get_new_node() and info()?
Actually, following a brief offline discussion today I've thought of a
way that doesn't require subclassing. Will post that probably tomorrow.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 3/23/21 7:09 PM, Andrew Dunstan wrote:
On 3/23/21 6:36 PM, Michael Paquier wrote:
On Thu, Jan 28, 2021 at 10:19:57AM -0500, Andrew Dunstan wrote:
+BEGIN +{ + + # putting this in a BEGIN block means it's run and checked by perl -c + + + # everything other than info and get_new_node that we need to override. + # they are all instance methods, so we can use the same template for all. + my @instance_overrides = qw(init backup start kill9 stop reload restart + promote logrotate safe_psql psql background_psql + interactive_psql poll_query_until command_ok + command_fails command_like command_checks_all + issues_sql_like run_log pg_recvlogical_upto + );No actual objections here, but it would be easy to miss the addition
of a new routine. Would an exclusion filter be more adapted, aka
override everything except get_new_node() and info()?Actually, following a brief offline discussion today I've thought of a
way that doesn't require subclassing. Will post that probably tomorrow.
And here it is. No subclass, no eval, no magic :-) Some of my colleagues
are a lot happier ;-)
The downside is that we need to litter PostgresNode with a bunch of
lines like:
local %ENV = %ENV;
_set_install_env($self);
The upside is that there's no longer a possibility that someone will add
a new routine to PostgresNode and forget to update the subclass.
Here is my simple test program:
#!/usr/bin/perl
use lib '/home/andrew/pgl/pg_head/src/test/perl';
# PostgresNode (via TestLib) hijacks stdout, so make a dup before it
gets a chance
use vars qw($out);
BEGIN
{
��� open ($out, ">&STDOUT");
}
use PostgresNode;
my $node = PostgresNode->get_new_node('v12', install_path =>
'/home/andrew/pgl/inst.12.5711');
$ENV{PG_REGRESS} = '/bin/true'; # stupid but necessary
$node->init();
$node->start();
my $version = $node->safe_psql('postgres', 'select version()');
$node->stop();
print $out "Version: $version\n";
print $out $node->info();
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
PostgresNodePath-4.patchtext/x-patch; charset=UTF-8; name=PostgresNodePath-4.patchDownload
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 97e05993be..5eabea4b2b 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -355,6 +355,7 @@ sub info
print $fh "Archive directory: " . $self->archive_dir . "\n";
print $fh "Connection string: " . $self->connstr . "\n";
print $fh "Log file: " . $self->logfile . "\n";
+ print $fh "Install Path: " , $self->{_install_path} . "\n" if $self->{_install_path};
close $fh or die;
return $_info;
}
@@ -428,6 +429,9 @@ sub init
my $pgdata = $self->data_dir;
my $host = $self->host;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
$params{allows_streaming} = 0 unless defined $params{allows_streaming};
$params{has_archiving} = 0 unless defined $params{has_archiving};
@@ -555,6 +559,9 @@ sub backup
my $backup_path = $self->backup_dir . '/' . $backup_name;
my $name = $self->name;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
TestLib::system_or_bail(
'pg_basebackup', '-D', $backup_path, '-h',
@@ -780,22 +787,22 @@ sub start
my $name = $self->name;
my $ret;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
BAIL_OUT("node \"$name\" is already running") if defined $self->{_pid};
print("### Starting node \"$name\"\n");
- {
- # Temporarily unset PGAPPNAME so that the server doesn't
- # inherit it. Otherwise this could affect libpqwalreceiver
- # connections in confusing ways.
- local %ENV = %ENV;
- delete $ENV{PGAPPNAME};
-
- # Note: We set the cluster_name here, not in postgresql.conf (in
- # sub init) so that it does not get copied to standbys.
- $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
+ # Temporarily unset PGAPPNAME so that the server doesn't
+ # inherit it. Otherwise this could affect libpqwalreceiver
+ # connections in confusing ways.
+ delete $ENV{PGAPPNAME};
+
+ # Note: We set the cluster_name here, not in postgresql.conf (in
+ # sub init) so that it does not get copied to standbys.
+ $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
$self->logfile, '-o', "--cluster-name=$name", 'start');
- }
if ($ret != 0)
{
@@ -826,6 +833,10 @@ sub kill9
my ($self) = @_;
my $name = $self->name;
return unless defined $self->{_pid};
+
+ local %ENV = %ENV;
+ _set_install_env($self);
+
print "### Killing node \"$name\" using signal 9\n";
# kill(9, ...) fails under msys Perl 5.8.8, so fall back on pg_ctl.
kill(9, $self->{_pid})
@@ -852,6 +863,10 @@ sub stop
my $port = $self->port;
my $pgdata = $self->data_dir;
my $name = $self->name;
+
+ local %ENV = %ENV;
+ _set_install_env($self);
+
$mode = 'fast' unless defined $mode;
return unless defined $self->{_pid};
print "### Stopping node \"$name\" using mode $mode\n";
@@ -874,6 +889,10 @@ sub reload
my $port = $self->port;
my $pgdata = $self->data_dir;
my $name = $self->name;
+
+ local %ENV = %ENV;
+ _set_install_env($self);
+
print "### Reloading node \"$name\"\n";
TestLib::system_or_bail('pg_ctl', '-D', $pgdata, 'reload');
return;
@@ -895,15 +914,15 @@ sub restart
my $logfile = $self->logfile;
my $name = $self->name;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
print "### Restarting node \"$name\"\n";
- {
- local %ENV = %ENV;
- delete $ENV{PGAPPNAME};
+ delete $ENV{PGAPPNAME};
- TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+ TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
'restart');
- }
$self->_update_pid(1);
return;
@@ -924,6 +943,10 @@ sub promote
my $pgdata = $self->data_dir;
my $logfile = $self->logfile;
my $name = $self->name;
+
+ local %ENV = %ENV;
+ _set_install_env($self);
+
print "### Promoting node \"$name\"\n";
TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
'promote');
@@ -945,6 +968,10 @@ sub logrotate
my $pgdata = $self->data_dir;
my $logfile = $self->logfile;
my $name = $self->name;
+
+ local %ENV = %ENV;
+ _set_install_env($self);
+
print "### Rotating log in node \"$name\"\n";
TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
'logrotate');
@@ -1117,6 +1144,14 @@ By default, all nodes use the same PGHOST value. If specified, generate a
PGHOST specific to this node. This allows multiple nodes to use the same
port.
+=item install_path => '/path/to/postgres/installation'
+
+Using this parameter is it possible to have nodes pointing to different
+installations, for testing different versions together or the same version
+with different build parameters. The provided path must be the parent of the
+installation's 'bin' and 'lib' directories. In the common case where this is
+not provided, Postgres binaries will be found in the caller's PATH.
+
=back
For backwards compatibility, it is also exported as a standalone function,
@@ -1165,12 +1200,65 @@ sub get_new_node
# Lock port number found by creating a new node
my $node = $class->new($name, $host, $port);
+ if ($params{install_path})
+ {
+ $node->{_install_path} = $params{install_path};
+ }
+
# Add node to list of nodes
push(@all_nodes, $node);
return $node;
}
+# Private routine to set the PATH and (DY)LD_LIBRARY_PATH correctly when there
+# is an install path set for the node.
+# Routines that call Postgres binaries need to call this routine like this:
+#
+# local %ENV = %ENV;
+# _set_install_env{$self);
+#
+# This is effectively a no-op if the install path isn't set.
+#
+# The install path set in get_new_node needs to be a directory containing
+# bin and lib subdirectories as in a standard PostgreSQL installation, so this
+# can't be used with installations where the bin and lib directories don't have
+# a common parent directory.
+sub _set_install_env
+{
+ my $self = shift;
+ my $inst = $self->{_install_path};
+ return unless $inst;
+ # the caller must have set up a local copy of %ENV
+ if ($TestLib::windows_os)
+ {
+ # Windows picks up DLLs from the PATH rather than *LD_LIBRARY_PATH
+ # choose the right path separator
+ if ($Config{osname} eq 'MSWin32')
+ {
+ $ENV{PATH} = "$inst/bin;$inst/lib;$ENV{PATH}";
+ }
+ else
+ {
+ $ENV{PATH} = "$inst/bin:$inst/lib:$ENV{PATH}";
+ }
+ }
+ else
+ {
+ my $dylib_name =
+ $Config{osname} eq 'darwin' ? "DYLD_LIBRARY_PATH" : "LD_LIBRARY_PATH";
+ $ENV{PATH} = "$inst/bin:$ENV{PATH}";
+ if (exists $ENV{$dylib_name})
+ {
+ $ENV{$dylib_name} = "$inst/lib:$ENV{$dylib_name}";
+ }
+ else
+ {
+ $ENV{$dylib_name} = "$inst/lib";
+ }
+ }
+}
+
=pod
=item get_free_port()
@@ -1330,6 +1418,9 @@ sub safe_psql
{
my ($self, $dbname, $sql, %params) = @_;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
my ($stdout, $stderr);
my $ret = $self->psql(
@@ -1441,6 +1532,9 @@ sub psql
{
my ($self, $dbname, $sql, %params) = @_;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
my $stdout = $params{stdout};
my $stderr = $params{stderr};
my $replication = $params{replication};
@@ -1634,6 +1728,9 @@ sub background_psql
{
my ($self, $dbname, $stdin, $stdout, $timer, %params) = @_;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
my $replication = $params{replication};
my @psql_params = (
@@ -1712,6 +1809,9 @@ sub interactive_psql
{
my ($self, $dbname, $stdin, $stdout, $timer, %params) = @_;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
my @psql_params = ('psql', '-XAt', '-d', $self->connstr($dbname));
push @psql_params, @{ $params{extra_params} }
@@ -1755,6 +1855,9 @@ sub poll_query_until
{
my ($self, $dbname, $query, $expected) = @_;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
$expected = 't' unless defined($expected); # default value
my $cmd = [ 'psql', '-XAt', '-c', $query, '-d', $self->connstr($dbname) ];
@@ -1810,8 +1913,11 @@ sub command_ok
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
+ $ENV{PGHOST} = $self->host;
+ $ENV{PGPORT} = $self->port;
TestLib::command_ok(@_);
return;
@@ -1831,8 +1937,11 @@ sub command_fails
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
+ $ENV{PGHOST} = $self->host;
+ $ENV{PGPORT} = $self->port;
TestLib::command_fails(@_);
return;
@@ -1852,8 +1961,11 @@ sub command_like
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
+ $ENV{PGHOST} = $self->host;
+ $ENV{PGPORT} = $self->port;
TestLib::command_like(@_);
return;
@@ -1874,8 +1986,11 @@ sub command_checks_all
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
+ $ENV{PGHOST} = $self->host;
+ $ENV{PGPORT} = $self->port;
TestLib::command_checks_all(@_);
return;
@@ -1899,8 +2014,11 @@ sub issues_sql_like
my ($self, $cmd, $expected_sql, $test_name) = @_;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
+ $ENV{PGHOST} = $self->host;
+ $ENV{PGPORT} = $self->port;
truncate $self->logfile, 0;
my $result = TestLib::run_log($cmd);
@@ -1923,8 +2041,11 @@ sub run_log
{
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = %ENV;
+ _set_install_env($self);
+
+ $ENV{PGHOST} = $self->host;
+ $ENV{PGPORT} = $self->port;
TestLib::run_log(@_);
return;
@@ -2174,6 +2295,10 @@ sub pg_recvlogical_upto
{
my ($self, $dbname, $slot_name, $endpos, $timeout_secs, %plugin_options)
= @_;
+
+ local %ENV = %ENV;
+ _set_install_env($self);
+
my ($stdout, $stderr);
my $timeout_exception = 'pg_recvlogical timed out';
Andrew Dunstan <andrew@dunslane.net> writes:
And here it is. No subclass, no eval, no magic :-) Some of my colleagues
are a lot happier ;-)The downside is that we need to litter PostgresNode with a bunch of
lines like:local %ENV = %ENV;
_set_install_env($self);
I think it would be even neater having a method that returns the desired
environment and then have the other methods do:
local %ENV = $self->_get_install_env();
The function could be something like this:
sub _get_install_env
{
my $self = shift;
my $inst = $self->{_install_path};
return %ENV unless $inst;
my %install_env;
if ($TestLib::windows_os)
{
# Windows picks up DLLs from the PATH rather than *LD_LIBRARY_PATH
# choose the right path separator
if ($Config{osname} eq 'MSWin32')
{
$install_env{PATH} = "$inst/bin;$inst/lib;$ENV{PATH}";
}
else
{
$install_env{PATH} = "$inst/bin:$inst/lib:$ENV{PATH}";
}
}
else
{
my $dylib_name =
$Config{osname} eq 'darwin' ? "DYLD_LIBRARY_PATH" : "LD_LIBRARY_PATH";
$install_env{PATH} = "$inst/bin:$ENV{PATH}";
if (exists $ENV{$dylib_name})
{
$install_env{$dylib_name} = "$inst/lib:$ENV{$dylib_name}";
}
else
{
$install_env{$dylib_name} = "$inst/lib";
}
}
return (%ENV, %install_env);
}
- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen
On 3/24/21 7:54 AM, Dagfinn Ilmari Mannsåker wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
And here it is. No subclass, no eval, no magic :-) Some of my colleagues
are a lot happier ;-)The downside is that we need to litter PostgresNode with a bunch of
lines like:local %ENV = %ENV;
_set_install_env($self);I think it would be even neater having a method that returns the desired
environment and then have the other methods do:local %ENV = $self->_get_install_env();
Yeah, that's nice. I'll do that. Thanks.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 2021-Mar-24, Dagfinn Ilmari Manns�ker wrote:
I think it would be even neater having a method that returns the desired
environment and then have the other methods do:local %ENV = $self->_get_install_env();
Hmm, is it possible to integrate PGHOST and PGPORT handling into this
too? Seems like it is, so the name of the routine should be something
more general (and also it should not have the quick "return unless
$inst").
--
�lvaro Herrera 39�49'30"S 73�17'W
"How amazing is that? I call it a night and come back to find that a bug has
been identified and patched while I sleep." (Robert Davidson)
http://archives.postgresql.org/pgsql-sql/2006-03/msg00378.php
On 3/24/21 8:29 AM, Alvaro Herrera wrote:
On 2021-Mar-24, Dagfinn Ilmari Mannsåker wrote:
I think it would be even neater having a method that returns the desired
environment and then have the other methods do:local %ENV = $self->_get_install_env();
Hmm, is it possible to integrate PGHOST and PGPORT handling into this
too? Seems like it is, so the name of the routine should be something
more general (and also it should not have the quick "return unless
$inst").
If we're going to do that we probably shouldn't special case any
particular settings, but simply take any extra arguments as extra env
settings. And if any setting has undef (e.g. PGAPPNAME) we should unset it.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 3/24/21 9:23 AM, Andrew Dunstan wrote:
On 3/24/21 8:29 AM, Alvaro Herrera wrote:
On 2021-Mar-24, Dagfinn Ilmari Mannsåker wrote:
I think it would be even neater having a method that returns the desired
environment and then have the other methods do:local %ENV = $self->_get_install_env();
Hmm, is it possible to integrate PGHOST and PGPORT handling into this
too? Seems like it is, so the name of the routine should be something
more general (and also it should not have the quick "return unless
$inst").If we're going to do that we probably shouldn't special case any
particular settings, but simply take any extra arguments as extra env
settings. And if any setting has undef (e.g. PGAPPNAME) we should unset it.
like this.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
PostgresNodePath-5.patchtext/x-patch; charset=UTF-8; name=PostgresNodePath-5.patchDownload
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 97e05993be..fa2ccf027f 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -355,6 +355,7 @@ sub info
print $fh "Archive directory: " . $self->archive_dir . "\n";
print $fh "Connection string: " . $self->connstr . "\n";
print $fh "Log file: " . $self->logfile . "\n";
+ print $fh "Install Path: " , $self->{_install_path} . "\n" if $self->{_install_path};
close $fh or die;
return $_info;
}
@@ -428,6 +429,8 @@ sub init
my $pgdata = $self->data_dir;
my $host = $self->host;
+ local %ENV = $self->_get_install_env();
+
$params{allows_streaming} = 0 unless defined $params{allows_streaming};
$params{has_archiving} = 0 unless defined $params{has_archiving};
@@ -555,6 +558,8 @@ sub backup
my $backup_path = $self->backup_dir . '/' . $backup_name;
my $name = $self->name;
+ local %ENV = $self->_get_install_env();
+
print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
TestLib::system_or_bail(
'pg_basebackup', '-D', $backup_path, '-h',
@@ -784,18 +789,15 @@ sub start
print("### Starting node \"$name\"\n");
- {
- # Temporarily unset PGAPPNAME so that the server doesn't
- # inherit it. Otherwise this could affect libpqwalreceiver
- # connections in confusing ways.
- local %ENV = %ENV;
- delete $ENV{PGAPPNAME};
-
- # Note: We set the cluster_name here, not in postgresql.conf (in
- # sub init) so that it does not get copied to standbys.
- $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
+ # Temporarily unset PGAPPNAME so that the server doesn't
+ # inherit it. Otherwise this could affect libpqwalreceiver
+ # connections in confusing ways.
+ local %ENV = $self->_get_install_env(PGAPPNAME => undef);
+
+ # Note: We set the cluster_name here, not in postgresql.conf (in
+ # sub init) so that it does not get copied to standbys.
+ $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
$self->logfile, '-o', "--cluster-name=$name", 'start');
- }
if ($ret != 0)
{
@@ -826,6 +828,9 @@ sub kill9
my ($self) = @_;
my $name = $self->name;
return unless defined $self->{_pid};
+
+ local %ENV = $self->_get_install_env();
+
print "### Killing node \"$name\" using signal 9\n";
# kill(9, ...) fails under msys Perl 5.8.8, so fall back on pg_ctl.
kill(9, $self->{_pid})
@@ -852,6 +857,9 @@ sub stop
my $port = $self->port;
my $pgdata = $self->data_dir;
my $name = $self->name;
+
+ local %ENV = $self->_get_install_env();
+
$mode = 'fast' unless defined $mode;
return unless defined $self->{_pid};
print "### Stopping node \"$name\" using mode $mode\n";
@@ -874,6 +882,9 @@ sub reload
my $port = $self->port;
my $pgdata = $self->data_dir;
my $name = $self->name;
+
+ local %ENV = $self->_get_install_env();
+
print "### Reloading node \"$name\"\n";
TestLib::system_or_bail('pg_ctl', '-D', $pgdata, 'reload');
return;
@@ -895,15 +906,12 @@ sub restart
my $logfile = $self->logfile;
my $name = $self->name;
- print "### Restarting node \"$name\"\n";
+ local %ENV = $self->_get_install_env(PGAPPNAME => undef);
- {
- local %ENV = %ENV;
- delete $ENV{PGAPPNAME};
+ print "### Restarting node \"$name\"\n";
- TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+ TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
'restart');
- }
$self->_update_pid(1);
return;
@@ -924,6 +932,9 @@ sub promote
my $pgdata = $self->data_dir;
my $logfile = $self->logfile;
my $name = $self->name;
+
+ local %ENV = $self->_get_install_env();
+
print "### Promoting node \"$name\"\n";
TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
'promote');
@@ -945,6 +956,9 @@ sub logrotate
my $pgdata = $self->data_dir;
my $logfile = $self->logfile;
my $name = $self->name;
+
+ local %ENV = $self->_get_install_env();
+
print "### Rotating log in node \"$name\"\n";
TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
'logrotate');
@@ -1117,6 +1131,14 @@ By default, all nodes use the same PGHOST value. If specified, generate a
PGHOST specific to this node. This allows multiple nodes to use the same
port.
+=item install_path => '/path/to/postgres/installation'
+
+Using this parameter is it possible to have nodes pointing to different
+installations, for testing different versions together or the same version
+with different build parameters. The provided path must be the parent of the
+installation's 'bin' and 'lib' directories. In the common case where this is
+not provided, Postgres binaries will be found in the caller's PATH.
+
=back
For backwards compatibility, it is also exported as a standalone function,
@@ -1165,12 +1187,87 @@ sub get_new_node
# Lock port number found by creating a new node
my $node = $class->new($name, $host, $port);
+ if ($params{install_path})
+ {
+ $node->{_install_path} = $params{install_path};
+ }
+
# Add node to list of nodes
push(@all_nodes, $node);
return $node;
}
+# Private routine to return a copy of the evnvironment with the PATH and (DY)LD_LIBRARY_PATH
+# correctly set when there is an install path set for the node.
+# Routines that call Postgres binaries need to call this routine like this:
+#
+# local %ENV = $self->_get_install_env{[%extra_settings]);
+#
+# A copy of the environmnt is taken and the extra settings (if any) applied. Any setting
+# in %extra_settings which a value that is undefined is deleted; the remainder are
+# set. Then the PATH and (DY)LD_LIBRARY_PATH are adjusted if the node's install path
+# is set, and the copy environment is returned.
+#
+# This original environment is returned if the install path isn't set and there are
+# no extra settings.
+#
+# The install path set in get_new_node needs to be a directory containing
+# bin and lib subdirectories as in a standard PostgreSQL installation, so this
+# can't be used with installations where the bin and lib directories don't have
+# a common parent directory.
+sub _get_install_env
+{
+ my $self = shift;
+ my %inst_env = %ENV;
+ # the remaining arguments are modifications to make to the environment
+ my %mods = (@_);
+ while (my ($k,$v) = each %mods)
+ {
+ if (defined $v)
+ {
+ $inst_env{$k} = "$v";
+ }
+ else
+ {
+ delete $inst_env{$k};
+ }
+ }
+ # now fix up the new environment for the install path
+ my $inst = $self->{_install_path};
+ if ($inst)
+ {
+ if ($TestLib::windows_os)
+ {
+ # Windows picks up DLLs from the PATH rather than *LD_LIBRARY_PATH
+ # choose the right path separator
+ if ($Config{osname} eq 'MSWin32')
+ {
+ $inst_env{PATH} = "$inst/bin;$inst/lib;$ENV{PATH}";
+ }
+ else
+ {
+ $inst_env{PATH} = "$inst/bin:$inst/lib:$ENV{PATH}";
+ }
+ }
+ else
+ {
+ my $dylib_name =
+ $Config{osname} eq 'darwin' ? "DYLD_LIBRARY_PATH" : "LD_LIBRARY_PATH";
+ $inst_env{PATH} = "$inst/bin:$ENV{PATH}";
+ if (exists $ENV{$dylib_name})
+ {
+ $inst_env{$dylib_name} = "$inst/lib:$ENV{$dylib_name}";
+ }
+ else
+ {
+ $inst_env{$dylib_name} = "$inst/lib";
+ }
+ }
+ }
+ return (%inst_env);
+}
+
=pod
=item get_free_port()
@@ -1330,6 +1427,8 @@ sub safe_psql
{
my ($self, $dbname, $sql, %params) = @_;
+ local %ENV = $self->_get_install_env();
+
my ($stdout, $stderr);
my $ret = $self->psql(
@@ -1441,6 +1540,8 @@ sub psql
{
my ($self, $dbname, $sql, %params) = @_;
+ local %ENV = $self->_get_install_env();
+
my $stdout = $params{stdout};
my $stderr = $params{stderr};
my $replication = $params{replication};
@@ -1634,6 +1735,8 @@ sub background_psql
{
my ($self, $dbname, $stdin, $stdout, $timer, %params) = @_;
+ local %ENV = $self->_get_install_env();
+
my $replication = $params{replication};
my @psql_params = (
@@ -1712,6 +1815,8 @@ sub interactive_psql
{
my ($self, $dbname, $stdin, $stdout, $timer, %params) = @_;
+ local %ENV = $self->_get_install_env();
+
my @psql_params = ('psql', '-XAt', '-d', $self->connstr($dbname));
push @psql_params, @{ $params{extra_params} }
@@ -1755,6 +1860,8 @@ sub poll_query_until
{
my ($self, $dbname, $query, $expected) = @_;
+ local %ENV = $self->_get_install_env();
+
$expected = 't' unless defined($expected); # default value
my $cmd = [ 'psql', '-XAt', '-c', $query, '-d', $self->connstr($dbname) ];
@@ -1810,8 +1917,10 @@ sub command_ok
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env(
+ PGHOST => $self->host,
+ PGPORT => $self->port,
+ );
TestLib::command_ok(@_);
return;
@@ -1831,8 +1940,10 @@ sub command_fails
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env(
+ PGHOST => $self->host,
+ PGPORT => $self->port,
+ );
TestLib::command_fails(@_);
return;
@@ -1852,8 +1963,10 @@ sub command_like
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env(
+ PGHOST => $self->host,
+ PGPORT => $self->port,
+ );
TestLib::command_like(@_);
return;
@@ -1874,8 +1987,10 @@ sub command_checks_all
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env(
+ PGHOST => $self->host,
+ PGPORT => $self->port,
+ );
TestLib::command_checks_all(@_);
return;
@@ -1899,8 +2014,10 @@ sub issues_sql_like
my ($self, $cmd, $expected_sql, $test_name) = @_;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env(
+ PGHOST => $self->host,
+ PGPORT => $self->port,
+ );
truncate $self->logfile, 0;
my $result = TestLib::run_log($cmd);
@@ -1923,8 +2040,10 @@ sub run_log
{
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env(
+ PGHOST => $self->host,
+ PGPORT => $self->port,
+ );
TestLib::run_log(@_);
return;
@@ -2174,6 +2293,9 @@ sub pg_recvlogical_upto
{
my ($self, $dbname, $slot_name, $endpos, $timeout_secs, %plugin_options)
= @_;
+
+ local %ENV = $self->_get_install_env();
+
my ($stdout, $stderr);
my $timeout_exception = 'pg_recvlogical timed out';
On 2021-Mar-24, Andrew Dunstan wrote:
On 3/24/21 9:23 AM, Andrew Dunstan wrote:
On 3/24/21 8:29 AM, Alvaro Herrera wrote:
If we're going to do that we probably shouldn't special case any
particular settings, but simply take any extra arguments as extra env
settings. And if any setting has undef (e.g. PGAPPNAME) we should unset it.
like this.
Hmm, I like that PGAPPNAME handling has resulted in an overall
simplification. I'm not sure why you prefer to keep PGHOST and PGPORT
handled individually at each callsite however; why not do it like
_install, and add them to the environment always? I doubt there's
anything that requires them *not* to be set; and if there is, it's easy
to make the claim that that's broken and should be fixed.
I'm just saying that cluttering _get_install_env() with those two
settings would result in less clutter overall.
--
�lvaro Herrera Valdivia, Chile
On 3/24/21 11:41 AM, Alvaro Herrera wrote:
On 2021-Mar-24, Andrew Dunstan wrote:
On 3/24/21 9:23 AM, Andrew Dunstan wrote:
On 3/24/21 8:29 AM, Alvaro Herrera wrote:
If we're going to do that we probably shouldn't special case any
particular settings, but simply take any extra arguments as extra env
settings. And if any setting has undef (e.g. PGAPPNAME) we should unset it.like this.
Hmm, I like that PGAPPNAME handling has resulted in an overall
simplification. I'm not sure why you prefer to keep PGHOST and PGPORT
handled individually at each callsite however; why not do it like
_install, and add them to the environment always? I doubt there's
anything that requires them *not* to be set; and if there is, it's easy
to make the claim that that's broken and should be fixed.I'm just saying that cluttering _get_install_env() with those two
settings would result in less clutter overall.
OK, like this?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachments:
PostgresNodePath-6.patchtext/x-patch; charset=UTF-8; name=PostgresNodePath-6.patchDownload
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 97e05993be..a1473d220f 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -355,6 +355,7 @@ sub info
print $fh "Archive directory: " . $self->archive_dir . "\n";
print $fh "Connection string: " . $self->connstr . "\n";
print $fh "Log file: " . $self->logfile . "\n";
+ print $fh "Install Path: " , $self->{_install_path} . "\n" if $self->{_install_path};
close $fh or die;
return $_info;
}
@@ -428,6 +429,8 @@ sub init
my $pgdata = $self->data_dir;
my $host = $self->host;
+ local %ENV = $self->_get_install_env();
+
$params{allows_streaming} = 0 unless defined $params{allows_streaming};
$params{has_archiving} = 0 unless defined $params{has_archiving};
@@ -555,6 +558,8 @@ sub backup
my $backup_path = $self->backup_dir . '/' . $backup_name;
my $name = $self->name;
+ local %ENV = $self->_get_install_env();
+
print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
TestLib::system_or_bail(
'pg_basebackup', '-D', $backup_path, '-h',
@@ -784,18 +789,15 @@ sub start
print("### Starting node \"$name\"\n");
- {
- # Temporarily unset PGAPPNAME so that the server doesn't
- # inherit it. Otherwise this could affect libpqwalreceiver
- # connections in confusing ways.
- local %ENV = %ENV;
- delete $ENV{PGAPPNAME};
-
- # Note: We set the cluster_name here, not in postgresql.conf (in
- # sub init) so that it does not get copied to standbys.
- $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
+ # Temporarily unset PGAPPNAME so that the server doesn't
+ # inherit it. Otherwise this could affect libpqwalreceiver
+ # connections in confusing ways.
+ local %ENV = $self->_get_install_env(PGAPPNAME => undef);
+
+ # Note: We set the cluster_name here, not in postgresql.conf (in
+ # sub init) so that it does not get copied to standbys.
+ $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
$self->logfile, '-o', "--cluster-name=$name", 'start');
- }
if ($ret != 0)
{
@@ -826,6 +828,9 @@ sub kill9
my ($self) = @_;
my $name = $self->name;
return unless defined $self->{_pid};
+
+ local %ENV = $self->_get_install_env();
+
print "### Killing node \"$name\" using signal 9\n";
# kill(9, ...) fails under msys Perl 5.8.8, so fall back on pg_ctl.
kill(9, $self->{_pid})
@@ -852,6 +857,9 @@ sub stop
my $port = $self->port;
my $pgdata = $self->data_dir;
my $name = $self->name;
+
+ local %ENV = $self->_get_install_env();
+
$mode = 'fast' unless defined $mode;
return unless defined $self->{_pid};
print "### Stopping node \"$name\" using mode $mode\n";
@@ -874,6 +882,9 @@ sub reload
my $port = $self->port;
my $pgdata = $self->data_dir;
my $name = $self->name;
+
+ local %ENV = $self->_get_install_env();
+
print "### Reloading node \"$name\"\n";
TestLib::system_or_bail('pg_ctl', '-D', $pgdata, 'reload');
return;
@@ -895,15 +906,12 @@ sub restart
my $logfile = $self->logfile;
my $name = $self->name;
- print "### Restarting node \"$name\"\n";
+ local %ENV = $self->_get_install_env(PGAPPNAME => undef);
- {
- local %ENV = %ENV;
- delete $ENV{PGAPPNAME};
+ print "### Restarting node \"$name\"\n";
- TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+ TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
'restart');
- }
$self->_update_pid(1);
return;
@@ -924,6 +932,9 @@ sub promote
my $pgdata = $self->data_dir;
my $logfile = $self->logfile;
my $name = $self->name;
+
+ local %ENV = $self->_get_install_env();
+
print "### Promoting node \"$name\"\n";
TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
'promote');
@@ -945,6 +956,9 @@ sub logrotate
my $pgdata = $self->data_dir;
my $logfile = $self->logfile;
my $name = $self->name;
+
+ local %ENV = $self->_get_install_env();
+
print "### Rotating log in node \"$name\"\n";
TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
'logrotate');
@@ -1117,6 +1131,14 @@ By default, all nodes use the same PGHOST value. If specified, generate a
PGHOST specific to this node. This allows multiple nodes to use the same
port.
+=item install_path => '/path/to/postgres/installation'
+
+Using this parameter is it possible to have nodes pointing to different
+installations, for testing different versions together or the same version
+with different build parameters. The provided path must be the parent of the
+installation's 'bin' and 'lib' directories. In the common case where this is
+not provided, Postgres binaries will be found in the caller's PATH.
+
=back
For backwards compatibility, it is also exported as a standalone function,
@@ -1165,12 +1187,85 @@ sub get_new_node
# Lock port number found by creating a new node
my $node = $class->new($name, $host, $port);
+ if ($params{install_path})
+ {
+ $node->{_install_path} = $params{install_path};
+ }
+
# Add node to list of nodes
push(@all_nodes, $node);
return $node;
}
+# Private routine to return a copy of the environment with the PATH and (DY)LD_LIBRARY_PATH
+# correctly set when there is an install path set for the node.
+# Routines that call Postgres binaries need to call this routine like this:
+#
+# local %ENV = $self->_get_install_env{[%extra_settings]);
+#
+# A copy of the environmnt is taken and node's host and port settings are added
+# as PGHOST and PGPORT, Then the extra settings (if any) are applied. Any setting
+# in %extra_settings with a value that is undefined is deleted; the remainder are
+# set. Then the PATH and (DY)LD_LIBRARY_PATH are adjusted if the node's install path
+# is set, and the copy environment is returned.
+# #
+# The install path set in get_new_node needs to be a directory containing
+# bin and lib subdirectories as in a standard PostgreSQL installation, so this
+# can't be used with installations where the bin and lib directories don't have
+# a common parent directory.
+sub _get_install_env
+{
+ my $self = shift;
+ my %inst_env = (%ENV, PGHOST => $self->{_host}, PGPORT => $self->{_port});
+ # the remaining arguments are modifications to make to the environment
+ my %mods = (@_);
+ while (my ($k,$v) = each %mods)
+ {
+ if (defined $v)
+ {
+ $inst_env{$k} = "$v";
+ }
+ else
+ {
+ delete $inst_env{$k};
+ }
+ }
+ # now fix up the new environment for the install path
+ my $inst = $self->{_install_path};
+ if ($inst)
+ {
+ if ($TestLib::windows_os)
+ {
+ # Windows picks up DLLs from the PATH rather than *LD_LIBRARY_PATH
+ # choose the right path separator
+ if ($Config{osname} eq 'MSWin32')
+ {
+ $inst_env{PATH} = "$inst/bin;$inst/lib;$ENV{PATH}";
+ }
+ else
+ {
+ $inst_env{PATH} = "$inst/bin:$inst/lib:$ENV{PATH}";
+ }
+ }
+ else
+ {
+ my $dylib_name =
+ $Config{osname} eq 'darwin' ? "DYLD_LIBRARY_PATH" : "LD_LIBRARY_PATH";
+ $inst_env{PATH} = "$inst/bin:$ENV{PATH}";
+ if (exists $ENV{$dylib_name})
+ {
+ $inst_env{$dylib_name} = "$inst/lib:$ENV{$dylib_name}";
+ }
+ else
+ {
+ $inst_env{$dylib_name} = "$inst/lib";
+ }
+ }
+ }
+ return (%inst_env);
+}
+
=pod
=item get_free_port()
@@ -1330,6 +1425,8 @@ sub safe_psql
{
my ($self, $dbname, $sql, %params) = @_;
+ local %ENV = $self->_get_install_env();
+
my ($stdout, $stderr);
my $ret = $self->psql(
@@ -1441,6 +1538,8 @@ sub psql
{
my ($self, $dbname, $sql, %params) = @_;
+ local %ENV = $self->_get_install_env();
+
my $stdout = $params{stdout};
my $stderr = $params{stderr};
my $replication = $params{replication};
@@ -1634,6 +1733,8 @@ sub background_psql
{
my ($self, $dbname, $stdin, $stdout, $timer, %params) = @_;
+ local %ENV = $self->_get_install_env();
+
my $replication = $params{replication};
my @psql_params = (
@@ -1712,6 +1813,8 @@ sub interactive_psql
{
my ($self, $dbname, $stdin, $stdout, $timer, %params) = @_;
+ local %ENV = $self->_get_install_env();
+
my @psql_params = ('psql', '-XAt', '-d', $self->connstr($dbname));
push @psql_params, @{ $params{extra_params} }
@@ -1755,6 +1858,8 @@ sub poll_query_until
{
my ($self, $dbname, $query, $expected) = @_;
+ local %ENV = $self->_get_install_env();
+
$expected = 't' unless defined($expected); # default value
my $cmd = [ 'psql', '-XAt', '-c', $query, '-d', $self->connstr($dbname) ];
@@ -1810,8 +1915,7 @@ sub command_ok
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env();
TestLib::command_ok(@_);
return;
@@ -1831,8 +1935,7 @@ sub command_fails
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env();
TestLib::command_fails(@_);
return;
@@ -1852,8 +1955,7 @@ sub command_like
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env();
TestLib::command_like(@_);
return;
@@ -1874,8 +1976,7 @@ sub command_checks_all
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env();
TestLib::command_checks_all(@_);
return;
@@ -1899,8 +2000,7 @@ sub issues_sql_like
my ($self, $cmd, $expected_sql, $test_name) = @_;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env();
truncate $self->logfile, 0;
my $result = TestLib::run_log($cmd);
@@ -1923,8 +2023,7 @@ sub run_log
{
my $self = shift;
- local $ENV{PGHOST} = $self->host;
- local $ENV{PGPORT} = $self->port;
+ local %ENV = $self->_get_install_env();
TestLib::run_log(@_);
return;
@@ -2174,6 +2273,9 @@ sub pg_recvlogical_upto
{
my ($self, $dbname, $slot_name, $endpos, $timeout_secs, %plugin_options)
= @_;
+
+ local %ENV = $self->_get_install_env();
+
my ($stdout, $stderr);
my $timeout_exception = 'pg_recvlogical timed out';
On 2021-Mar-24, Andrew Dunstan wrote:
OK, like this?
Yeah, looks good!
+# Private routine to return a copy of the environment with the PATH and (DY)LD_LIBRARY_PATH +# correctly set when there is an install path set for the node. +# Routines that call Postgres binaries need to call this routine like this: +# +# local %ENV = $self->_get_install_env{[%extra_settings]); +# +# A copy of the environmnt is taken and node's host and port settings are added +# as PGHOST and PGPORT, Then the extra settings (if any) are applied. Any setting +# in %extra_settings with a value that is undefined is deleted; the remainder are +# set. Then the PATH and (DY)LD_LIBRARY_PATH are adjusted if the node's install path +# is set, and the copy environment is returned.
There's a typo "environmnt" here, and a couple of lines appear
overlength.
+sub _get_install_env
I'd use a name that doesn't have "install" in it -- maybe _get_env or
_get_postgres_env or _get_PostgresNode_env -- but don't really care too
much about it.
+# The install path set in get_new_node needs to be a directory containing +# bin and lib subdirectories as in a standard PostgreSQL installation, so this +# can't be used with installations where the bin and lib directories don't have +# a common parent directory.
I've never heard of an installation where that wasn't true. If there
was a need for that, seems like it'd be possible to set them with
{ bindir => ..., libdir => ...} but I doubt it'll ever be necessary.
--
�lvaro Herrera Valdivia, Chile
"We're here to devour each other alive" (Hobbes)
On Wed, Mar 24, 2021 at 03:33:51PM -0300, Alvaro Herrera wrote:
On 2021-Mar-24, Andrew Dunstan wrote:
+# The install path set in get_new_node needs to be a directory containing +# bin and lib subdirectories as in a standard PostgreSQL installation, so this +# can't be used with installations where the bin and lib directories don't have +# a common parent directory.I've never heard of an installation where that wasn't true. If there
was a need for that, seems like it'd be possible to set them with
{ bindir => ..., libdir => ...} but I doubt it'll ever be necessary.
This would imply an installation with some fancy --bindir or --libdir
specified in ./configure. Never say never, but I also think that what
has been committed is fine. And the result is simple, that's really
cool. So now pg_upgrade's test.sh can be switched to a TAP test.
--
Michael
On 25.03.21 04:47, Michael Paquier wrote:
On Wed, Mar 24, 2021 at 03:33:51PM -0300, Alvaro Herrera wrote:
On 2021-Mar-24, Andrew Dunstan wrote:
+# The install path set in get_new_node needs to be a directory containing +# bin and lib subdirectories as in a standard PostgreSQL installation, so this +# can't be used with installations where the bin and lib directories don't have +# a common parent directory.I've never heard of an installation where that wasn't true. If there
was a need for that, seems like it'd be possible to set them with
{ bindir => ..., libdir => ...} but I doubt it'll ever be necessary.This would imply an installation with some fancy --bindir or --libdir
specified in ./configure. Never say never, but I also think that what
has been committed is fine.
/usr/lib64/? /usr/lib/x86_64-linux-gnu/? Seems pretty common.
On Thu, Mar 25, 2021 at 09:23:22AM +0100, Peter Eisentraut wrote:
On 25.03.21 04:47, Michael Paquier wrote:
This would imply an installation with some fancy --bindir or --libdir
specified in ./configure. Never say never, but I also think that what
has been committed is fine./usr/lib64/? /usr/lib/x86_64-linux-gnu/? Seems pretty common.
As part of the main PostgreSQL package set, yes, things are usually
mixed. Now, when it comes to the handling of conflicts between
multiple major versions, I have yet to see installations that do not
use the same base path for the binaries and libraries, and the PGDG
packages do that with /usr/pgsql-NN/. So, I doubt that we are going
to need this amount of control in reality, but I may be wrong, of
course :)
--
Michael