multi-install PostgresNode

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