Обсуждение: 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
Вложения
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
Вложения
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
Вложения
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
Вложения
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
Вложения
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
Вложения
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
Вложения
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