Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 5/20/21 11:04 PM, Tom Lane wrote:
> Andrew Dunstan  writes:
>> On 5/20/21 9:49 PM, Michael Paquier wrote:
>>> Are older versions of the perl MSI that activestate provides hard to
>>> come by?  FWIW, I would not mind if this README and the docs are
>>> updated to mention that on Windows we require a newer version for this
>>> set of MSIs.
>> I've fixed the coding that led to this particular problem. So for now
>> let's let sleeping dogs lie.
> Seems like the right solution is for somebody to be running a
> buildfarm animal on Windows with an old perl version.
>
>   


Maybe Amit can :-) Getting hold of old builds isn't always easy.
Strawberry's downloads page has versions back to 5.14, ActiveState only
to 5.26.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Amit Kapila]

On Thu, May 20, 2021 at 5:55 PM Andrew Dunstan  wrote:
>
> On 5/20/21 7:15 AM, Michael Paquier wrote:
> > On Thu, May 20, 2021 at 07:05:05AM -0400, Andrew Dunstan wrote:
> >> Your version of perl is apparently too old for this. Looks like that
> >> needs to be 5.22 or later: 
> > Hmm.  src/test/perl/README tells about 5.8.0.  That's quite a jump.
>
> Yes. I've pushed a fix that should take care of the issue.
>

Thanks. It is working now.

-- 
With Regards,
Amit Kapila.

Re: multi-install PostgresNode fails with older postgres versions

[Tom Lane]

Andrew Dunstan  writes:
> On 5/20/21 9:49 PM, Michael Paquier wrote:
>> Are older versions of the perl MSI that activestate provides hard to
>> come by?  FWIW, I would not mind if this README and the docs are
>> updated to mention that on Windows we require a newer version for this
>> set of MSIs.

> I've fixed the coding that led to this particular problem. So for now
> let's let sleeping dogs lie.

Seems like the right solution is for somebody to be running a
buildfarm animal on Windows with an old perl version.

regards, tom lane

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 5/20/21 9:49 PM, Michael Paquier wrote:
> On Thu, May 20, 2021 at 08:25:45AM -0400, Andrew Dunstan wrote:
>> 5.8 is ancient. Yes I know it's what's in the Msys1 DTK, but the DTK
>> perl seems happy with the list form of pipe open - it's only native
>> windows perl's that aren't.
> Respect to the Msys1 DTK for that.
>
>> Maybe it's time to update the requirement a bit, at least for running
>> TAP tests.
> Are older versions of the perl MSI that activestate provides hard to
> come by?  FWIW, I would not mind if this README and the docs are
> updated to mention that on Windows we require a newer version for this
> set of MSIs.


I've fixed the coding that led to this particular problem. So for now
let's let sleeping dogs lie.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Thu, May 20, 2021 at 08:25:45AM -0400, Andrew Dunstan wrote:
> 5.8 is ancient. Yes I know it's what's in the Msys1 DTK, but the DTK
> perl seems happy with the list form of pipe open - it's only native
> windows perl's that aren't.

Respect to the Msys1 DTK for that.

> Maybe it's time to update the requirement a bit, at least for running
> TAP tests.

Are older versions of the perl MSI that activestate provides hard to
come by?  FWIW, I would not mind if this README and the docs are
updated to mention that on Windows we require a newer version for this
set of MSIs.
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 5/20/21 9:53 AM, Tom Lane wrote:
> Michael Paquier  writes:
>> On Thu, May 20, 2021 at 07:05:05AM -0400, Andrew Dunstan wrote:
>>> Your version of perl is apparently too old for this. Looks like that
>>> needs to be 5.22 or later: 
>> Hmm.  src/test/perl/README tells about 5.8.0.  That's quite a jump.
> Something odd about that, because my dinosaurs aren't complaining;
> prairiedog for example uses perl 5.8.3.
>
>   



It was only on Windows that this form of pipe open was not supported.
5.22 fixed that.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Tom Lane]

Michael Paquier  writes:
> On Thu, May 20, 2021 at 07:05:05AM -0400, Andrew Dunstan wrote:
>> Your version of perl is apparently too old for this. Looks like that
>> needs to be 5.22 or later: 

> Hmm.  src/test/perl/README tells about 5.8.0.  That's quite a jump.

Something odd about that, because my dinosaurs aren't complaining;
prairiedog for example uses perl 5.8.3.

regards, tom lane

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 5/20/21 7:15 AM, Michael Paquier wrote:
> On Thu, May 20, 2021 at 07:05:05AM -0400, Andrew Dunstan wrote:
>> Your version of perl is apparently too old for this. Looks like that
>> needs to be 5.22 or later: 
> Hmm.  src/test/perl/README tells about 5.8.0.  That's quite a jump.



Yes. I've pushed a fix that should take care of the issue.


5.8 is ancient. Yes I know it's what's in the Msys1 DTK, but the DTK
perl seems happy with the list form of pipe open - it's only native
windows perl's that aren't.


Maybe it's time to update the requirement a bit, at least for running
TAP tests.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Amit Kapila]

On Thu, May 20, 2021 at 4:43 PM Andrew Dunstan  wrote:
>
> On 5/20/21 7:05 AM, Andrew Dunstan wrote:
> >>
> > Your version of perl is apparently too old for this. Looks like that
> > needs to be 5.22 or later: 
> >
> >
>
> However, we could probably rewrite this in a way that would work with
> your older perl and at the same time not offend perlcritic, using qx{}
> instead of an explicit open.
>
>
> Will test.
>

Okay, thanks. BTW, our docs don't seem to reflect that we need a newer
version of Perl. See [1] (version 5.8.3 or later is required).

[1] - https://www.postgresql.org/docs/devel/install-windows-full.html

-- 
With Regards,
Amit Kapila.

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Thu, May 20, 2021 at 07:05:05AM -0400, Andrew Dunstan wrote:
> Your version of perl is apparently too old for this. Looks like that
> needs to be 5.22 or later: 

Hmm.  src/test/perl/README tells about 5.8.0.  That's quite a jump.
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 5/20/21 7:05 AM, Andrew Dunstan wrote:
> On 5/20/21 4:36 AM, Amit Kapila wrote:
>> On Thu, Apr 22, 2021 at 8:43 PM Andrew Dunstan  wrote:
>>> On 4/22/21 2:52 AM, Michael Paquier wrote:
 On Wed, Apr 21, 2021 at 10:04:40AM -0400, Andrew Dunstan wrote:
> Here's a patch with these things attended to.
 Thanks.  Reading through it, that seems pretty much fine to me.  I
 have not spent time checking _version_cmp in details though :)
>>>
>>>
>>> pushed with a couple of fixes.
>>>
>> In my windows environment (Windows 10), I am not able to successfully
>> execute taptests and the failure indicates the line by this commit
>> (4c4eaf3d  Make PostgresNode version aware). I am trying to execute
>> tests with command: vcregress.bat taptest src/test/subscription
>>
>> I am seeing below in the log file:
>> Log file: 
>> D:/WorkSpace/postgresql/src/test/subscription/tmp_check/log/001_rep_changes_publisher.log
>> List form of pipe open not implemented at
>> D:/WorkSpace/postgresql/src/test/perl/PostgresNode.pm line 1251.
>> # Looks like your test exited with 255 before it could output anything.
>>
>> Can you please let me know if I need to do something additional here?
>>
>>
>>
> Your version of perl is apparently too old for this. Looks like that
> needs to be 5.22 or later: 
>
>

However, we could probably rewrite this in a way that would work with
your older perl and at the same time not offend perlcritic, using qx{}
instead of an explicit open.


Will test.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 5/20/21 4:36 AM, Amit Kapila wrote:
> On Thu, Apr 22, 2021 at 8:43 PM Andrew Dunstan  wrote:
>> On 4/22/21 2:52 AM, Michael Paquier wrote:
>>> On Wed, Apr 21, 2021 at 10:04:40AM -0400, Andrew Dunstan wrote:
 Here's a patch with these things attended to.
>>> Thanks.  Reading through it, that seems pretty much fine to me.  I
>>> have not spent time checking _version_cmp in details though :)
>>
>>
>>
>> pushed with a couple of fixes.
>>
> In my windows environment (Windows 10), I am not able to successfully
> execute taptests and the failure indicates the line by this commit
> (4c4eaf3d  Make PostgresNode version aware). I am trying to execute
> tests with command: vcregress.bat taptest src/test/subscription
>
> I am seeing below in the log file:
> Log file: 
> D:/WorkSpace/postgresql/src/test/subscription/tmp_check/log/001_rep_changes_publisher.log
> List form of pipe open not implemented at
> D:/WorkSpace/postgresql/src/test/perl/PostgresNode.pm line 1251.
> # Looks like your test exited with 255 before it could output anything.
>
> Can you please let me know if I need to do something additional here?
>
>
>

Your version of perl is apparently too old for this. Looks like that
needs to be 5.22 or later: 


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Amit Kapila]

On Thu, Apr 22, 2021 at 8:43 PM Andrew Dunstan  wrote:
>
> On 4/22/21 2:52 AM, Michael Paquier wrote:
> > On Wed, Apr 21, 2021 at 10:04:40AM -0400, Andrew Dunstan wrote:
> >> Here's a patch with these things attended to.
> > Thanks.  Reading through it, that seems pretty much fine to me.  I
> > have not spent time checking _version_cmp in details though :)
>
>
>
>
> pushed with a couple of fixes.
>

In my windows environment (Windows 10), I am not able to successfully
execute taptests and the failure indicates the line by this commit
(4c4eaf3d  Make PostgresNode version aware). I am trying to execute
tests with command: vcregress.bat taptest src/test/subscription

I am seeing below in the log file:
Log file: 
D:/WorkSpace/postgresql/src/test/subscription/tmp_check/log/001_rep_changes_publisher.log
List form of pipe open not implemented at
D:/WorkSpace/postgresql/src/test/perl/PostgresNode.pm line 1251.
# Looks like your test exited with 255 before it could output anything.

Can you please let me know if I need to do something additional here?



--
With Regards,
Amit Kapila.

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/24/21 1:54 AM, Michael Paquier wrote:
> On Fri, Apr 23, 2021 at 08:10:01AM -0400, Andrew Dunstan wrote:
>> Yeah, I think it's ok for comparison purposes just to lump them all
>> together. Here's a patch that does that and some consequent cleanup.
>> Note we now cache the string rather than trying to reconstruct it.
> No objections from here to build the version string beforehand.  
>
>> +  (devel|(?:alpha|beta|rc)\d+)?   # dev marker - see 
>> version_stamp.pl
>> + !x);
> I have been playing with patch and version_stamp.pl, and that does the
> job.  Nice.


Thanks, pushed.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Fri, Apr 23, 2021 at 08:10:01AM -0400, Andrew Dunstan wrote:
> Yeah, I think it's ok for comparison purposes just to lump them all
> together. Here's a patch that does that and some consequent cleanup.
> Note we now cache the string rather than trying to reconstruct it.

No objections from here to build the version string beforehand.  

> +  (devel|(?:alpha|beta|rc)\d+)?   # dev marker - see version_stamp.pl
> +  !x);

I have been playing with patch and version_stamp.pl, and that does the
job.  Nice.
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/23/21 12:41 AM, Michael Paquier wrote:
> On Thu, Apr 22, 2021 at 08:43:10PM -0400, Andrew Dunstan wrote:
>> Interesting point. Maybe we need to do something like devel = -4, alpha
>> = -3, beta = -2, rc = -1. Or maybe that's overkill.
> And after that it would come to how many betas, alphas or RCs you
> have, but you can never be sure of how many of each you may finish
> with.  I think that you have the right answer with just marking all
> of them with -1 for the minor number, keeping the code a maximum
> simple.


Yeah, I think it's ok for comparison purposes just to lump them all
together. Here's a patch that does that and some consequent cleanup.
Note we now cache the string rather than trying to reconstruct it.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresVersion.pm b/src/test/perl/PostgresVersion.pm
index 3f3744ccfa..7ce9e62b79 100644
--- a/src/test/perl/PostgresVersion.pm
+++ b/src/test/perl/PostgresVersion.pm
@@ -79,18 +79,24 @@ sub new
 	# postgres command line tool
 	my $devel;
 	($arg,$devel) = ($1, $2)
-	  if ($arg =~  m/^(?:\(?PostgreSQL\)? )?(\d+(?:\.\d+)*)(devel)?/);
+	  if ($arg =~
+		  m!^ # beginning of line
+  (?:\(?PostgreSQL\)?\s)? # ignore PostgreSQL marker
+  (\d+(?:\.\d+)*) # version number, dotted notation
+  (devel|(?:alpha|beta|rc)\d+)?   # dev marker - see version_stamp.pl
+		 !x);
 
 	# Split into an array
-	my @result = split(/\./, $arg);
+	my @numbers = split(/\./, $arg);
 
 	# Treat development versions as having a minor/micro version one less than
 	# the first released version of that branch.
-	push @result, -1 if ($devel);
+	push @numbers, -1 if ($devel);
 
-	return bless \@result, $class;
-}
+	$devel ||= "";
 
+	return bless  { str => "$arg$devel", num => \@numbers }, $class;
+}
 
 # Routine which compares the _pg_version_array obtained for the two
 # arguments and returns -1, 0, or 1, allowing comparison between two
@@ -108,27 +114,21 @@ sub _version_cmp
 
 	$b = __PACKAGE__->new($b) unless blessed($b);
 
+	my ($an, $bn) = ($a->{num}, $b->{num});
+
 	for (my $idx = 0;; $idx++)
 	{
-		return 0 unless (defined $a->[$idx] && defined $b->[$idx]);
-		return $a->[$idx] <=> $b->[$idx]
-		  if ($a->[$idx] <=> $b->[$idx]);
+		return 0 unless (defined $an->[$idx] && defined $bn->[$idx]);
+		return $an->[$idx] <=> $bn->[$idx]
+		  if ($an->[$idx] <=> $bn->[$idx]);
 	}
 }
 
-# Render the version number in the standard "joined by dots" notation if
-# interpolated into a string. Put back 'devel' if we previously turned it
-# into a -1.
+# Render the version number using the saved string.
 sub _stringify
 {
 	my $self = shift;
-	my @sections = @$self;
-	if ($sections[-1] == -1)
-	{
-		pop @sections;
-		$sections[-1] = "$sections[-1]devel";
-	}
-	return join('.', @sections);
+	return $self->{str};
 }
 
 1;

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Thu, Apr 22, 2021 at 08:43:10PM -0400, Andrew Dunstan wrote:
> Interesting point. Maybe we need to do something like devel = -4, alpha
> = -3, beta = -2, rc = -1. Or maybe that's overkill.

And after that it would come to how many betas, alphas or RCs you
have, but you can never be sure of how many of each you may finish
with.  I think that you have the right answer with just marking all
of them with -1 for the minor number, keeping the code a maximum
simple.
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/22/21 5:08 PM, Michael Paquier wrote:
> On Thu, Apr 22, 2021 at 02:35:13PM -0400, Alvaro Herrera wrote:
>> WFM, thanks :-)
> Also, do we need to worry about beta releases?  Just recalled that
> now.


Interesting point. Maybe we need to do something like devel = -4, alpha
= -3, beta = -2, rc = -1. Or maybe that's overkill.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Thu, Apr 22, 2021 at 02:35:13PM -0400, Alvaro Herrera wrote:
> WFM, thanks :-)

Also, do we need to worry about beta releases?  Just recalled that
now.
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-22, Andrew Dunstan wrote:

>     # Accept standard formats, in case caller has handed us the
> output of a
>     # postgres command line tool
>     my $devel;
>     ($arg,$devel) = ($1, $2)
>       if ($arg =~  m/^(?:\(?PostgreSQL\)? )?(\d+(?:\.\d+)*)(devel)?/);
> 
>     # Split into an array
>     my @result = split(/\./, $arg);
> 
>     # Treat development versions as having a minor/micro version one
> less than
>     # the first released version of that branch.
>     push @result, -1 if ($devel);
> 
>     return bless \@result, $class;

WFM, thanks :-)

-- 
Álvaro Herrera39°49'30"S 73°17'W

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/22/21 11:09 AM, Alvaro Herrera wrote:
> On 2021-Apr-21, Andrew Dunstan wrote:
>
>> +=head1 DESCRIPTION
>> +
>> +PostgresVersion encapsulated Postgres version numbers, providing parsing
>> +of common version formats and comparison operations.
> Small typo here: should be "encapsulates"
>
>> +# Accept standard formats, in case caller has handed us the output of a
>> +# postgres command line tool
>> +$arg = $1
>> +if ($arg =~ m/\(?PostgreSQL\)? (\d+(?:\.\d+)*(?:devel)?)/);
>> +
>> +# Split into an array
>> +my @result = split(/\./, $arg);
>> +
>> +# Treat development versions as having a minor/micro version one less 
>> than
>> +# the first released version of that branch.
>> +if ($result[$#result] =~ m/^(\d+)devel$/)
>> +{
>> +pop(@result);
>> +push(@result, $1, -1);
>> +}
> It's a bit weird to parse the "devel" bit twice.  Would it work to leave
> (?:devel)? out of the capturing parens that becomes $1 in the first
> regex and make it capturing itself, so you get "devel" in $2, and decide
> based on its presence/absence?  Then you don't have to pop and push a -1.


How about this?


    # Accept standard formats, in case caller has handed us the
output of a
    # postgres command line tool
    my $devel;
    ($arg,$devel) = ($1, $2)
      if ($arg =~  m/^(?:\(?PostgreSQL\)? )?(\d+(?:\.\d+)*)(devel)?/);

    # Split into an array
    my @result = split(/\./, $arg);

    # Treat development versions as having a minor/micro version one
less than
    # the first released version of that branch.
    push @result, -1 if ($devel);

    return bless \@result, $class;


cheers


andrew






-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/22/21 11:46 AM, Mark Dilger wrote:
>
>> On Apr 22, 2021, at 8:09 AM, Alvaro Herrera  wrote:
>>
>>> +   # Accept standard formats, in case caller has handed us the output of a
>>> +   # postgres command line tool
>>> +   $arg = $1
>>> +   if ($arg =~ m/\(?PostgreSQL\)? (\d+(?:\.\d+)*(?:devel)?)/);
>>> +
>>> +   # Split into an array
>>> +   my @result = split(/\./, $arg);
>>> +
>>> +   # Treat development versions as having a minor/micro version one less 
>>> than
>>> +   # the first released version of that branch.
>>> +   if ($result[$#result] =~ m/^(\d+)devel$/)
>>> +   {
>>> +   pop(@result);
>>> +   push(@result, $1, -1);
>>> +   }
>> It's a bit weird to parse the "devel" bit twice.  Would it work to leave
>> (?:devel)? out of the capturing parens that becomes $1 in the first
>> regex and make it capturing itself, so you get "devel" in $2, and decide
>> based on its presence/absence?  Then you don't have to pop and push a -1.
> The first regex should match things like "12", "12.1", "14devel", or those 
> same things prefixed with "(PostgreSQL) ", and strip off the "(PostgreSQL)" 
> part if it exists.  But the code should also BAIL_OUT if the regex completely 
> fails to match.
>


Not quite. PostgresVersion doesn't know about Test::More. It could die
(or croak) and we could catch it in an eval.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 22, 2021, at 8:09 AM, Alvaro Herrera  wrote:
> 
>> 
>> +# Accept standard formats, in case caller has handed us the output of a
>> +# postgres command line tool
>> +$arg = $1
>> +if ($arg =~ m/\(?PostgreSQL\)? (\d+(?:\.\d+)*(?:devel)?)/);
>> +
>> +# Split into an array
>> +my @result = split(/\./, $arg);
>> +
>> +# Treat development versions as having a minor/micro version one less 
>> than
>> +# the first released version of that branch.
>> +if ($result[$#result] =~ m/^(\d+)devel$/)
>> +{
>> +pop(@result);
>> +push(@result, $1, -1);
>> +}
> 
> It's a bit weird to parse the "devel" bit twice.  Would it work to leave
> (?:devel)? out of the capturing parens that becomes $1 in the first
> regex and make it capturing itself, so you get "devel" in $2, and decide
> based on its presence/absence?  Then you don't have to pop and push a -1.

The first regex should match things like "12", "12.1", "14devel", or those same 
things prefixed with "(PostgreSQL) ", and strip off the "(PostgreSQL)" part if 
it exists.  But the code should also BAIL_OUT if the regex completely fails to 
match.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/22/21 2:52 AM, Michael Paquier wrote:
> On Wed, Apr 21, 2021 at 10:04:40AM -0400, Andrew Dunstan wrote:
>> Here's a patch with these things attended to.
> Thanks.  Reading through it, that seems pretty much fine to me.  I
> have not spent time checking _version_cmp in details though :)




pushed with a couple of fixes.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-21, Andrew Dunstan wrote:

> +=head1 DESCRIPTION
> +
> +PostgresVersion encapsulated Postgres version numbers, providing parsing
> +of common version formats and comparison operations.

Small typo here: should be "encapsulates"

> + # Accept standard formats, in case caller has handed us the output of a
> + # postgres command line tool
> + $arg = $1
> + if ($arg =~ m/\(?PostgreSQL\)? (\d+(?:\.\d+)*(?:devel)?)/);
> +
> + # Split into an array
> + my @result = split(/\./, $arg);
> +
> + # Treat development versions as having a minor/micro version one less 
> than
> + # the first released version of that branch.
> + if ($result[$#result] =~ m/^(\d+)devel$/)
> + {
> + pop(@result);
> + push(@result, $1, -1);
> + }

It's a bit weird to parse the "devel" bit twice.  Would it work to leave
(?:devel)? out of the capturing parens that becomes $1 in the first
regex and make it capturing itself, so you get "devel" in $2, and decide
based on its presence/absence?  Then you don't have to pop and push a -1.

> + my $res = [ @result ];

Hmm, isn't this just \@result?  So you could do
return bless \@result, $class;

-- 
Álvaro Herrera39°49'30"S 73°17'W

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/22/21 2:52 AM, Michael Paquier wrote:
> On Wed, Apr 21, 2021 at 10:04:40AM -0400, Andrew Dunstan wrote:
>> Here's a patch with these things attended to.
> Thanks.  Reading through it, that seems pretty much fine to me.  I
> have not spent time checking _version_cmp in details though :)


Ok, Thanks.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Wed, Apr 21, 2021 at 10:04:40AM -0400, Andrew Dunstan wrote:
> Here's a patch with these things attended to.

Thanks.  Reading through it, that seems pretty much fine to me.  I
have not spent time checking _version_cmp in details though :)
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/21/21 1:13 AM, Michael Paquier wrote:
> On Tue, Apr 20, 2021 at 01:11:59PM -0400, Andrew Dunstan wrote:
>> Here's the patch for that.
> Thanks.
>
>> +# Accept standard formats, in case caller has handed us the output of a
>> +# postgres command line tool
>> +$arg = $1
>> +if ($arg =~ m/\(?PostgreSQL\)? (\d+(?:\.\d+)*(?:devel)?)/);
> Interesting.  This would work even if using --with-extra-version,
> which is a good thing.
>
>> +# render the version number in the standard "joined by dots" notation if
>> +# interpolated into a string
>> +sub _stringify
>> +{
>> +   my $self = shift;
>> +   return join('.',  @$self);
>> +}
> This comes out a bit strangely when using a devel build as this
> appends -1 as sub-version number, becoming say 14.-1.  It may be
> clearer to add back "devel" in this case?
>
> Wouldn't it be better to add some perldoc to PostgresVersion.pm?




Here's a patch with these things attended to.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index b32223f716..d126c1df9f 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -96,6 +96,7 @@ use File::Spec;
 use File::stat qw(stat);
 use File::Temp ();
 use IPC::Run;
+use PostgresVersion;
 use RecursiveCopy;
 use Socket;
 use Test::More;
@@ -1196,9 +1197,62 @@ sub get_new_node
 	# Add node to list of nodes
 	push(@all_nodes, $node);
 
+	$node->_set_pg_version;
+
+	my $v = $node->{_pg_version};
+
+	carp("PostgresNode isn't fully compatible with version " . $v)
+	  if $v < 12;
+
 	return $node;
 }
 
+# Private routine to run the pg_config binary found in our environment (or in
+# our install_path, if we have one), and set the version from it
+#
+sub _set_pg_version
+{
+my ($self) = @_;
+my $inst = $self->{_install_path};
+my $pg_config = "pg_config";
+
+if (defined $inst)
+{
+# If the _install_path is invalid, our PATH variables might find an
+# unrelated pg_config executable elsewhere.  Sanity check the
+# directory.
+BAIL_OUT("directory not found: $inst")
+unless -d $inst;
+
+# If the directory exists but is not the root of a postgresql
+# installation, or if the user configured using
+# --bindir=$SOMEWHERE_ELSE, we're not going to find pg_config, so
+# complain about that, too.
+$pg_config = "$inst/bin/pg_config";
+BAIL_OUT("pg_config not found: $pg_config")
+unless -e $pg_config;
+BAIL_OUT("pg_config not executable: $pg_config")
+unless -x $pg_config;
+
+# Leave $pg_config install_path qualified, to be sure we get the right
+# version information, below, or die trying
+}
+
+local %ENV = $self->_get_env();
+
+# We only want the version field
+open my $fh, "-|", $pg_config, "--version"
+or
+BAIL_OUT("$pg_config failed: $!");
+my $version_line = <$fh>;
+close $fh or die;
+
+$self->{_pg_version} = PostgresVersion->new($version_line);
+
+BAIL_OUT("could not parse pg_config --version output: $version_line")
+	  unless defined $self->{_pg_version};
+}
+
 # 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.
diff --git a/src/test/perl/PostgresVersion.pm b/src/test/perl/PostgresVersion.pm
new file mode 100644
index 00..9bb0b667d2
--- /dev/null
+++ b/src/test/perl/PostgresVersion.pm
@@ -0,0 +1,138 @@
+
+#
+# PostgresVersion.pm
+#
+# Module encapsulating Postgres Version numbers
+#
+# Copyright (c) 2021, PostgreSQL Global Development Group
+#
+
+
+=pod
+
+=head1 NAME
+
+PostgresVersion - class representing PostgreSQL version numbers
+
+=head1 SYNOPSIS
+
+  use PostgresVersion;
+
+  my $version = PostgresVersion->new($version_arg);
+
+  # compare two versions
+  my $bool = $version1 <= $version2;
+
+  # or compare with a number
+  $bool = $version < 12;
+
+  # or with a string
+  $bool = $version lt "13.1";
+
+  # interpolate in a string
+  my $stringyval = "version: $version";
+
+=head1 DESCRIPTION
+
+PostgresVersion encapsulated Postgres version numbers, providing parsing
+of common version formats and comparison operations.
+
+=cut
+
+
+
+package PostgresVersion;
+
+use strict;
+use warnings;
+
+use Scalar::Util qw(blessed);
+
+use overload
+  '<=>' => \&_version_cmp,
+  'cmp' => \&_version_cmp,
+  '""' => \&_stringify;
+
+=pod
+
+=head1 METHODS
+
+=over
+
+=item PostgresVersion->new($version)
+
+Create a new PostgresVersion instance.
+
+The argument can be a number like 12, or a string like '12.2' or the output
+of a Postgres command like `psql --version` or `pg_config --version`;
+
+=back
+
+=cut
+
+sub new
+{
+	my $class = 

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Tue, Apr 20, 2021 at 01:11:59PM -0400, Andrew Dunstan wrote:
> Here's the patch for that.

Thanks.

> + # Accept standard formats, in case caller has handed us the output of a
> + # postgres command line tool
> + $arg = $1
> + if ($arg =~ m/\(?PostgreSQL\)? (\d+(?:\.\d+)*(?:devel)?)/);

Interesting.  This would work even if using --with-extra-version,
which is a good thing.

> +# render the version number in the standard "joined by dots" notation if
> +# interpolated into a string
> +sub _stringify
> +{
> +   my $self = shift;
> +   return join('.',  @$self);
> +}

This comes out a bit strangely when using a devel build as this
appends -1 as sub-version number, becoming say 14.-1.  It may be
clearer to add back "devel" in this case?

Wouldn't it be better to add some perldoc to PostgresVersion.pm?
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/19/21 12:37 PM, Andrew Dunstan wrote:
> On 4/19/21 10:43 AM, Mark Dilger wrote:
>>> On Apr 19, 2021, at 5:11 AM, Andrew Dunstan  wrote:
>>>
>>> I think therefore I'm inclined for now to do nothing for old version
>>> compatibility.
>> I agree with waiting until the v15 development cycle.
>>
>>> I would commit the fix for the IPC::Run caching glitch,
>>> and version detection
>> Thank you.


I've committed this piece.



>>
>>> I would add a warning if the module is used with
>>> a version <= 11.
>> Sounds fine for now.


Here's the patch for that.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index b32223f716..d126c1df9f 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -96,6 +96,7 @@ use File::Spec;
 use File::stat qw(stat);
 use File::Temp ();
 use IPC::Run;
+use PostgresVersion;
 use RecursiveCopy;
 use Socket;
 use Test::More;
@@ -1196,9 +1197,62 @@ sub get_new_node
 	# Add node to list of nodes
 	push(@all_nodes, $node);
 
+	$node->_set_pg_version;
+
+	my $v = $node->{_pg_version};
+
+	carp("PostgresNode isn't fully compatible with version " . $v)
+	  if $v < 12;
+
 	return $node;
 }
 
+# Private routine to run the pg_config binary found in our environment (or in
+# our install_path, if we have one), and set the version from it
+#
+sub _set_pg_version
+{
+my ($self) = @_;
+my $inst = $self->{_install_path};
+my $pg_config = "pg_config";
+
+if (defined $inst)
+{
+# If the _install_path is invalid, our PATH variables might find an
+# unrelated pg_config executable elsewhere.  Sanity check the
+# directory.
+BAIL_OUT("directory not found: $inst")
+unless -d $inst;
+
+# If the directory exists but is not the root of a postgresql
+# installation, or if the user configured using
+# --bindir=$SOMEWHERE_ELSE, we're not going to find pg_config, so
+# complain about that, too.
+$pg_config = "$inst/bin/pg_config";
+BAIL_OUT("pg_config not found: $pg_config")
+unless -e $pg_config;
+BAIL_OUT("pg_config not executable: $pg_config")
+unless -x $pg_config;
+
+# Leave $pg_config install_path qualified, to be sure we get the right
+# version information, below, or die trying
+}
+
+local %ENV = $self->_get_env();
+
+# We only want the version field
+open my $fh, "-|", $pg_config, "--version"
+or
+BAIL_OUT("$pg_config failed: $!");
+my $version_line = <$fh>;
+close $fh or die;
+
+$self->{_pg_version} = PostgresVersion->new($version_line);
+
+BAIL_OUT("could not parse pg_config --version output: $version_line")
+	  unless defined $self->{_pg_version};
+}
+
 # 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.
diff --git a/src/test/perl/PostgresVersion.pm b/src/test/perl/PostgresVersion.pm
new file mode 100644
index 00..8f1284c61b
--- /dev/null
+++ b/src/test/perl/PostgresVersion.pm
@@ -0,0 +1,82 @@
+
+#
+# PostgresVersion.pm
+#
+# Module encapsulating Postgres Version numbers
+#
+# Copyright (c) 2021, PostgreSQL Global Development Group
+#
+
+package PostgresVersion;
+
+use strict;
+use warnings;
+
+use Scalar::Util qw(blessed);
+
+use overload
+  '<=>' => \&_version_cmp,
+  'cmp' => \&_version_cmp,
+  '""' => \&_stringify;
+
+
+sub new
+{
+	my $class = shift;
+	my $arg = shift;
+
+	# Accept standard formats, in case caller has handed us the output of a
+	# postgres command line tool
+	$arg = $1
+		if ($arg =~ m/\(?PostgreSQL\)? (\d+(?:\.\d+)*(?:devel)?)/);
+
+	# Split into an array
+	my @result = split(/\./, $arg);
+
+	# Treat development versions as having a minor/micro version one less than
+	# the first released version of that branch.
+	if ($result[$#result] =~ m/^(\d+)devel$/)
+	{
+		pop(@result);
+		push(@result, $1, -1);
+	}
+
+	my $res = [ @result ];
+	bless $res, $class;
+	return $res;
+}
+
+
+# Routine which compares the _pg_version_array obtained for the two
+# arguments and returns -1, 0, or 1, allowing comparison between two
+# PostgresNodes or a PostgresNode and a version string.
+#
+# If the second argument is not a blessed object we call the constructor
+# to make one.
+#
+# Because we're overloading '<=>' and 'cmp' this function supplies us with
+# all the comparison operators ('<' and friends, 'gt' and friends)
+#
+sub _version_cmp
+{
+	my ($a, $b) = @_;
+
+	$b = __PACKAGE__->new($b) unless blessed($b);
+
+	for (my $idx = 0; ; $idx++)
+	{
+		return 0 unless (defined $a->[$idx] && defined $b->[$idx]);
+		return $a->[$idx] <=> $b->[$idx]
+			if ($a->[$idx] <=> $b->[$idx]);
+	}
+}
+
+# render the 

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 19, 2021, at 10:50 AM, Jehan-Guillaume de Rorthais  
> wrote:
> 
>> The community needs a single shared PostgresNode implementation that can be
>> used by scripts which reproduce bugs.
> 
> Which means it could be OK to have a PostgresNode implementation, leaving in
> core source-tree, which supports broader needs than the core ones (older
> versions and some more methods)? Did I understood correctly?

Yes, I believe it should be in core.

I don't know about "some more methods", as it depends which methods you are 
proposing.

> If this is correct, I suppose this effort could be committed early in v15 
> cycle?

I don't care to speculate on that yet.

> Does it deserve some effort to build some dedicated TAP tests for these
> modules? I already have a small patch for this waiting on my disk for some 
> more
> tests and review...

I did that, too, in the 0002 version of my patch.  Perhaps we need to merge 
your work and mine.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Mon, 19 Apr 2021 10:35:39 -0700
Mark Dilger  wrote:

> > On Apr 19, 2021, at 10:25 AM, Jehan-Guillaume de Rorthais 
> > wrote:
> > 
> > On Mon, 19 Apr 2021 12:37:08 -0400
> > Andrew Dunstan  wrote:
> >   
> >> 
> >> On 4/19/21 10:43 AM, Mark Dilger wrote:  
> >>>   
>  On Apr 19, 2021, at 5:11 AM, Andrew Dunstan  wrote:
>  
>  I think therefore I'm inclined for now to do nothing for old version
>  compatibility.  
> >>> I agree with waiting until the v15 development cycle.
> >>>   
>  I would commit the fix for the IPC::Run caching glitch,
>  and version detection  
> >>> Thank you.
> >>>   
>  I would add a warning if the module is used with
>  a version <= 11.  
> >>> Sounds fine for now.
> >>>   
>  The original goal of these changes was to allow testing of combinations
>  of different builds with openssl and nss, which doesn't involve old
>  version compatibility.  
> >>> Hmm.  I think different folks had different goals.  My personal interest
> >>> is to write automated tests which spin up older servers, create data that
> >>> cannot be created on newer servers (such as heap tuples with HEAP_MOVED_IN
> >>> or HEAP_MOVED_OFF bits set), upgrade, and test that new code handles the
> >>> old data correctly.  I think this is not only useful for our test suites
> >>> as a community, but is also useful for companies providing support
> >>> services who need to reproduce problems that customers are having on
> >>> clusters that have been pg_upgraded across large numbers of postgres
> >>> versions. 
>  As far as I know, without any compatibility changes the module is fully
>  compatible with releases 13 and 12, and with releases 11 and 10 so long
>  as you don't want a standby, and with releases 9.6 and 9.5 if you also
>  don't want a backup. That makes it suitable for a lot of testing without
>  any attempt at version compatibility.
>  
>  We can revisit compatibility further in the next release.  
> >>> Sounds good.  
> >> 
> >> 
> >> I'll work on this. Meanwhile FTR here's my latest revision - it's a lot
> >> less invasive of the main module, so it seems much more palatable to me,
> >> and still passes my test down to 7.2.  
> > 
> > I spend a fair bit of time to wonder how useful it could be to either
> > maintain such a module in core, including for external needs, or creating a
> > separate external project with a different release/distribution/packaging
> > policy.
> > 
> > Wherever the module is maintained, the goal would be to address broader
> > needs, eg. adding a switch_wal() method or wait_for_archive(), supporting
> > replication, backups, etc for multi-old-deprecated-PostgreSQL versions.
> >
> > To be honest I have mixed feelings. I feel this burden shouldn't be carried
> > by the core, which has restricted needs compared to external projects. In
> > the opposite, maintaining an external project which shares 90% of the code
> > seems to be a useless duplicate and backport effort. Moreover Craig Ringer
> > already opened the door for external use of PostgresNode with his effort to
> > install/package it, see:
> > https://www.postgresql.org/message-id/CAGRY4nxxKSFJEgVAv5YAk%3DbqULtFmNw7gEJef0CCgzpNy6O%3D-w%40mail.gmail.com
> > 
> > Thoughts?  
> 
> The community needs a single shared PostgresNode implementation that can be
> used by scripts which reproduce bugs.

Which means it could be OK to have a PostgresNode implementation, leaving in
core source-tree, which supports broader needs than the core ones (older
versions and some more methods)? Did I understood correctly?

If this is correct, I suppose this effort could be committed early in v15 cycle?

Does it deserve some effort to build some dedicated TAP tests for these
modules? I already have a small patch for this waiting on my disk for some more
tests and review...

Regards

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 19, 2021, at 10:25 AM, Jehan-Guillaume de Rorthais  
> wrote:
> 
> On Mon, 19 Apr 2021 12:37:08 -0400
> Andrew Dunstan  wrote:
> 
>> 
>> On 4/19/21 10:43 AM, Mark Dilger wrote:
>>> 
 On Apr 19, 2021, at 5:11 AM, Andrew Dunstan  wrote:
 
 I think therefore I'm inclined for now to do nothing for old version
 compatibility.
>>> I agree with waiting until the v15 development cycle.
>>> 
 I would commit the fix for the IPC::Run caching glitch,
 and version detection
>>> Thank you.
>>> 
 I would add a warning if the module is used with
 a version <= 11.
>>> Sounds fine for now.
>>> 
 The original goal of these changes was to allow testing of combinations
 of different builds with openssl and nss, which doesn't involve old
 version compatibility.
>>> Hmm.  I think different folks had different goals.  My personal interest is
>>> to write automated tests which spin up older servers, create data that
>>> cannot be created on newer servers (such as heap tuples with HEAP_MOVED_IN
>>> or HEAP_MOVED_OFF bits set), upgrade, and test that new code handles the
>>> old data correctly.  I think this is not only useful for our test suites as
>>> a community, but is also useful for companies providing support services
>>> who need to reproduce problems that customers are having on clusters that
>>> have been pg_upgraded across large numbers of postgres versions.
>>> 
 As far as I know, without any compatibility changes the module is fully
 compatible with releases 13 and 12, and with releases 11 and 10 so long
 as you don't want a standby, and with releases 9.6 and 9.5 if you also
 don't want a backup. That makes it suitable for a lot of testing without
 any attempt at version compatibility.
 
 We can revisit compatibility further in the next release.
>>> Sounds good.
>> 
>> 
>> I'll work on this. Meanwhile FTR here's my latest revision - it's a lot
>> less invasive of the main module, so it seems much more palatable to me,
>> and still passes my test down to 7.2.
> 
> I spend a fair bit of time to wonder how useful it could be to either maintain
> such a module in core, including for external needs, or creating a separate
> external project with a different release/distribution/packaging policy.
> 
> Wherever the module is maintained, the goal would be to address broader
> needs, eg. adding a switch_wal() method or wait_for_archive(), supporting
> replication, backups, etc for multi-old-deprecated-PostgreSQL versions.
> 
> To be honest I have mixed feelings. I feel this burden shouldn't be carried
> by the core, which has restricted needs compared to external projects. In the
> opposite, maintaining an external project which shares 90% of the code seems 
> to
> be a useless duplicate and backport effort. Moreover Craig Ringer already 
> opened
> the door for external use of PostgresNode with his effort to install/package 
> it,
> see:
> https://www.postgresql.org/message-id/CAGRY4nxxKSFJEgVAv5YAk%3DbqULtFmNw7gEJef0CCgzpNy6O%3D-w%40mail.gmail.com
> 
> Thoughts?

The community needs a single shared PostgresNode implementation that can be 
used by scripts which reproduce bugs.  For bugs that can only be triggered by 
cross version upgrades, the scripts need a PostgresNode implementation which 
can work across versions.  Likewise for bugs that can only be triggered when 
client applications connect to servers of a different version.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Mon, 19 Apr 2021 12:37:08 -0400
Andrew Dunstan  wrote:

> 
> On 4/19/21 10:43 AM, Mark Dilger wrote:
> >
> >> On Apr 19, 2021, at 5:11 AM, Andrew Dunstan  wrote:
> >>
> >> I think therefore I'm inclined for now to do nothing for old version
> >> compatibility.
> > I agree with waiting until the v15 development cycle.
> >
> >> I would commit the fix for the IPC::Run caching glitch,
> >> and version detection
> > Thank you.
> >
> >> I would add a warning if the module is used with
> >> a version <= 11.
> > Sounds fine for now.
> >
> >> The original goal of these changes was to allow testing of combinations
> >> of different builds with openssl and nss, which doesn't involve old
> >> version compatibility.
> > Hmm.  I think different folks had different goals.  My personal interest is
> > to write automated tests which spin up older servers, create data that
> > cannot be created on newer servers (such as heap tuples with HEAP_MOVED_IN
> > or HEAP_MOVED_OFF bits set), upgrade, and test that new code handles the
> > old data correctly.  I think this is not only useful for our test suites as
> > a community, but is also useful for companies providing support services
> > who need to reproduce problems that customers are having on clusters that
> > have been pg_upgraded across large numbers of postgres versions.
> >
> >> As far as I know, without any compatibility changes the module is fully
> >> compatible with releases 13 and 12, and with releases 11 and 10 so long
> >> as you don't want a standby, and with releases 9.6 and 9.5 if you also
> >> don't want a backup. That makes it suitable for a lot of testing without
> >> any attempt at version compatibility.
> >>
> >> We can revisit compatibility further in the next release.
> > Sounds good.
> 
> 
> I'll work on this. Meanwhile FTR here's my latest revision - it's a lot
> less invasive of the main module, so it seems much more palatable to me,
> and still passes my test down to 7.2.

I spend a fair bit of time to wonder how useful it could be to either maintain
such a module in core, including for external needs, or creating a separate
external project with a different release/distribution/packaging policy.

Wherever the module is maintained, the goal would be to address broader
needs, eg. adding a switch_wal() method or wait_for_archive(), supporting
replication, backups, etc for multi-old-deprecated-PostgreSQL versions.

To be honest I have mixed feelings. I feel this burden shouldn't be carried
by the core, which has restricted needs compared to external projects. In the
opposite, maintaining an external project which shares 90% of the code seems to
be a useless duplicate and backport effort. Moreover Craig Ringer already opened
the door for external use of PostgresNode with his effort to install/package it,
see:
https://www.postgresql.org/message-id/CAGRY4nxxKSFJEgVAv5YAk%3DbqULtFmNw7gEJef0CCgzpNy6O%3D-w%40mail.gmail.com

Thoughts?

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Mon, 19 Apr 2021 07:43:58 -0700
Mark Dilger  wrote:

> > On Apr 19, 2021, at 5:11 AM, Andrew Dunstan  wrote:
> > 
> > I think therefore I'm inclined for now to do nothing for old version
> > compatibility.  
> 
> I agree with waiting until the v15 development cycle.

Agree.

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/19/21 10:43 AM, Mark Dilger wrote:
>
>> On Apr 19, 2021, at 5:11 AM, Andrew Dunstan  wrote:
>>
>> I think therefore I'm inclined for now to do nothing for old version
>> compatibility.
> I agree with waiting until the v15 development cycle.
>
>> I would commit the fix for the IPC::Run caching glitch,
>> and version detection
> Thank you.
>
>> I would add a warning if the module is used with
>> a version <= 11.
> Sounds fine for now.
>
>> The original goal of these changes was to allow testing of combinations
>> of different builds with openssl and nss, which doesn't involve old
>> version compatibility.
> Hmm.  I think different folks had different goals.  My personal interest is 
> to write automated tests which spin up older servers, create data that cannot 
> be created on newer servers (such as heap tuples with HEAP_MOVED_IN or 
> HEAP_MOVED_OFF bits set), upgrade, and test that new code handles the old 
> data correctly.  I think this is not only useful for our test suites as a 
> community, but is also useful for companies providing support services who 
> need to reproduce problems that customers are having on clusters that have 
> been pg_upgraded across large numbers of postgres versions.
>
>> As far as I know, without any compatibility changes the module is fully
>> compatible with releases 13 and 12, and with releases 11 and 10 so long
>> as you don't want a standby, and with releases 9.6 and 9.5 if you also
>> don't want a backup. That makes it suitable for a lot of testing without
>> any attempt at version compatibility.
>>
>> We can revisit compatibility further in the next release.
> Sounds good.


I'll work on this. Meanwhile FTR here's my latest revision - it's a lot
less invasive of the main module, so it seems much more palatable to me,
and still passes my test down to 7.2.


cheers


andrew


-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index e209ea7163..c6086101f2 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -96,6 +96,7 @@ use File::Spec;
 use File::stat qw(stat);
 use File::Temp ();
 use IPC::Run;
+use PostgresVersion;
 use RecursiveCopy;
 use Socket;
 use Test::More;
@@ -127,6 +128,20 @@ INIT
 	$last_port_assigned = int(rand() * 16384) + 49152;
 }
 
+# Current dev version, for which we have no subclass
+# When a new stable branch is made this and the subclass hierarchy below
+# need to be adjusted.
+my $devtip = 14;
+
+INIT
+{
+	# sanity check to make sure there is a subclass for the last stable branch
+	my $last_child = 'PostgresNodeV_' . ($devtip -1);
+	eval "${last_child}->can('get_new_node') || die('not found');";
+	die "No child package $last_child found" if $@;
+}
+
+
 =pod
 
 =head1 METHODS
@@ -347,9 +362,12 @@ about this node.
 sub info
 {
 	my ($self) = @_;
+	my $varr = $self->{_pg_version};
+	my $vstr = join('.', @$varr) if ref $varr;
 	my $_info = '';
 	open my $fh, '>', \$_info or die;
 	print $fh "Name: " . $self->name . "\n";
+	print $fh "Version: " . $vstr . "\n" if $vstr;
 	print $fh "Data directory: " . $self->data_dir . "\n";
 	print $fh "Backup directory: " . $self->backup_dir . "\n";
 	print $fh "Archive directory: " . $self->archive_dir . "\n";
@@ -438,7 +456,7 @@ sub init
 	mkdir $self->backup_dir;
 	mkdir $self->archive_dir;
 
-	TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
+	TestLib::system_or_bail('initdb', '-D', $pgdata, ($self->_initdb_flags),
 		@{ $params{extra} });
 	TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata,
 		@{ $params{auth_extra} });
@@ -465,31 +483,36 @@ sub init
 
 	if ($params{allows_streaming})
 	{
-		if ($params{allows_streaming} eq "logical")
-		{
-			print $conf "wal_level = logical\n";
-		}
-		else
-		{
-			print $conf "wal_level = replica\n";
-		}
-		print $conf "max_wal_senders = 10\n";
-		print $conf "max_replication_slots = 10\n";
-		print $conf "wal_log_hints = on\n";
-		print $conf "hot_standby = on\n";
-		# conservative settings to ensure we can run multiple postmasters:
-		print $conf "shared_buffers = 1MB\n";
-		print $conf "max_connections = 10\n";
-		# limit disk space consumption, too:
-		print $conf "max_wal_size = 128MB\n";
+		$self->_init_streaming($conf, $params{allows_streaming})
 	}
 	else
 	{
-		print $conf "wal_level = minimal\n";
-		print $conf "max_wal_senders = 0\n";
+		$self->_init_wal_level_minimal($conf);
 	}
 
 	print $conf "port = $port\n";
+
+	$self->_init_network($conf, $use_tcp, $host);
+
+	close $conf;
+
+	chmod($self->group_access ? 0640 : 0600, "$pgdata/postgresql.conf")
+	  or die("unable to set permissions for $pgdata/postgresql.conf");
+
+	$self->set_replication_conf if $params{allows_streaming};
+	$self->enable_archiving if $params{has_archiving};
+	return;
+}
+
+
+# methods use in init() which can be overridden in older versions
+
+sub _initdb_flags {	return ('-A', 'trust', '-N'); }
+
+sub _init_network
+{
+	my ($self, 

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 19, 2021, at 5:11 AM, Andrew Dunstan  wrote:
> 
> I think therefore I'm inclined for now to do nothing for old version
> compatibility.

I agree with waiting until the v15 development cycle.

> I would commit the fix for the IPC::Run caching glitch,
> and version detection

Thank you.

> I would add a warning if the module is used with
> a version <= 11.

Sounds fine for now.

> The original goal of these changes was to allow testing of combinations
> of different builds with openssl and nss, which doesn't involve old
> version compatibility.

Hmm.  I think different folks had different goals.  My personal interest is to 
write automated tests which spin up older servers, create data that cannot be 
created on newer servers (such as heap tuples with HEAP_MOVED_IN or 
HEAP_MOVED_OFF bits set), upgrade, and test that new code handles the old data 
correctly.  I think this is not only useful for our test suites as a community, 
but is also useful for companies providing support services who need to 
reproduce problems that customers are having on clusters that have been 
pg_upgraded across large numbers of postgres versions.

> As far as I know, without any compatibility changes the module is fully
> compatible with releases 13 and 12, and with releases 11 and 10 so long
> as you don't want a standby, and with releases 9.6 and 9.5 if you also
> don't want a backup. That makes it suitable for a lot of testing without
> any attempt at version compatibility.
> 
> We can revisit compatibility further in the next release.

Sounds good.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/19/21 8:32 AM, Michael Paquier wrote:
> On Mon, Apr 19, 2021 at 08:11:01AM -0400, Andrew Dunstan wrote:
>> As far as I know, without any compatibility changes the module is fully
>> compatible with releases 13 and 12, and with releases 11 and 10 so long
>> as you don't want a standby, and with releases 9.6 and 9.5 if you also
>> don't want a backup. That makes it suitable for a lot of testing without
>> any attempt at version compatibility.
>>
>> We can revisit compatibility further in the next release.
> Agreed, and I am not convinced that there is any strong need for any
> of that in the close future either, as long as there are no
> ground-breaking compatibility changes.
>
> How far does the buildfarm test pg_upgrade?  One thing that I
> personally care about here is the possibility to make pg_upgrade's
> test.sh become a TAP test.  However, I am also pretty sure that we
> could apply some local changes to the TAP test of pg_upgrade itself to
> not require any wide changes to PostgresNode.pm either to make the
> central logic as simple as possible with all the stable branches still
> supported or even older ones.  Having compatibility for free down to
> 12 is nice enough IMO for most of the core logic, and pg_upgrade would
> also work just fine down to 9.5 without any extra changes because we
> don't care there about standbys or backups.


The buildfarm tests self-targetted pg_upgrade by calling the builtin
tests (make check / vcregress.pl upgradecheck).


However, for cross version testing the regime is quite different. The
cross version module doesn't ever construct a repo. Rather, it tries to
upgrade a repo saved from a prior run. So all it does is some
adjustments for things that have changed between releases and then calls
pg_upgrade. See



Note that we currently test upgrades down to 9.2 on crake. However, now
I have some working binaries for really old releases I might extend that
all the way back to 8.4 at some stage. pg_upgrade and pg_dump/pg_restore
testing are the major use cases I can see for backwards compatibility -
pg_dump is still supposed to be able to go back into the dim dark ages,
which is why I built the old binaries all the way back to 7.2.





It's just occurred to me that a much nicer way of doing this
PostgresNode stuff would be to have a function that instead of appending
to the config file would adjust it. Then we wouldn't need all those
little settings functions to be overridden - the subclasses could just
post-process the  config files. I'm going to try that and see what I can
come up with. I think it will look heaps nicer.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

-- 
--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Mon, Apr 19, 2021 at 08:11:01AM -0400, Andrew Dunstan wrote:
> As far as I know, without any compatibility changes the module is fully
> compatible with releases 13 and 12, and with releases 11 and 10 so long
> as you don't want a standby, and with releases 9.6 and 9.5 if you also
> don't want a backup. That makes it suitable for a lot of testing without
> any attempt at version compatibility.
> 
> We can revisit compatibility further in the next release.

Agreed, and I am not convinced that there is any strong need for any
of that in the close future either, as long as there are no
ground-breaking compatibility changes.

How far does the buildfarm test pg_upgrade?  One thing that I
personally care about here is the possibility to make pg_upgrade's
test.sh become a TAP test.  However, I am also pretty sure that we
could apply some local changes to the TAP test of pg_upgrade itself to
not require any wide changes to PostgresNode.pm either to make the
central logic as simple as possible with all the stable branches still
supported or even older ones.  Having compatibility for free down to
12 is nice enough IMO for most of the core logic, and pg_upgrade would
also work just fine down to 9.5 without any extra changes because we
don't care there about standbys or backups.
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/17/21 12:35 PM, Andrew Dunstan wrote:
>
>> OK, here is more WIP on this item. I have drawn substantially on Mark's
>> and Jehan-Guillaime's work, but it doesn't really resemble either, and I
>> take full responsibility for it.
>>
>> The guiding principles have been:
>>
>> . avoid doing version tests, or capability tests which are the moral
>> equivalent, and rely instead on pure overloading.
>>
>> . avoid overriding large pieces of code.
>>
>>
>> The last has involved breaking up some large subroutines (like init)
>> into pieces which can more sensibly be overridden. Breaking them up
>> isn't a bad thing to do anyway.
>>
>> There is a new PostgresVersion object, but it's deliberately very
>> minimal. Since we're not doing version tests we don't need more complex
>> routines.
>>
>>
>
> I should have also attached my test program - here it is. Also, I have
> been testing with the binaries which I've published here:
>  along with some saved by my
> buildfarm animal.
>
>

I've been thinking about this some more over the weekend. I'm not really
happy with any of the three approaches to this problem:


a) Use version or capability tests in the main package

b) No changes to main package, use overrides

c) Changes to main package to allow smaller overrides


The problem is that a) and c) involve substantial changes to the main
PostgresNode package, while b) involves overriding large functions (like
init) sometimes for quite trivial changes.

I think therefore I'm inclined for now to do nothing for old version
compatibility. I would commit the fix for the IPC::Run caching glitch,
and version detection. I would add a warning if the module is used with
a version <= 11.

The original goal of these changes was to allow testing of combinations
of different builds with openssl and nss, which doesn't involve old
version compatibility.

As far as I know, without any compatibility changes the module is fully
compatible with releases 13 and 12, and with releases 11 and 10 so long
as you don't want a standby, and with releases 9.6 and 9.5 if you also
don't want a backup. That makes it suitable for a lot of testing without
any attempt at version compatibility.

We can revisit compatibility further in the next release.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/17/21 12:31 PM, Andrew Dunstan wrote:
> On 4/12/21 10:57 AM, Jehan-Guillaume de Rorthais wrote:
>> On Mon, 12 Apr 2021 09:52:24 -0400
>> Andrew Dunstan  wrote:
>>
>>> On 4/12/21 8:59 AM, Jehan-Guillaume de Rorthais wrote:
 Hi,

 On Wed, 7 Apr 2021 20:07:41 +0200
 Jehan-Guillaume de Rorthais  wrote:
 [...]  
>>> Let me know if it worth that I work on an official patch.  
>> Let's give it a try ...
> OK  
 So, as promised, here is my take to port my previous work on PostgreSQL
 source tree.

 Make check pass with no errors. The new class probably deserve some own TAP
 tests.

 Note that I added a PostgresVersion class for easier and cleaner version
 comparisons. This could be an interesting take away no matter what.

 I still have some more ideas to cleanup, revamp and extend the base class,
 but I prefer to go incremental to keep things review-ability.
  
>>> Thanks for this. We have been working somewhat on parallel lines. With
>>> your permission I'm going to take some of what you've done and
>>> incorporate it in the patch I'm working on.
>> The current context makes my weeks difficult to plan and quite chaotic, 
>> that's
>> why it took me some days to produce the patch I promised.
>>
>> I'm fine with working with a common base code, thanks. Feel free to merge 
>> both
>> code, we'll trade patches during review. However, I'm not sure what is the
>> status of your patch, so I can not judge what would be the easier way to
>> incorporate. Mine is tested down to 9.1 (9.0 was meaningless because of lack 
>> of
>> pg_stat_replication) with:
>>
>> * get_new_node
>> * init(allows_streaming => 1)
>> * start
>> * stop
>> * backup
>> * init_from_backup
>> * wait_for_catchup
>> * command_checks_all
>>
>> Note the various changes in my init() and new method allow_streaming(), etc.
>>
>> FYI (to avoid duplicate work), the next step on my todo was to produce some
>> meaningful tap tests to prove the class.
>>
>>> A PostgresVersion class is a good idea - I was already contemplating
>>> something of the kind.
>> Thanks!
>>
>
> OK, here is more WIP on this item. I have drawn substantially on Mark's
> and Jehan-Guillaime's work, but it doesn't really resemble either, and I
> take full responsibility for it.
>
> The guiding principles have been:
>
> . avoid doing version tests, or capability tests which are the moral
> equivalent, and rely instead on pure overloading.
>
> . avoid overriding large pieces of code.
>
>
> The last has involved breaking up some large subroutines (like init)
> into pieces which can more sensibly be overridden. Breaking them up
> isn't a bad thing to do anyway.
>
> There is a new PostgresVersion object, but it's deliberately very
> minimal. Since we're not doing version tests we don't need more complex
> routines.
>
>


I should have also attached my test program - here it is. Also, I have
been testing with the binaries which I've published here:
 along with some saved by my
buildfarm animal.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com



testpgnode.pl
Description: Perl program

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/12/21 10:57 AM, Jehan-Guillaume de Rorthais wrote:
> On Mon, 12 Apr 2021 09:52:24 -0400
> Andrew Dunstan  wrote:
>
>> On 4/12/21 8:59 AM, Jehan-Guillaume de Rorthais wrote:
>>> Hi,
>>>
>>> On Wed, 7 Apr 2021 20:07:41 +0200
>>> Jehan-Guillaume de Rorthais  wrote:
>>> [...]  
>> Let me know if it worth that I work on an official patch.  
> Let's give it a try ...
 OK  
>>> So, as promised, here is my take to port my previous work on PostgreSQL
>>> source tree.
>>>
>>> Make check pass with no errors. The new class probably deserve some own TAP
>>> tests.
>>>
>>> Note that I added a PostgresVersion class for easier and cleaner version
>>> comparisons. This could be an interesting take away no matter what.
>>>
>>> I still have some more ideas to cleanup, revamp and extend the base class,
>>> but I prefer to go incremental to keep things review-ability.
>>>  
>> Thanks for this. We have been working somewhat on parallel lines. With
>> your permission I'm going to take some of what you've done and
>> incorporate it in the patch I'm working on.
> The current context makes my weeks difficult to plan and quite chaotic, that's
> why it took me some days to produce the patch I promised.
>
> I'm fine with working with a common base code, thanks. Feel free to merge both
> code, we'll trade patches during review. However, I'm not sure what is the
> status of your patch, so I can not judge what would be the easier way to
> incorporate. Mine is tested down to 9.1 (9.0 was meaningless because of lack 
> of
> pg_stat_replication) with:
>
> * get_new_node
> * init(allows_streaming => 1)
> * start
> * stop
> * backup
> * init_from_backup
> * wait_for_catchup
> * command_checks_all
>
> Note the various changes in my init() and new method allow_streaming(), etc.
>
> FYI (to avoid duplicate work), the next step on my todo was to produce some
> meaningful tap tests to prove the class.
>
>> A PostgresVersion class is a good idea - I was already contemplating
>> something of the kind.
> Thanks!
>


OK, here is more WIP on this item. I have drawn substantially on Mark's
and Jehan-Guillaime's work, but it doesn't really resemble either, and I
take full responsibility for it.

The guiding principles have been:

. avoid doing version tests, or capability tests which are the moral
equivalent, and rely instead on pure overloading.

. avoid overriding large pieces of code.


The last has involved breaking up some large subroutines (like init)
into pieces which can more sensibly be overridden. Breaking them up
isn't a bad thing to do anyway.

There is a new PostgresVersion object, but it's deliberately very
minimal. Since we're not doing version tests we don't need more complex
routines.


cheers


andrew




--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index e209ea7163..1d0866eea7 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -96,6 +96,7 @@ use File::Spec;
 use File::stat qw(stat);
 use File::Temp ();
 use IPC::Run;
+use PostgresVersion;
 use RecursiveCopy;
 use Socket;
 use Test::More;
@@ -127,6 +128,20 @@ INIT
 	$last_port_assigned = int(rand() * 16384) + 49152;
 }
 
+# Current dev version, for which we have no subclass
+# When a new stable branch is made this and the subclass hierarchy below
+# need to be adjusted.
+my $devtip = 14;
+
+INIT
+{
+	# sanity check to make sure there is a subclass for the last stable branch
+	my $last_child = 'PostgresNodeV_' . ($devtip -1);
+	eval "${last_child}->can('get_new_node') || die('not found');";
+	die "No child package $last_child found" if $@;
+}
+
+
 =pod
 
 =head1 METHODS
@@ -347,9 +362,12 @@ about this node.
 sub info
 {
 	my ($self) = @_;
+	my $varr = $self->{_pg_version};
+	my $vstr = join('.', @$varr) if ref $varr;
 	my $_info = '';
 	open my $fh, '>', \$_info or die;
 	print $fh "Name: " . $self->name . "\n";
+	print $fh "Version: " . $vstr . "\n" if $vstr;
 	print $fh "Data directory: " . $self->data_dir . "\n";
 	print $fh "Backup directory: " . $self->backup_dir . "\n";
 	print $fh "Archive directory: " . $self->archive_dir . "\n";
@@ -438,7 +456,7 @@ sub init
 	mkdir $self->backup_dir;
 	mkdir $self->archive_dir;
 
-	TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
+	TestLib::system_or_bail('initdb', '-D', $pgdata, ($self->_initdb_flags),
 		@{ $params{extra} });
 	TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata,
 		@{ $params{auth_extra} });
@@ -446,11 +464,11 @@ sub init
 	open my $conf, '>>', "$pgdata/postgresql.conf";
 	print $conf "\n# Added by PostgresNode.pm\n";
 	print $conf "fsync = off\n";
-	print $conf "restart_after_crash = off\n";
-	print $conf "log_line_prefix = '%m [%p] %q%a '\n";
-	print $conf "log_statement = all\n";
-	print $conf "log_replication_commands = on\n";
-	print $conf "wal_retrieve_retry_interval = '500ms'\n";
+	print $conf $self->_init_restart_after_crash;
+	print $conf 

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Mon, 12 Apr 2021 09:52:24 -0400
Andrew Dunstan  wrote:

> On 4/12/21 8:59 AM, Jehan-Guillaume de Rorthais wrote:
> > Hi,
> >
> > On Wed, 7 Apr 2021 20:07:41 +0200
> > Jehan-Guillaume de Rorthais  wrote:
> > [...]  
>  Let me know if it worth that I work on an official patch.  
> >>> Let's give it a try ...
> >> OK  
> > So, as promised, here is my take to port my previous work on PostgreSQL
> > source tree.
> >
> > Make check pass with no errors. The new class probably deserve some own TAP
> > tests.
> >
> > Note that I added a PostgresVersion class for easier and cleaner version
> > comparisons. This could be an interesting take away no matter what.
> >
> > I still have some more ideas to cleanup, revamp and extend the base class,
> > but I prefer to go incremental to keep things review-ability.
> >  
> 
> Thanks for this. We have been working somewhat on parallel lines. With
> your permission I'm going to take some of what you've done and
> incorporate it in the patch I'm working on.

The current context makes my weeks difficult to plan and quite chaotic, that's
why it took me some days to produce the patch I promised.

I'm fine with working with a common base code, thanks. Feel free to merge both
code, we'll trade patches during review. However, I'm not sure what is the
status of your patch, so I can not judge what would be the easier way to
incorporate. Mine is tested down to 9.1 (9.0 was meaningless because of lack of
pg_stat_replication) with:

* get_new_node
* init(allows_streaming => 1)
* start
* stop
* backup
* init_from_backup
* wait_for_catchup
* command_checks_all

Note the various changes in my init() and new method allow_streaming(), etc.

FYI (to avoid duplicate work), the next step on my todo was to produce some
meaningful tap tests to prove the class.

> A PostgresVersion class is a good idea - I was already contemplating
> something of the kind.

Thanks!

Regards,

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/12/21 8:59 AM, Jehan-Guillaume de Rorthais wrote:
> Hi,
>
> On Wed, 7 Apr 2021 20:07:41 +0200
> Jehan-Guillaume de Rorthais  wrote:
> [...]
 Let me know if it worth that I work on an official patch.
>>> Let's give it a try ...  
>> OK
> So, as promised, here is my take to port my previous work on PostgreSQL
> source tree.
>
> Make check pass with no errors. The new class probably deserve some own TAP
> tests.
>
> Note that I added a PostgresVersion class for easier and cleaner version
> comparisons. This could be an interesting take away no matter what.
>
> I still have some more ideas to cleanup, revamp and extend the base class, but
> I prefer to go incremental to keep things review-ability.
>

Thanks for this. We have been working somewhat on parallel lines. With
your permission I'm going to take some of what you've done and
incorporate it in the patch I'm working on.


A PostgresVersion class is a good idea - I was already contemplating
something of the kind.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

Hi,

On Wed, 7 Apr 2021 20:07:41 +0200
Jehan-Guillaume de Rorthais  wrote:
[...]
> > > Let me know if it worth that I work on an official patch.
> > 
> > Let's give it a try ...  
> 
> OK

So, as promised, here is my take to port my previous work on PostgreSQL
source tree.

Make check pass with no errors. The new class probably deserve some own TAP
tests.

Note that I added a PostgresVersion class for easier and cleaner version
comparisons. This could be an interesting take away no matter what.

I still have some more ideas to cleanup, revamp and extend the base class, but
I prefer to go incremental to keep things review-ability.

Thanks,
>From 077e447995b3b8bb4c0594a6616e1350acd9d48b Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais 
Date: Mon, 12 Apr 2021 14:45:17 +0200
Subject: [PATCH] Draft: Makes PostgresNode multi-version aware

---
 src/test/perl/PostgresNode.pm| 708 ---
 src/test/perl/PostgresVersion.pm |  65 +++
 2 files changed, 704 insertions(+), 69 deletions(-)
 create mode 100644 src/test/perl/PostgresVersion.pm

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index e26b2b3f30..d7a9e54efd 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -102,6 +102,7 @@ use Test::More;
 use TestLib ();
 use Time::HiRes qw(usleep);
 use Scalar::Util qw(blessed);
+use PostgresVersion;
 
 our @EXPORT = qw(
   get_new_node
@@ -376,9 +377,42 @@ sub dump_info
 	return;
 }
 
+# Internal routine to allows streaming replication on a primary node.
+sub allows_streaming
+{
+	my ($self, $allows_streaming) = @_;
+	my $pgdata = $self->data_dir;
+	my $wal_level;
+
+	if ($allows_streaming eq "logical")
+	{
+		$wal_level = "logical";
+	}
+	else
+	{
+		$wal_level = "replica";
+	}
+
+	$self->append_conf( 'postgresql.conf', qq{
+		wal_level = $wal_level
+		max_wal_senders = 10
+		max_replication_slots = 10
+		wal_log_hints = on
+		hot_standby = on
+		# conservative settings to ensure we can run multiple postmasters:
+		shared_buffers = 1MB
+		max_connections = 10
+		# limit disk space consumption, too:
+		max_wal_size = 128MB
+	});
 
-# Internal method to set up trusted pg_hba.conf for replication.  Not
-# documented because you shouldn't use it, it's called automatically if needed.
+	$self->set_replication_conf;
+
+	return;
+}
+
+# Internal to set up trusted pg_hba.conf for replication.  Not documented
+# because you shouldn't use it, it's called automatically if needed.
 sub set_replication_conf
 {
 	my ($self) = @_;
@@ -398,6 +432,15 @@ sub set_replication_conf
 	return;
 }
 
+# Internal methods to track capabilities depending on the PostgreSQL major
+# version used
+
+sub can_slots   { return 1 }
+sub can_log_hints   { return 1 }
+sub can_restart_after_crash { return 1 }
+sub can_skip_init_fsync { return 1 }
+
+
 =pod
 
 =item $node->init(...)
@@ -429,6 +472,7 @@ sub init
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
 	my $host   = $self->host;
+	my @cmd;
 
 	local %ENV = $self->_get_env();
 
@@ -438,19 +482,25 @@ sub init
 	mkdir $self->backup_dir;
 	mkdir $self->archive_dir;
 
-	TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
-		@{ $params{extra} });
+	@cmd = ( 'initdb', '-D', $pgdata, '-A', 'trust' );
+	push @cmd, '-N' if $self->can_skip_init_fsync;
+	push @cmd, @{ $params{extra} } if defined $params{extra};
+
+	TestLib::system_or_bail(@cmd);
 	TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata,
 		@{ $params{auth_extra} });
 
 	open my $conf, '>>', "$pgdata/postgresql.conf";
 	print $conf "\n# Added by PostgresNode.pm\n";
 	print $conf "fsync = off\n";
-	print $conf "restart_after_crash = off\n";
+	print $conf "restart_after_crash = off\n" if $self->can_restart_after_crash;
 	print $conf "log_line_prefix = '%m [%p] %q%a '\n";
 	print $conf "log_statement = all\n";
-	print $conf "log_replication_commands = on\n";
-	print $conf "wal_retrieve_retry_interval = '500ms'\n";
+	if ($self->version >= '9.5.0')
+	{
+		print $conf "log_replication_commands = on\n";
+		print $conf "wal_retrieve_retry_interval = '500ms'\n";
+	}
 
 	# If a setting tends to affect whether tests pass or fail, print it after
 	# TEMP_CONFIG.  Otherwise, print it before TEMP_CONFIG, thereby permitting
@@ -463,31 +513,8 @@ sub init
 	# concurrently must not share a stats_temp_directory.
 	print $conf "stats_temp_directory = 'pg_stat_tmp'\n";
 
-	if ($params{allows_streaming})
-	{
-		if ($params{allows_streaming} eq "logical")
-		{
-			print $conf "wal_level = logical\n";
-		}
-		else
-		{
-			print $conf "wal_level = replica\n";
-		}
-		print $conf "max_wal_senders = 10\n";
-		print $conf "max_replication_slots = 10\n";
-		print $conf "wal_log_hints = on\n";
-		print $conf "hot_standby = on\n";
-		# conservative settings to ensure we can run multiple postmasters:
-		print $conf "shared_buffers = 1MB\n";
-		print $conf "max_connections = 10\n";
-		# limit disk space consumption, 

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/7/21 5:06 PM, Alvaro Herrera wrote:
> On 2021-Apr-07, Andrew Dunstan wrote:
>
>> Oh, you want to roll them all up into one file? That could work. It's a
>> bit frowned on by perl purists, but I've done similar (see PGBuild/SCM.pm).
> Ah!  Yeah, pretty much exactly like that, including the "no critic" flag ...
>


OK, here's an attempt at that. There is almost certainly more work to
do, but it does pass my basic test (set up a node, start it, talk to it,
shut it down) on some very old versions down as low as 7.2.


Is this is more to your liking?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index e26b2b3f30..e48379e3fd 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -127,6 +127,11 @@ INIT
 	$last_port_assigned = int(rand() * 16384) + 49152;
 }
 
+# Current dev version, for which we have no subclass
+# When a new stable branch is made this and the subclass hierarchy below
+# need to be adjusted.
+my $devtip = 14;
+
 =pod
 
 =head1 METHODS
@@ -347,9 +352,12 @@ about this node.
 sub info
 {
 	my ($self) = @_;
+	my $varr = $self->{_pg_version};
+	my $vstr = join('.', @$varr) if ref $varr;
 	my $_info = '';
 	open my $fh, '>', \$_info or die;
 	print $fh "Name: " . $self->name . "\n";
+	print $fh "Version: " . $vstr . "\n" if $vstr;
 	print $fh "Data directory: " . $self->data_dir . "\n";
 	print $fh "Backup directory: " . $self->backup_dir . "\n";
 	print $fh "Archive directory: " . $self->archive_dir . "\n";
@@ -797,7 +805,7 @@ sub start
 
 	# 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',
+	$ret = TestLib::system_log('pg_ctl', '-w', '-D', $self->data_dir, '-l',
 		$self->logfile, '-o', "--cluster-name=$name", 'start');
 
 	if ($ret != 0)
@@ -911,7 +919,7 @@ sub restart
 
 	print "### Restarting node \"$name\"\n";
 
-	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+	TestLib::system_or_bail('pg_ctl', '-w', '-D', $pgdata, '-l', $logfile,
 		'restart');
 
 	$self->_update_pid(1);
@@ -1196,9 +1204,188 @@ sub get_new_node
 	# Add node to list of nodes
 	push(@all_nodes, $node);
 
+	# Get information about the node
+	$node->_read_pg_config;
+
+	# bless the object into the appropriate subclass,
+	# according to the found version
+	if (ref $node->{_pg_version} && $node->{_pg_version}->[0] <= $devtip )
+	{
+		my $maj = $node->{_pg_version}->[0];
+		my $subclass = __PACKAGE__ . "V_$maj";
+		if ($maj < 10)
+		{
+			$maj = $node->{_pg_version}->[1];
+			$subclass .= "_$maj";
+		}
+		bless $node, $subclass;
+	}
+
 	return $node;
 }
 
+# Private routine to run the pg_config binary found in our environment (or in
+# our install_path, if we have one), and collect all fields that matter to us.
+#
+sub _read_pg_config
+{
+	my ($self) = @_;
+	my $inst = $self->{_install_path};
+	my $pg_config = "pg_config";
+
+	if (defined $inst)
+	{
+		# If the _install_path is invalid, our PATH variables might find an
+		# unrelated pg_config executable elsewhere.  Sanity check the
+		# directory.
+		BAIL_OUT("directory not found: $inst")
+			unless -d $inst;
+
+		# If the directory exists but is not the root of a postgresql
+		# installation, or if the user configured using
+		# --bindir=$SOMEWHERE_ELSE, we're not going to find pg_config, so
+		# complain about that, too.
+		$pg_config = "$inst/bin/pg_config";
+		BAIL_OUT("pg_config not found: $pg_config")
+			unless -e $pg_config;
+		BAIL_OUT("pg_config not executable: $pg_config")
+			unless -x $pg_config;
+
+		# Leave $pg_config install_path qualified, to be sure we get the right
+		# version information, below, or die trying
+	}
+
+	local %ENV = $self->_get_env();
+
+	# We only want the version field
+	open my $fh, "-|", $pg_config, "--version"
+		or
+		BAIL_OUT("$pg_config failed: $!");
+	my $version_line = <$fh>;
+	close $fh or die;
+
+	$self->{_pg_version} = _pg_version_array($version_line);
+
+	BAIL_OUT("could not parse pg_config --version output: $version_line")
+		unless defined $self->{_pg_version};
+}
+
+# Private routine which returns a reference to an array of integers
+# representing the pg_version of a PostgresNode, or parsed from a postgres
+# version string.  Development versions (such as "14devel") are converted
+# to an array with minus one as the last value (such as [14, -1]).
+#
+# For idempotency, will return the argument back to the caller if handed an
+# array reference.
+sub _pg_version_array
+{
+	my ($arg) = @_;
+
+	# accept node arguments
+	return _pg_version_array($arg->{_pg_version})
+		if (blessed($arg) && $arg->isa("PostgresNode"));
+
+	# idempotency
+	return $arg
+		if (ref($arg) && ref($arg) =~ /ARRAY/);
+
+	# Accept standard formats, in case caller has handed us the output of a
+	# postgres command line tool
+	$arg = $1
+		if ($arg =~ m/\(?PostgreSQL\)? 

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 7, 2021, at 8:43 AM, Andrew Dunstan  wrote:
> 
> 
> On 4/7/21 1:03 AM, Mark Dilger wrote:
>> The v1 patch supported postgres versions back to 8.4, but v2 pushes that 
>> back to 8.1.
>> 
>> The version of PostgresNode currently committed relies on IPC::Run in a way 
>> that is subtly wrong.  The first time IPC::Run::run(X, ...) is called, it 
>> uses the PATH as it exists at that time, resolves the path for X, and caches 
>> it.  Subsequent calls to IPC::Run::run(X, ...) use the cached path, without 
>> respecting changes to $ENV{PATH}.  In practice, this means that:
>> 
>>  use PostgresNode;
>> 
>>  my $a = PostgresNode->get_new_node('a', install_path => '/my/install/8.4');
>>  my $b = PostgresNode->get_new_node('b', install_path => '/my/install/9.0');
>> 
>>  $a->safe_psql(...)   # <=== Resolves and caches 'psql' as 
>> /my/install/8.4/bin/psql
>> 
>>  $b->safe_psql(...)   # <=== Executes /my/install/8.4/bin/psql, not 
>> /my/install/9.0/bin/psql as one might expect
>> 
>> PostgresNode::safe_psql() and PostgresNode::psql() both suffer from this, 
>> and similarly PostgresNode::pg_recvlogical_upto() because the path to 
>> pg_recvlogical gets cached.  Calls to initdb and pg_ctl do not appear to 
>> suffer this problem, as they are ultimately handled by perl's system() call, 
>> not by IPC::Run::run.
>> 
>> Since postgres commands work fairly similarly from one release to another, 
>> this can cause subtle and hard to diagnose bugs in regression tests.  The 
>> fix in v2-0001 works for me, as demonstrated by v2-0002, but whether the fix 
>> in the attached v2 patch set gets used or not, I think something needs to be 
>> done to fix this.
>> 
>> 
> 
> Awesome work. The IPC::Run behaviour is darned unfriendly, and AFAICS
> completely undocumented. It can't even be easily modified by a client
> because the cache is stashed in a lexical variable.

Yes, I noticed that, too.  Even if we could get a patch accepted into IPC::Run, 
we'd need to be compatible with older versions.  So there doesn't seem to be 
any option but to work around the issue.

> You fix looks good.

Thanks for reviewing!


> other notes:
> 
> 
> . needs a perltidy run, some lines are too long (see
> src/tools/pgindent/perltidyrc)
> 
> 
> . Please use an explicit return here:
> 
> 
> +# Return an array reference
> +[ @result ];

Done.


> . I'm not sure the computation in _pg_version_cmp is right. What if the
> number of elements differ? As I read it you would return 0 for a
> comparison of '1.2' and '1.2.3'. Is that what's intended?

Yes, that is intended.  '1.2' and '1.2.0' are not the same.  '1.2' means "some 
unspecified micro release or development version of 1.2", whereas '1.2.0' does 
not.

This is useful for things like $node->at_least_version("13") returning true for 
development versions of version 13, which are otherwise less than (not equal 
to) 13.0

> . The second patch has a bunch of stuff it doesn't need. The control
> file should be unnecessary as should all the lines above 'ifdef
> USE_PGXS' in the Makefile except 'TAP_TESTS = 1'

Done.

Yeah, I started the second patch as simply a means of testing the first and 
didn't clean it up after copying boilerplate from elsewhere.  The second patch 
has turned into something possibly worth keeping in its own right, and having 
the build farm execute on a regular basis.

> . the test script should have a way of passing a non-default version
> file to CrossVersion::nodes(). Possible get it from an environment variable?

Good idea.  I changed it to use $ENV{PG_TEST_VERSIONS_FILE}.  I'm open to other 
names for this variable.

> . I'm somewhat inclined to say that CrossVersion should just return a
> {name => path} map, and let the client script do the node creation. Then
> crossVersion doesn't need to know anything much about the
> infrastructure. But I could possibly be persuaded otherwise. Also, maybe
> it belongs in src/test/perl.

Hmm.  That's a good thought. I've moved it to src/test/perl with the change you 
suggest.

> . This line appears deundant, the variable is not referenced:
> 
> 
> +my $path = $ENV{PATH};

Removed.

> Also these lines at the bottom of CrossVersion.pm are redundant:
> 
> 
> +use strict;
> +use warnings;

Yeah, those are silly.  Removed.

This patch has none of the Object Oriented changes Alvaro and Jehan-Guillaume 
requested, but that should not be construed as an argument against their 
request.  It's just not handled in this particular patch.



v3-0001-Extending-PostgresNode-cross-version-functionalit.patch
Description: Binary data


v3-0002-Adding-modules-test_cross_version.patch
Description: Binary data


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 7, 2021, at 2:04 PM, Alvaro Herrera  wrote:
> 
> On 2021-Apr-07, Mark Dilger wrote:
> 
>> It seems we're debating between two designs.  In the first, each
>> PostgresNode function knows about version limitations and has code
>> like:
>> 
>>  DoSomething() if $self->at_least_version("11")
> 
> Yeah, I didn't like this approach -- it is quite messy.
> 
>> and in the second design we're subclassing for each postgres release
>> where something changed, so that DoSomething is implemented
>> differently in one class than another.
> 
> So DoSomething still does Something, regardless of what it has to do in
> order to get it done.
> 
>> I think the subclassing solution is cleaner if the number of version
>> tests is large, but not so much otherwise.
> 
> Well, your patch has rather a lot of at_least_version() tests.

I don't really care about this part, and you do, so you win.  I'm happy enough 
to have this be done with subclassing.  My problems upthread never had anything 
to do with whether we used subclassing, but rather which behaviors were 
supported.  And that appears not to be controversial, so that's all for the 
good


>> To wit:
>> 
>># "restart_after_crash" was introduced in version 9.1.  Older versions
>># always restart after crash.
>>print $conf "restart_after_crash = off\n"
>>if $self->at_least_version("9.1");
>> 
>> PostgresNode is mostly designed around supporting regression tests for
>> the current postgres version under development.
> 
> I think we should make PostgresNode do what makes the most sense for the
> current branch, and go to whatever contortions are necessary to do the
> same thing in older versions as long as it is sensible.  If we were
> robots, then we would care to preserve behavior down to the very last
> byte, but I think we can make judgement calls to ignore the changes that
> are not relevant.  Whenever we introduce a behavior that is not
> supportable by the older version, then the function would throw an error
> if that behavior is requested from that older version.

Beep bop boop beeb bop, danger Will Robinson:

for my $i (@all_postgres_versions)
{
for my $j (grep { $_ > $i } @all_postgres_versions)
{
for my $k (grep { $_ > $j } @all_postgres_versions)
{
my $node = node_of_version($i);
$node->do_stuff();
$node->pg_upgrade_to($j);
$node->do_more_stuff();
$node->pg_upgrade_to($k);
$node->do_yet_more_stuff();

# verify $node isn't broken
}
}
}

I think it is harder to write simple tests like this when how $node behaves 
changes as the values of (i,j,k) change.  Of course the behaviors change to the 
extent that postgres itself changed between versions.  I mean changes because 
PostgresNode behaves differently.

We don't need to debate this now, though.  It will be better to discuss 
individual issues as they come up.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-07, Andrew Dunstan wrote:

> Oh, you want to roll them all up into one file? That could work. It's a
> bit frowned on by perl purists, but I've done similar (see PGBuild/SCM.pm).

Ah!  Yeah, pretty much exactly like that, including the "no critic" flag ...

-- 
Álvaro Herrera39°49'30"S 73°17'W
"El sudor es la mejor cura para un pensamiento enfermo" (Bardia)

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-07, Mark Dilger wrote:

> It seems we're debating between two designs.  In the first, each
> PostgresNode function knows about version limitations and has code
> like:
> 
>   DoSomething() if $self->at_least_version("11")

Yeah, I didn't like this approach -- it is quite messy.

> and in the second design we're subclassing for each postgres release
> where something changed, so that DoSomething is implemented
> differently in one class than another.

So DoSomething still does Something, regardless of what it has to do in
order to get it done.

> I think the subclassing solution is cleaner if the number of version
> tests is large, but not so much otherwise.

Well, your patch has rather a lot of at_least_version() tests.

> To wit:
> 
> # "restart_after_crash" was introduced in version 9.1.  Older versions
> # always restart after crash.
> print $conf "restart_after_crash = off\n"
> if $self->at_least_version("9.1");
> 
> PostgresNode is mostly designed around supporting regression tests for
> the current postgres version under development.

I think we should make PostgresNode do what makes the most sense for the
current branch, and go to whatever contortions are necessary to do the
same thing in older versions as long as it is sensible.  If we were
robots, then we would care to preserve behavior down to the very last
byte, but I think we can make judgement calls to ignore the changes that
are not relevant.  Whenever we introduce a behavior that is not
supportable by the older version, then the function would throw an error
if that behavior is requested from that older version.

> Prior to Andrew's recent introduction of support for alternate
> installation paths, it made sense to have restart_after_crash be off.
> But now, if you spin up a postgres node for version 9.0 or before, you
> get different behavior, because the prior behavior is to implicitly
> have this "on", not "off".

That seems mostly okay, except in a very few narrow cases where the node
staying down is critical.  So we can let things be.

> Again:
> 
> # "log_replication_commands" was introduced in 9.5.  Older versions do
> # not log replication commands.
> print $conf "log_replication_commands = on\n"
> if $self->at_least_version("9.5");
> 
> Should we have "log_replication_commands" be off by default so that
> nodes of varying postgres version behave more similarly?

If it's important for the tests, then let's have a method to change it
if necessary.  Otherwise, we don't care.

-- 
Álvaro Herrera   Valdivia, Chile

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/7/21 4:48 PM, Mark Dilger wrote:
>
>> On Apr 7, 2021, at 1:28 PM, Alvaro Herrera  wrote:
>>
>> On 2021-Apr-07, Mark Dilger wrote:
>>
>>> I was commenting on the design to have the PostgresNode derived
>>> subclass hard-coded to return "10" as the version:
>>>
>>>sub version { return 10 }
>> That seems a minor bug rather than a showstopper design deficiency.
>> I agree that hardcoding the version in the source code is not very
>> usable; it should store the version number when it runs pg_config
>> --version in an instance variable that can be returned.
> It seems we're debating between two designs.  In the first, each PostgresNode 
> function knows about version limitations and has code like:
>
>   DoSomething() if $self->at_least_version("11")
>
> and in the second design we're subclassing for each postgres release where 
> something changed, so that DoSomething is implemented differently in one 
> class than another.  I think the subclassing solution is cleaner if the 
> number of version tests is large, but not so much otherwise.



I think you and I are of one mind here.


>
>
> There is a much bigger design decision to be made that I have delayed making. 
>  The PostgresNode implementation has functions that work a certain way, but 
> cannot work that same way with older versions of postgres that don't have the 
> necessary support.  This means that
>
>   $my_node->do_something(...)
>
> works differently based on which version of postgres $my_node is based upon, 
> even though PostgresNode could have avoided it.  To wit:
>
> # "restart_after_crash" was introduced in version 9.1.  Older versions
> # always restart after crash.
> print $conf "restart_after_crash = off\n"
> if $self->at_least_version("9.1");
>
> PostgresNode is mostly designed around supporting regression tests for the 
> current postgres version under development.  Prior to Andrew's recent 
> introduction of support for alternate installation paths, it made sense to 
> have restart_after_crash be off.  But now, if you spin up a postgres node for 
> version 9.0 or before, you get different behavior, because the prior behavior 
> is to implicitly have this "on", not "off".
>
> Again:
>
> # "log_replication_commands" was introduced in 9.5.  Older versions do
> # not log replication commands.
> print $conf "log_replication_commands = on\n"
> if $self->at_least_version("9.5");
>
> Should we have "log_replication_commands" be off by default so that nodes of 
> varying postgres version behave more similarly?
>
> Again:
>
> # "wal_retrieve_retry_interval" was introduced in 9.5.  Older versions
> # always wait 5 seconds.
> print $conf "wal_retrieve_retry_interval = '500ms'\n"
> if $self->at_least_version("9.5");
>
>
> Should we have "wal_retrieve_retry_interval" be 5 seconds for consistency?
>
> I didn't do these things, as I didn't want to break the majority of tests 
> which don't care about cross version compatibility, but if we're going to 
> debate this thing, subclassing is a distraction.  The real question is, *what 
> do we want it to do*?
>
>


Yeah, much more important. I think I would say that anything that's
simply not possible in an older version should cause an error and for
the rest the old version should probably behave by default as much like
its default as possible. I don't think we should try to make different
versions behave identically (or as close to as possible). In some
particular cases we should be able to override default behavior (e.g. by
setting the config explicitly).


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 7, 2021, at 1:28 PM, Alvaro Herrera  wrote:
> 
> On 2021-Apr-07, Mark Dilger wrote:
> 
>> I was commenting on the design to have the PostgresNode derived
>> subclass hard-coded to return "10" as the version:
>> 
>>sub version { return 10 }
> 
> That seems a minor bug rather than a showstopper design deficiency.
> I agree that hardcoding the version in the source code is not very
> usable; it should store the version number when it runs pg_config
> --version in an instance variable that can be returned.

It seems we're debating between two designs.  In the first, each PostgresNode 
function knows about version limitations and has code like:

DoSomething() if $self->at_least_version("11")

and in the second design we're subclassing for each postgres release where 
something changed, so that DoSomething is implemented differently in one class 
than another.  I think the subclassing solution is cleaner if the number of 
version tests is large, but not so much otherwise.


There is a much bigger design decision to be made that I have delayed making.  
The PostgresNode implementation has functions that work a certain way, but 
cannot work that same way with older versions of postgres that don't have the 
necessary support.  This means that

$my_node->do_something(...)

works differently based on which version of postgres $my_node is based upon, 
even though PostgresNode could have avoided it.  To wit:

# "restart_after_crash" was introduced in version 9.1.  Older versions
# always restart after crash.
print $conf "restart_after_crash = off\n"
if $self->at_least_version("9.1");

PostgresNode is mostly designed around supporting regression tests for the 
current postgres version under development.  Prior to Andrew's recent 
introduction of support for alternate installation paths, it made sense to have 
restart_after_crash be off.  But now, if you spin up a postgres node for 
version 9.0 or before, you get different behavior, because the prior behavior 
is to implicitly have this "on", not "off".

Again:

# "log_replication_commands" was introduced in 9.5.  Older versions do
# not log replication commands.
print $conf "log_replication_commands = on\n"
if $self->at_least_version("9.5");

Should we have "log_replication_commands" be off by default so that nodes of 
varying postgres version behave more similarly?

Again:

# "wal_retrieve_retry_interval" was introduced in 9.5.  Older versions
# always wait 5 seconds.
print $conf "wal_retrieve_retry_interval = '500ms'\n"
if $self->at_least_version("9.5");


Should we have "wal_retrieve_retry_interval" be 5 seconds for consistency?

I didn't do these things, as I didn't want to break the majority of tests which 
don't care about cross version compatibility, but if we're going to debate this 
thing, subclassing is a distraction.  The real question is, *what do we want it 
to do*?


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/7/21 4:19 PM, Alvaro Herrera wrote:
> On 2021-Apr-07, Andrew Dunstan wrote:
>
>> b) as it stands pgaTester.pm can't be used for multiple versions in a
>> single program, which is a design goal here - it sets the single class
>> to invoke in its BEGIN block. At the very least we would need to replace
>> that with code which would require the relevant class as needed.
> I'm not suggesting that we adopt pgaTester.pm!  I think a real patch for
> this approach involves moving that stuff into PostgresNode::new itself,
> as I said upthread: if install_path is given, call pg_config --version
> and then parse the version number into a class name $versionclass, then
> "bless $versionclass, $self".  So the object returned by
> PostgresNode::new already has the correct class.  We don't need to
> require anything, since all classes are in the same PostgresNode.pm
> file.
>


Oh, you want to roll them all up into one file? That could work. It's a
bit frowned on by perl purists, but I've done similar (see PGBuild/SCM.pm).


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-07, Mark Dilger wrote:

> I was commenting on the design to have the PostgresNode derived
> subclass hard-coded to return "10" as the version:
> 
> sub version { return 10 }

That seems a minor bug rather than a showstopper design deficiency.
I agree that hardcoding the version in the source code is not very
usable; it should store the version number when it runs pg_config
--version in an instance variable that can be returned.

-- 
Álvaro Herrera39°49'30"S 73°17'W
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-07, Andrew Dunstan wrote:

> b) as it stands pgaTester.pm can't be used for multiple versions in a
> single program, which is a design goal here - it sets the single class
> to invoke in its BEGIN block. At the very least we would need to replace
> that with code which would require the relevant class as needed.

I'm not suggesting that we adopt pgaTester.pm!  I think a real patch for
this approach involves moving that stuff into PostgresNode::new itself,
as I said upthread: if install_path is given, call pg_config --version
and then parse the version number into a class name $versionclass, then
"bless $versionclass, $self".  So the object returned by
PostgresNode::new already has the correct class.  We don't need to
require anything, since all classes are in the same PostgresNode.pm
file.

-- 
Álvaro Herrera   Valdivia, Chile
"This is what I like so much about PostgreSQL.  Most of the surprises
are of the "oh wow!  That's cool" Not the "oh shit!" kind.  :)"
Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/7/21 3:09 PM, Alvaro Herrera wrote:
> On 2021-Apr-07, Andrew Dunstan wrote:
>
>> Aren't you likely to end up duplicating substantial amounts of code,
>> though?
> No — did you look at his code?  Each version is child of the one just
> above, so you only need to override things where behavior changes from
> one version to the next.
>

yes


>> I'm certainly not at the stage where I think the version-aware
>> code is creating too much clutter. The "forest of conditionals" seems
>> more like a small thicket.
> After comparing both approaches, I think ioguix's is superior in
> cleanliness.
>

a) I'm not mad keen on having oodles of little classes. I should point
out that this will have to traverse possibly the whole hierarchy of
classes at each call to get the the actual method, which is not very
efficient. But to some extent this is a matter of taste. OTOH

b) as it stands pgaTester.pm can't be used for multiple versions in a
single program, which is a design goal here - it sets the single class
to invoke in its BEGIN block. At the very least we would need to replace
that with code which would require the relevant class as needed.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 7, 2021, at 12:13 PM, Alvaro Herrera  wrote:
> 
> On 2021-Apr-07, Mark Dilger wrote:
> 
>> It's not sufficient to think about postgres versions as "10", "11",
>> etc.  You have to be able to spin up nodes of any build, like "9.0.7".
>> There are specific versions of postgres with specific bugs that cause
>> specific problems, and later versions of postgres on that same
>> development branch have been patched.  If you only ever spin up the
>> latest version, you can't reproduce the problems and test how they
>> impact cross version compatibility.
> 
> I don't get it.  Surely if you need 10.0.7 you just compile that version
> and give its path as install path?  You can have two 1.0.x as long as
> install them separately, right?

I was commenting on the design to have the PostgresNode derived subclass 
hard-coded to return "10" as the version:

sub version { return 10 }


It's hard to think about how this other system works when you have lots of 
separate micro releases all compiled and used as the basis of the $node's, 
since this other system does not support that at all.  So maybe it can be done 
properly, but I don't want to have different microversions of 10 and then find 
that $a->version eq $b->version when $a is 10.1 and $b is 10.2.

>> I don't think it works to have a class per micro release.
> 
> I don't understand why you would do that.

If you need a "version" subroutine per derived class, then the only way to 
solve the problem is to have a profusion of derived classes.  It would make 
more sense to me if the version method worked the way I implemented it, where 
it just returns the version obtained from pg_config


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-07, Mark Dilger wrote:

> It's not sufficient to think about postgres versions as "10", "11",
> etc.  You have to be able to spin up nodes of any build, like "9.0.7".
> There are specific versions of postgres with specific bugs that cause
> specific problems, and later versions of postgres on that same
> development branch have been patched.  If you only ever spin up the
> latest version, you can't reproduce the problems and test how they
> impact cross version compatibility.

I don't get it.  Surely if you need 10.0.7 you just compile that version
and give its path as install path?  You can have two 1.0.x as long as
install them separately, right?

> I don't think it works to have a class per micro release.

I don't understand why you would do that.


-- 
Álvaro Herrera39°49'30"S 73°17'W
"Crear es tan difícil como ser libre" (Elsa Triolet)

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-07, Andrew Dunstan wrote:

> Aren't you likely to end up duplicating substantial amounts of code,
> though?

No — did you look at his code?  Each version is child of the one just
above, so you only need to override things where behavior changes from
one version to the next.

> I'm certainly not at the stage where I think the version-aware
> code is creating too much clutter. The "forest of conditionals" seems
> more like a small thicket.

After comparing both approaches, I think ioguix's is superior in
cleanliness.

-- 
Álvaro Herrera39°49'30"S 73°17'W
 It does it in a really, really complicated way
 why does it need to be complicated?
 Because it's MakeMaker.

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 7, 2021, at 11:35 AM, Jehan-Guillaume de Rorthais  
> wrote:
> 
> On Wed, 7 Apr 2021 13:38:39 -0400
> Andrew Dunstan  wrote:
> 
>> On 4/7/21 1:19 PM, Jehan-Guillaume de Rorthais wrote:
>>> On Wed, 7 Apr 2021 12:51:55 -0400
>>> Alvaro Herrera  wrote:
>>> 
 On 2021-Apr-07, Jehan-Guillaume de Rorthais wrote:
 
> When I'm creating a new node, I'm using the "pgaTester" factory class. It
> relies on PATH to check the major version using pg_config, then loads the
> appropriate class.
 From a code cleanliness point of view, I agree that having separate
 classes for each version is neater than what you call a forest of
 conditionals.  I'm not sure I like the way you instantiate the classes
 in pgaTester though -- wouldn't it be saner to have PostgresNode::new
 itself be in charge of deciding which class to bless the object as?
 Since we're talking about modifying PostgresNode itself in order to
 support this, it would make sense to do that.  
>>> Yes, it would be much saner to make PostgresNode the factory class. Plus,
>>> some more logic could be injected there to either auto-detect the version
>>> (current behavior) or eg. use a given path to the binaries as Mark did in
>>> its patch.  
>> 
>> 
>> Aren't you likely to end up duplicating substantial amounts of code,
>> though? I'm certainly not at the stage where I think the version-aware
>> code is creating too much clutter. The "forest of conditionals" seems
>> more like a small thicket.
> 
> I started with a patched PostgresNode. Then I had to support backups and
> replication for my tests. Then it become hard to follow the logic in
> conditional blocks, moreover some conditionals needed to appear in multiple
> places in the same methods, depending on the enabled features, the conditions,
> what GUC was enabled by default or not, etc. So I end up with this design.
> 
> I really don't want to waste community brain cycles in discussions and useless
> reviews. But as far as someone agree to review it, I already have the material
> and I can create a patch with a limited amount of work to compare and review.
> The one-class approach would need to support replication down to 9.0 to be 
> fair
> though :/

If you can create a patch that integrates your ideas into PostgresNode.pm, I'd 
be interested in reviewing it.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Wed, 7 Apr 2021 13:38:39 -0400
Andrew Dunstan  wrote:

> On 4/7/21 1:19 PM, Jehan-Guillaume de Rorthais wrote:
> > On Wed, 7 Apr 2021 12:51:55 -0400
> > Alvaro Herrera  wrote:
> >  
> >> On 2021-Apr-07, Jehan-Guillaume de Rorthais wrote:
> >>  
> >>> When I'm creating a new node, I'm using the "pgaTester" factory class. It
> >>> relies on PATH to check the major version using pg_config, then loads the
> >>> appropriate class.
> >> From a code cleanliness point of view, I agree that having separate
> >> classes for each version is neater than what you call a forest of
> >> conditionals.  I'm not sure I like the way you instantiate the classes
> >> in pgaTester though -- wouldn't it be saner to have PostgresNode::new
> >> itself be in charge of deciding which class to bless the object as?
> >> Since we're talking about modifying PostgresNode itself in order to
> >> support this, it would make sense to do that.  
> > Yes, it would be much saner to make PostgresNode the factory class. Plus,
> > some more logic could be injected there to either auto-detect the version
> > (current behavior) or eg. use a given path to the binaries as Mark did in
> > its patch.  
> 
> 
> Aren't you likely to end up duplicating substantial amounts of code,
> though? I'm certainly not at the stage where I think the version-aware
> code is creating too much clutter. The "forest of conditionals" seems
> more like a small thicket.

I started with a patched PostgresNode. Then I had to support backups and
replication for my tests. Then it become hard to follow the logic in
conditional blocks, moreover some conditionals needed to appear in multiple
places in the same methods, depending on the enabled features, the conditions,
what GUC was enabled by default or not, etc. So I end up with this design.

I really don't want to waste community brain cycles in discussions and useless
reviews. But as far as someone agree to review it, I already have the material
and I can create a patch with a limited amount of work to compare and review.
The one-class approach would need to support replication down to 9.0 to be fair
though :/

Thanks,

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Wed, 7 Apr 2021 10:50:26 -0700
Mark Dilger  wrote:

> > On Apr 7, 2021, at 10:36 AM, Alvaro Herrera  wrote:
> >   
> >> Yes, it would be much saner to make PostgresNode the factory class. Plus,
> >> some more logic could be injected there to either auto-detect the version
> >> (current behavior) or eg. use a given path to the binaries as Mark did in
> >> its patch.  
> > 
> > I'm not sure what you mean about auto-detecting the version -- I assume
> > we would auto-detect the version by calling pg_config from the
> > configured path and parsing the binary, which is what Mark's patch is
> > supposed to do already.  So I don't see what the distinction between
> > those two things is.
> > 
> > In order to avoid having an ever-growing plethora of 100-byte .pm files,
> > we can put the version-specific classes in the same PostgresNode.pm
> > file, at the bottom, "class PostgresNode96; use parent PostgresNode10;"
> > followed by the routines that are overridden for each version.  
> 
> It's not sufficient to think about postgres versions as "10", "11", etc.  You
> have to be able to spin up nodes of any build, like "9.0.7". There are
> specific versions of postgres with specific bugs that cause specific
> problems, and later versions of postgres on that same development branch have
> been patched.  If you only ever spin up the latest version, you can't
> reproduce the problems and test how they impact cross version compatibility.


Agree.

> I don't think it works to have a class per micro release.  So you'd have a
> PostgresNode of type "10" or such, but how does that help?  If you have ten
> different versions of "10" in your test, they all look the same?

As PostgresNode factory already checked pg_config version, it can give it as
argument to the specific class constructor. We can then have eg.
major_version() and version() to return the major version and full versions.

Regards,

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Wed, 7 Apr 2021 13:36:31 -0400
Alvaro Herrera  wrote:

> On 2021-Apr-07, Jehan-Guillaume de Rorthais wrote:
> 
> > Yes, it would be much saner to make PostgresNode the factory class. Plus,
> > some more logic could be injected there to either auto-detect the version
> > (current behavior) or eg. use a given path to the binaries as Mark did in
> > its patch.  
> 
> I'm not sure what you mean about auto-detecting the version -- I assume
> we would auto-detect the version by calling pg_config from the
> configured path and parsing the binary, which is what Mark's patch is
> supposed to do already.  So I don't see what the distinction between
> those two things is.

My version is currently calling pg_config without any knowledge about its
absolute path.

Mark's patch is able to take an explicit binary path:

  my $a = PostgresNode->get_new_node('a', install_path => '/my/install/8.4');

> In order to avoid having an ever-growing plethora of 100-byte .pm files,
> we can put the version-specific classes in the same PostgresNode.pm
> file, at the bottom, "class PostgresNode96; use parent PostgresNode10;"
> followed by the routines that are overridden for each version.

Sure.

> > Let me know if it worth that I work on an official patch.  
> 
> Let's give it a try ...

OK

Regards,

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 7, 2021, at 10:36 AM, Alvaro Herrera  wrote:
> 
>> Yes, it would be much saner to make PostgresNode the factory class. Plus, 
>> some
>> more logic could be injected there to either auto-detect the version (current
>> behavior) or eg. use a given path to the binaries as Mark did in its patch.
> 
> I'm not sure what you mean about auto-detecting the version -- I assume
> we would auto-detect the version by calling pg_config from the
> configured path and parsing the binary, which is what Mark's patch is
> supposed to do already.  So I don't see what the distinction between
> those two things is.
> 
> In order to avoid having an ever-growing plethora of 100-byte .pm files,
> we can put the version-specific classes in the same PostgresNode.pm
> file, at the bottom, "class PostgresNode96; use parent PostgresNode10;"
> followed by the routines that are overridden for each version.

It's not sufficient to think about postgres versions as "10", "11", etc.  You 
have to be able to spin up nodes of any build, like "9.0.7".  There are 
specific versions of postgres with specific bugs that cause specific problems, 
and later versions of postgres on that same development branch have been 
patched.  If you only ever spin up the latest version, you can't reproduce the 
problems and test how they impact cross version compatibility.

I don't think it works to have a class per micro release.  So you'd have a 
PostgresNode of type "10" or such, but how does that help?  If you have ten 
different versions of "10" in your test, they all look the same?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/7/21 1:19 PM, Jehan-Guillaume de Rorthais wrote:
> On Wed, 7 Apr 2021 12:51:55 -0400
> Alvaro Herrera  wrote:
>
>> On 2021-Apr-07, Jehan-Guillaume de Rorthais wrote:
>>
>>> When I'm creating a new node, I'm using the "pgaTester" factory class. It
>>> relies on PATH to check the major version using pg_config, then loads the
>>> appropriate class.  
>> From a code cleanliness point of view, I agree that having separate
>> classes for each version is neater than what you call a forest of
>> conditionals.  I'm not sure I like the way you instantiate the classes
>> in pgaTester though -- wouldn't it be saner to have PostgresNode::new
>> itself be in charge of deciding which class to bless the object as?
>> Since we're talking about modifying PostgresNode itself in order to
>> support this, it would make sense to do that.
> Yes, it would be much saner to make PostgresNode the factory class. Plus, some
> more logic could be injected there to either auto-detect the version (current
> behavior) or eg. use a given path to the binaries as Mark did in its patch.


Aren't you likely to end up duplicating substantial amounts of code,
though? I'm certainly not at the stage where I think the version-aware
code is creating too much clutter. The "forest of conditionals" seems
more like a small thicket.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-07, Jehan-Guillaume de Rorthais wrote:

> Yes, it would be much saner to make PostgresNode the factory class. Plus, some
> more logic could be injected there to either auto-detect the version (current
> behavior) or eg. use a given path to the binaries as Mark did in its patch.

I'm not sure what you mean about auto-detecting the version -- I assume
we would auto-detect the version by calling pg_config from the
configured path and parsing the binary, which is what Mark's patch is
supposed to do already.  So I don't see what the distinction between
those two things is.

In order to avoid having an ever-growing plethora of 100-byte .pm files,
we can put the version-specific classes in the same PostgresNode.pm
file, at the bottom, "class PostgresNode96; use parent PostgresNode10;"
followed by the routines that are overridden for each version.

> Let me know if it worth that I work on an official patch.

Let's give it a try ...

-- 
Álvaro Herrera   Valdivia, Chile

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Wed, 7 Apr 2021 12:51:55 -0400
Alvaro Herrera  wrote:

> On 2021-Apr-07, Jehan-Guillaume de Rorthais wrote:
> 
> > When I'm creating a new node, I'm using the "pgaTester" factory class. It
> > relies on PATH to check the major version using pg_config, then loads the
> > appropriate class.  
> 
> From a code cleanliness point of view, I agree that having separate
> classes for each version is neater than what you call a forest of
> conditionals.  I'm not sure I like the way you instantiate the classes
> in pgaTester though -- wouldn't it be saner to have PostgresNode::new
> itself be in charge of deciding which class to bless the object as?
> Since we're talking about modifying PostgresNode itself in order to
> support this, it would make sense to do that.

Yes, it would be much saner to make PostgresNode the factory class. Plus, some
more logic could be injected there to either auto-detect the version (current
behavior) or eg. use a given path to the binaries as Mark did in its patch.

> (I understand that one of your decisions was to avoid modifying
> PostgresNode, so that you could ingest whatever came out of PGDG without
> having to patch it each time.)

Indeed, that's why I created this class with a not-very-fortunate name :) 

Let me know if it worth that I work on an official patch.

Regards,

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 7, 2021, at 9:26 AM, Jehan-Guillaume de Rorthais  
> wrote:
> 
> On Wed, 7 Apr 2021 09:08:31 -0700
> Mark Dilger  wrote:
> 
>>> On Apr 7, 2021, at 7:37 AM, Jehan-Guillaume de Rorthais 
> 
>>> And here is a demo test file:
>>> https://github.com/ioguix/check_pgactivity/blob/tests/t/01-streaming_delta.t
>>> 
>>> My limited set of tests are working with versions back to 9.0 so far.
>>> 
>>> My 2¢  
>> 
>> Hmm, I took a look.  I'm not sure that we're working on the same problem, but
>> I might have missed something.
> 
> I understood you were working on making PostgresNode compatible with older
> versions of PostgreSQL. So ou can create and interact with older versions,
> eg. 9.0. Did I misunderstood?

We're both working on compatibility with older versions, that is true.  I may 
have misunderstood your code somewhat.  It is hard to quickly review your 
changes, as they are all mixed together with a mass of copied code from the 
main project.  I'll look some more for parts that I could reuse.

Thanks

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Apr-07, Jehan-Guillaume de Rorthais wrote:

> When I'm creating a new node, I'm using the "pgaTester" factory class. It
> relies on PATH to check the major version using pg_config, then loads the
> appropriate class.

>From a code cleanliness point of view, I agree that having separate
classes for each version is neater than what you call a forest of
conditionals.  I'm not sure I like the way you instantiate the classes
in pgaTester though -- wouldn't it be saner to have PostgresNode::new
itself be in charge of deciding which class to bless the object as?
Since we're talking about modifying PostgresNode itself in order to
support this, it would make sense to do that.

(I understand that one of your decisions was to avoid modifying
PostgresNode, so that you could ingest whatever came out of PGDG without
having to patch it each time.)

-- 
Álvaro Herrera   Valdivia, Chile

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Wed, 7 Apr 2021 09:08:31 -0700
Mark Dilger  wrote:

> > On Apr 7, 2021, at 7:37 AM, Jehan-Guillaume de Rorthais 

> > And here is a demo test file:
> > https://github.com/ioguix/check_pgactivity/blob/tests/t/01-streaming_delta.t
> > 
> > My limited set of tests are working with versions back to 9.0 so far.
> > 
> > My 2¢  
> 
> Hmm, I took a look.  I'm not sure that we're working on the same problem, but
> I might have missed something.

I understood you were working on making PostgresNode compatible with older
versions of PostgreSQL. So ou can create and interact with older versions,
eg. 9.0. Did I misunderstood?

My set of class had the exact same goal: creating and managing PostgreSQL nodes
from various major versions. It's going a bit further than just init() though,
as it supports some more existing methods (eg. enable_streaming) and adds some
others (version, switch_wal, wait_for_archive).

Regards,

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

On Wed, 7 Apr 2021 11:54:36 -0400
Andrew Dunstan  wrote:

> On 4/7/21 10:37 AM, Jehan-Guillaume de Rorthais wrote:
> > Hi all,
> >
> > First, sorry to step in this discussion this late. I didn't noticed it
> > before :(
> >
> > I did some work about these compatibility issues in late 2020 to use
> > PostgresNode in the check_pgactivity TAP tests.
> >
> > See https://github.com/ioguix/check_pgactivity/tree/tests/t/lib
> >
> > PostgresNode.pm, TestLib.pm, SimpleTee.pm and RecursiveCopy.pm comes
> > unchanged from PostgreSQL source file (see headers and COPYRIGHT.pgsql).
> >
> > Then, I'm using the facet class PostgresNodeFacet to extend it with some
> > more methods. Finaly, I created one class per majpr version, each
> > inheriting from the next version. That means 13 inherits from
> > PostgresNodeFacet.pm, 12 inherits from 13, 11 inherits from 12, etc.
> >
> > When I'm creating a new node, I'm using the "pgaTester" factory class. It
> > relies on PATH to check the major version using pg_config, then loads the
> > appropriate class.
> >
> > That means some class overrides almost no methods but version(), which
> > returns the major version. Eg.:
> > https://github.com/ioguix/check_pgactivity/blob/tests/t/lib/PostgresNode12.pm
> >
> > From tests, I can check the node version using this method, eg.:
> >
> >   skip "skip non-compatible test on PostgreSQL 8.0 and before", 3
> > unless $node->version <= 8.0;
> >
> > Of course, there's a lot of duplicate code between classes, but my main goal
> > was to keep PostgresNode.pm unchanged from upstream so I can easily update
> > it.
> >
> > And here is a demo test file:
> > https://github.com/ioguix/check_pgactivity/blob/tests/t/01-streaming_delta.t
> >
> > My limited set of tests are working with versions back to 9.0 so far.
> >
> > My 2¢  
> 
> 
> 
> I don't really want to create a multitude of classes. I think Mark is
> basically on the right track. I started off using a subclass of
> PostgresNode but was persuaded not to go down that route, and instead we
> have made some fairly minimal changes to PostgresNode itself. I think
> that was a good decision. If you take out the versioning subroutines,
> the actual version-aware changes Mark is proposing to PostgresNode are
> quite small - less that 200 lines supporting versions all the way back
> to 8.1. That's pretty awesome.


I took this road because as soon as you want to use some other methods like
enable_streaming, enable_archiving, etc, you find much more incompatibilities
on your way. My current stack of classes is backward compatible with much
more methods than just init(). But I admit it creates a multitude of class and
some duplicate code...

It's still possible to patch each methods in PostgresNode, but you'll end up
with a forest of conditional blocks depending on how far you want to go with old
PostgreSQL versions.

Regards,

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 7, 2021, at 7:37 AM, Jehan-Guillaume de Rorthais  
> wrote:
> 
> Hi all,
> 
> First, sorry to step in this discussion this late. I didn't noticed it before 
> :(

Not a problem.

> I did some work about these compatibility issues in late 2020 to use
> PostgresNode in the check_pgactivity TAP tests.
> 
> See https://github.com/ioguix/check_pgactivity/tree/tests/t/lib
> 
> PostgresNode.pm, TestLib.pm, SimpleTee.pm and RecursiveCopy.pm comes unchanged
> from PostgreSQL source file (see headers and COPYRIGHT.pgsql).
> 
> Then, I'm using the facet class PostgresNodeFacet to extend it with some more
> methods. Finaly, I created one class per majpr version, each inheriting from 
> the
> next version. That means 13 inherits from PostgresNodeFacet.pm, 12 inherits 
> from
> 13, 11 inherits from 12, etc.
> 
> When I'm creating a new node, I'm using the "pgaTester" factory class. It
> relies on PATH to check the major version using pg_config, then loads the
> appropriate class.
> 
> That means some class overrides almost no methods but version(), which returns
> the major version. Eg.:
> https://github.com/ioguix/check_pgactivity/blob/tests/t/lib/PostgresNode12.pm
> 
> From tests, I can check the node version using this method, eg.:
> 
>  skip "skip non-compatible test on PostgreSQL 8.0 and before", 3
>unless $node->version <= 8.0;
> 
> Of course, there's a lot of duplicate code between classes, but my main goal
> was to keep PostgresNode.pm unchanged from upstream so I can easily update it.

I see that.

> And here is a demo test file:
> https://github.com/ioguix/check_pgactivity/blob/tests/t/01-streaming_delta.t
> 
> My limited set of tests are working with versions back to 9.0 so far.
> 
> My 2¢

Hmm, I took a look.  I'm not sure that we're working on the same problem, but I 
might have missed something.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/7/21 10:37 AM, Jehan-Guillaume de Rorthais wrote:
> Hi all,
>
> First, sorry to step in this discussion this late. I didn't noticed it before 
> :(
>
> I did some work about these compatibility issues in late 2020 to use
> PostgresNode in the check_pgactivity TAP tests.
>
> See https://github.com/ioguix/check_pgactivity/tree/tests/t/lib
>
> PostgresNode.pm, TestLib.pm, SimpleTee.pm and RecursiveCopy.pm comes unchanged
> from PostgreSQL source file (see headers and COPYRIGHT.pgsql).
>
> Then, I'm using the facet class PostgresNodeFacet to extend it with some more
> methods. Finaly, I created one class per majpr version, each inheriting from 
> the
> next version. That means 13 inherits from PostgresNodeFacet.pm, 12 inherits 
> from
> 13, 11 inherits from 12, etc.
>
> When I'm creating a new node, I'm using the "pgaTester" factory class. It
> relies on PATH to check the major version using pg_config, then loads the
> appropriate class.
>
> That means some class overrides almost no methods but version(), which returns
> the major version. Eg.:
> https://github.com/ioguix/check_pgactivity/blob/tests/t/lib/PostgresNode12.pm
>
> From tests, I can check the node version using this method, eg.:
>
>   skip "skip non-compatible test on PostgreSQL 8.0 and before", 3
> unless $node->version <= 8.0;
>
> Of course, there's a lot of duplicate code between classes, but my main goal
> was to keep PostgresNode.pm unchanged from upstream so I can easily update it.
>
> And here is a demo test file:
> https://github.com/ioguix/check_pgactivity/blob/tests/t/01-streaming_delta.t
>
> My limited set of tests are working with versions back to 9.0 so far.
>
> My 2¢



I don't really want to create a multitude of classes. I think Mark is
basically on the right track. I started off using a subclass of
PostgresNode but was persuaded not to go down that route, and instead we
have made some fairly minimal changes to PostgresNode itself. I think
that was a good decision. If you take out the versioning subroutines,
the actual version-aware changes Mark is proposing to PostgresNode are
quite small - less that 200 lines supporting versions all the way back
to 8.1. That's pretty awesome.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/7/21 1:03 AM, Mark Dilger wrote:
> The v1 patch supported postgres versions back to 8.4, but v2 pushes that back 
> to 8.1.
>
> The version of PostgresNode currently committed relies on IPC::Run in a way 
> that is subtly wrong.  The first time IPC::Run::run(X, ...) is called, it 
> uses the PATH as it exists at that time, resolves the path for X, and caches 
> it.  Subsequent calls to IPC::Run::run(X, ...) use the cached path, without 
> respecting changes to $ENV{PATH}.  In practice, this means that:
>
>   use PostgresNode;
>
>   my $a = PostgresNode->get_new_node('a', install_path => '/my/install/8.4');
>   my $b = PostgresNode->get_new_node('b', install_path => '/my/install/9.0');
>
>   $a->safe_psql(...)   # <=== Resolves and caches 'psql' as 
> /my/install/8.4/bin/psql
>
>   $b->safe_psql(...)   # <=== Executes /my/install/8.4/bin/psql, not 
> /my/install/9.0/bin/psql as one might expect
>
> PostgresNode::safe_psql() and PostgresNode::psql() both suffer from this, and 
> similarly PostgresNode::pg_recvlogical_upto() because the path to 
> pg_recvlogical gets cached.  Calls to initdb and pg_ctl do not appear to 
> suffer this problem, as they are ultimately handled by perl's system() call, 
> not by IPC::Run::run.
>
> Since postgres commands work fairly similarly from one release to another, 
> this can cause subtle and hard to diagnose bugs in regression tests.  The fix 
> in v2-0001 works for me, as demonstrated by v2-0002, but whether the fix in 
> the attached v2 patch set gets used or not, I think something needs to be 
> done to fix this.
>
>

Awesome work. The IPC::Run behaviour is darned unfriendly, and AFAICS
completely undocumented. It can't even be easily modified by a client
because the cache is stashed in a lexical variable. You fix looks good.


other notes:


. needs a perltidy run, some lines are too long (see
src/tools/pgindent/perltidyrc)


. Please use an explicit return here:


+    # Return an array reference
+    [ @result ];


. I'm not sure the computation in _pg_version_cmp is right. What if the
number of elements differ? As I read it you would return 0 for a
comparison of '1.2' and '1.2.3'. Is that what's intended?


. The second patch has a bunch of stuff it doesn't need. The control
file should be unnecessary as should all the lines above 'ifdef
USE_PGXS' in the Makefile except 'TAP_TESTS = 1'


. the test script should have a way of passing a non-default version
file to CrossVersion::nodes(). Possible get it from an environment variable?


. I'm somewhat inclined to say that CrossVersion should just return a
{name => path} map, and let the client script do the node creation. Then
crossVersion doesn't need to know anything much about the
infrastructure. But I could possibly be persuaded otherwise. Also, maybe
it belongs in src/test/perl.


. This line appears deundant, the variable is not referenced:


+    my $path = $ENV{PATH};


Also these lines at the bottom of CrossVersion.pm are redundant:


+use strict;
+use warnings;


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Jehan-Guillaume de Rorthais]

Hi all,

First, sorry to step in this discussion this late. I didn't noticed it before :(

I did some work about these compatibility issues in late 2020 to use
PostgresNode in the check_pgactivity TAP tests.

See https://github.com/ioguix/check_pgactivity/tree/tests/t/lib

PostgresNode.pm, TestLib.pm, SimpleTee.pm and RecursiveCopy.pm comes unchanged
from PostgreSQL source file (see headers and COPYRIGHT.pgsql).

Then, I'm using the facet class PostgresNodeFacet to extend it with some more
methods. Finaly, I created one class per majpr version, each inheriting from the
next version. That means 13 inherits from PostgresNodeFacet.pm, 12 inherits from
13, 11 inherits from 12, etc.

When I'm creating a new node, I'm using the "pgaTester" factory class. It
relies on PATH to check the major version using pg_config, then loads the
appropriate class.

That means some class overrides almost no methods but version(), which returns
the major version. Eg.:
https://github.com/ioguix/check_pgactivity/blob/tests/t/lib/PostgresNode12.pm

From tests, I can check the node version using this method, eg.:

  skip "skip non-compatible test on PostgreSQL 8.0 and before", 3
unless $node->version <= 8.0;

Of course, there's a lot of duplicate code between classes, but my main goal
was to keep PostgresNode.pm unchanged from upstream so I can easily update it.

And here is a demo test file:
https://github.com/ioguix/check_pgactivity/blob/tests/t/01-streaming_delta.t

My limited set of tests are working with versions back to 9.0 so far.

My 2¢

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Apr 3, 2021, at 11:01 AM, Andrew Dunstan  wrote:
> 
> I've had a look at the first of these patches. I think it's generally
> ok, but:
> 
> 
> -TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
> +TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust',
> +$self->at_least_version("9.3") ? '-N' : (),
>  @{ $params{extra} });
> 
> 
> I'd rather do this in two steps to make it clearer.

Changed.

> I still think just doing pg_ctl -w unconditionally would be simpler.

Changed.

> Prior to 9.3 "unix_socket_directories" was spelled
> "unix_socket_directory". We should just set a variable appropriately and
> use it. That should make the changes around that a whole lot simpler.
> (c.f. buildfarm code)

Ah, good to know.  Changed.


Other changes:

The v1 patch supported postgres versions back to 8.4, but v2 pushes that back 
to 8.1.

The version of PostgresNode currently committed relies on IPC::Run in a way 
that is subtly wrong.  The first time IPC::Run::run(X, ...) is called, it uses 
the PATH as it exists at that time, resolves the path for X, and caches it.  
Subsequent calls to IPC::Run::run(X, ...) use the cached path, without 
respecting changes to $ENV{PATH}.  In practice, this means that:

  use PostgresNode;

  my $a = PostgresNode->get_new_node('a', install_path => '/my/install/8.4');
  my $b = PostgresNode->get_new_node('b', install_path => '/my/install/9.0');

  $a->safe_psql(...)   # <=== Resolves and caches 'psql' as 
/my/install/8.4/bin/psql

  $b->safe_psql(...)   # <=== Executes /my/install/8.4/bin/psql, not 
/my/install/9.0/bin/psql as one might expect

PostgresNode::safe_psql() and PostgresNode::psql() both suffer from this, and 
similarly PostgresNode::pg_recvlogical_upto() because the path to 
pg_recvlogical gets cached.  Calls to initdb and pg_ctl do not appear to suffer 
this problem, as they are ultimately handled by perl's system() call, not by 
IPC::Run::run.

Since postgres commands work fairly similarly from one release to another, this 
can cause subtle and hard to diagnose bugs in regression tests.  The fix in 
v2-0001 works for me, as demonstrated by v2-0002, but whether the fix in the 
attached v2 patch set gets used or not, I think something needs to be done to 
fix this.



v2-0001-Extending-PostgresNode-cross-version-functionalit.patch
Description: Binary data


v2-0002-Adding-modules-test_cross_version.patch
Description: Binary data


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 3/31/21 10:28 PM, Mark Dilger wrote:
>
>> On Mar 31, 2021, at 1:07 PM, Mark Dilger  
>> wrote:
>>
>>
>>
>>> On Mar 31, 2021, at 1:05 PM, Andrew Dunstan  wrote:
>>>
>>>
>>> On 3/31/21 3:48 PM, Alvaro Herrera wrote:
 On 2021-Mar-31, Mark Dilger wrote:

> PostgresNode::start() doesn't work for servers older than version 10,
> either.  If I hack that function to sleep until the postmaster.pid
> file exists, it works, but that is really ugly and is just to prove to
> myself that it is a timing issue.  There were a few commits in the
> version 10 development cycle (cf, commit
> f13ea95f9e473a43ee4e1baeb94daaf83535d37c) which changed how pg_ctl
> works, though I haven't figured out yet exactly what the interaction
> with PostgresNode would be.  I'll keep looking.
 Do you need to do "pg_ctl -w" perhaps?
>>>
>>>
>>> Probably. The buildfarm does this unconditionally and has done for a
>>> very long time, so maybe we don't need a version test for it.
>> I put a version test for this and it works for me.  I guess you could do it 
>> unconditionally, if you want, but the condition is just:
>>
>> -   TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
>> +   TestLib::system_or_bail('pg_ctl',
>> +   $self->older_than_version('10') ? '-w' : (),
>> +   '-D', $pgdata, '-l', $logfile,
>>'restart');
> I have needed to do a number of these version checks to get PostgresNode 
> working across a variety of versions.  Attached is a WIP patch set with those 
> changes, and with a framework that exercises PostgresNode and can be extended 
> to check other things.  For now, it just checks that init(), start(), 
> safe_psql(), and teardown_node() work.
>
> With the existing changes to PostgresNode in 0001, the framework in 0002 
> works for server versions back to 9.3.  Versions 9.1 and 9.2 fail on the 
> safe_psql(), and I haven't dug into that far enough yet to explain why.  
> Versions 8.4 and 9.0 fail on the start().  I had trouble getting versions of 
> postgres older than 8.4 to compile on my laptop.  I haven't dug far enough 
> into that yet, either.
>
> To get this running, you need to install the versions you care about and edit 
> src/test/modules/test_cross_version/version.dat with the names and locations 
> of those installations.  (I committed the patch with my local settings, so 
> you can easily compare and edit.)  That should get you to the point where you 
> can run 'make check' in the test_cross_version directory.


I've had a look at the first of these patches. I think it's generally
ok, but:


-    TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
+    TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust',
+        $self->at_least_version("9.3") ? '-N' : (),
     @{ $params{extra} });


I'd rather do this in two steps to make it clearer.


I still think just doing pg_ctl -w unconditionally would be simpler.


Prior to 9.3 "unix_socket_directories" was spelled
"unix_socket_directory". We should just set a variable appropriately and
use it. That should make the changes around that a whole lot simpler.
(c.f. buildfarm code)


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 31, 2021, at 1:07 PM, Mark Dilger  wrote:
> 
> 
> 
>> On Mar 31, 2021, at 1:05 PM, Andrew Dunstan  wrote:
>> 
>> 
>> On 3/31/21 3:48 PM, Alvaro Herrera wrote:
>>> On 2021-Mar-31, Mark Dilger wrote:
>>> 
 PostgresNode::start() doesn't work for servers older than version 10,
 either.  If I hack that function to sleep until the postmaster.pid
 file exists, it works, but that is really ugly and is just to prove to
 myself that it is a timing issue.  There were a few commits in the
 version 10 development cycle (cf, commit
 f13ea95f9e473a43ee4e1baeb94daaf83535d37c) which changed how pg_ctl
 works, though I haven't figured out yet exactly what the interaction
 with PostgresNode would be.  I'll keep looking.
>>> Do you need to do "pg_ctl -w" perhaps?
>> 
>> 
>> 
>> Probably. The buildfarm does this unconditionally and has done for a
>> very long time, so maybe we don't need a version test for it.
> 
> I put a version test for this and it works for me.  I guess you could do it 
> unconditionally, if you want, but the condition is just:
> 
> -   TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
> +   TestLib::system_or_bail('pg_ctl',
> +   $self->older_than_version('10') ? '-w' : (),
> +   '-D', $pgdata, '-l', $logfile,
>'restart');

I have needed to do a number of these version checks to get PostgresNode 
working across a variety of versions.  Attached is a WIP patch set with those 
changes, and with a framework that exercises PostgresNode and can be extended 
to check other things.  For now, it just checks that init(), start(), 
safe_psql(), and teardown_node() work.

With the existing changes to PostgresNode in 0001, the framework in 0002 works 
for server versions back to 9.3.  Versions 9.1 and 9.2 fail on the safe_psql(), 
and I haven't dug into that far enough yet to explain why.  Versions 8.4 and 
9.0 fail on the start().  I had trouble getting versions of postgres older than 
8.4 to compile on my laptop.  I haven't dug far enough into that yet, either.

To get this running, you need to install the versions you care about and edit 
src/test/modules/test_cross_version/version.dat with the names and locations of 
those installations.  (I committed the patch with my local settings, so you can 
easily compare and edit.)  That should get you to the point where you can run 
'make check' in the test_cross_version directory.



v1-0001-Extending-PostgresNode-cross-version-functionalit.patch
Description: Binary data


v1-0002-Adding-modules-test_cross_version.patch
Description: Binary data

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 3/30/21 8:52 PM, Michael Paquier wrote:
> On Tue, Mar 30, 2021 at 08:44:26PM -0400, Andrew Dunstan wrote:
>> Yeah, it should be validated. All things considered I think just calling
>> 'pg_config --version' is probably the simplest validation, and likely to
>> be sufficient.
>>
>> I'll try to come up with something tomorrow.
> There is already TestLib::check_pg_config().  Shouldn't you leverage
> that with PG_VERSION_NUM or equivalent?



TBH, TestLib::check_pg_config looks like a bit of a wart, and I would be
tempted to remove it. It's the only Postgres-specific thing in
TestLib.pm I think. It's only used in one place AFAICT
(src/test/ssl/t/002_scram.pl) so we could just remove it and inline the
code.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 31, 2021, at 1:05 PM, Andrew Dunstan  wrote:
> 
> 
> On 3/31/21 3:48 PM, Alvaro Herrera wrote:
>> On 2021-Mar-31, Mark Dilger wrote:
>> 
>>> PostgresNode::start() doesn't work for servers older than version 10,
>>> either.  If I hack that function to sleep until the postmaster.pid
>>> file exists, it works, but that is really ugly and is just to prove to
>>> myself that it is a timing issue.  There were a few commits in the
>>> version 10 development cycle (cf, commit
>>> f13ea95f9e473a43ee4e1baeb94daaf83535d37c) which changed how pg_ctl
>>> works, though I haven't figured out yet exactly what the interaction
>>> with PostgresNode would be.  I'll keep looking.
>> Do you need to do "pg_ctl -w" perhaps?
> 
> 
> 
> Probably. The buildfarm does this unconditionally and has done for a
> very long time, so maybe we don't need a version test for it.

I put a version test for this and it works for me.  I guess you could do it 
unconditionally, if you want, but the condition is just:

-   TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+   TestLib::system_or_bail('pg_ctl',
+   $self->older_than_version('10') ? '-w' : (),
+   '-D', $pgdata, '-l', $logfile,
'restart');


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 3/31/21 3:48 PM, Alvaro Herrera wrote:
> On 2021-Mar-31, Mark Dilger wrote:
>
>> PostgresNode::start() doesn't work for servers older than version 10,
>> either.  If I hack that function to sleep until the postmaster.pid
>> file exists, it works, but that is really ugly and is just to prove to
>> myself that it is a timing issue.  There were a few commits in the
>> version 10 development cycle (cf, commit
>> f13ea95f9e473a43ee4e1baeb94daaf83535d37c) which changed how pg_ctl
>> works, though I haven't figured out yet exactly what the interaction
>> with PostgresNode would be.  I'll keep looking.
> Do you need to do "pg_ctl -w" perhaps?



Probably. The buildfarm does this unconditionally and has done for a
very long time, so maybe we don't need a version test for it.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Mar-31, Mark Dilger wrote:

> PostgresNode::start() doesn't work for servers older than version 10,
> either.  If I hack that function to sleep until the postmaster.pid
> file exists, it works, but that is really ugly and is just to prove to
> myself that it is a timing issue.  There were a few commits in the
> version 10 development cycle (cf, commit
> f13ea95f9e473a43ee4e1baeb94daaf83535d37c) which changed how pg_ctl
> works, though I haven't figured out yet exactly what the interaction
> with PostgresNode would be.  I'll keep looking.

Do you need to do "pg_ctl -w" perhaps?

-- 
Álvaro Herrera   Valdivia, Chile

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 30, 2021, at 5:41 PM, Mark Dilger  wrote:
> 
> 1) PostgresNode::init() doesn't work for older server versions


PostgresNode::start() doesn't work for servers older than version 10, either.  
If I hack that function to sleep until the postmaster.pid file exists, it 
works, but that is really ugly and is just to prove to myself that it is a 
timing issue.  There were a few commits in the version 10 development cycle 
(cf, commit f13ea95f9e473a43ee4e1baeb94daaf83535d37c) which changed how pg_ctl 
works, though I haven't figured out yet exactly what the interaction with 
PostgresNode would be.  I'll keep looking.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Tue, Mar 30, 2021 at 11:06:55PM -0300, Alvaro Herrera wrote:
> On 2021-Mar-31, Michael Paquier wrote:
>> There is already TestLib::check_pg_config().  Shouldn't you leverage
>> that with PG_VERSION_NUM or equivalent?
> 
> hmm, I wonder if we shouldn't take the stance that it is not TestLib's
> business to be calling any Pg binaries.  So that routine should be moved
> to PostgresNode.

Hmm.  I can see the intention, but I am not sure that this is
completely correct either to assume that we require a backend node
when tests just do tests with frontends in standalone, aka with
TestLib::program_*.
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Mar-31, Michael Paquier wrote:

> There is already TestLib::check_pg_config().  Shouldn't you leverage
> that with PG_VERSION_NUM or equivalent?

hmm, I wonder if we shouldn't take the stance that it is not TestLib's
business to be calling any Pg binaries.  So that routine should be moved
to PostgresNode.



-- 
Álvaro Herrera39°49'30"S 73°17'W

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 30, 2021, at 5:52 PM, Michael Paquier  wrote:
> 
> On Tue, Mar 30, 2021 at 08:44:26PM -0400, Andrew Dunstan wrote:
>> Yeah, it should be validated. All things considered I think just calling
>> 'pg_config --version' is probably the simplest validation, and likely to
>> be sufficient.
>> 
>> I'll try to come up with something tomorrow.
> 
> There is already TestLib::check_pg_config().  Shouldn't you leverage
> that with PG_VERSION_NUM or equivalent?

Only if you change that function.  It doesn't currently do anything special to 
run the *right* pg_config.

The patch I sent takes the view that once the install_path has been sanity 
checked and the *right* pg_config executed, relying on the environment's path 
variables thereafter is safe.  But that means the initial pg_config execution 
is unique in not being able to rely on the path.  There really isn't enough 
motivation for changing TestLib, I don't think, because subsequent calls to 
pg_config don't need to be paranoid, just the first call.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Michael Paquier]

On Tue, Mar 30, 2021 at 08:44:26PM -0400, Andrew Dunstan wrote:
> Yeah, it should be validated. All things considered I think just calling
> 'pg_config --version' is probably the simplest validation, and likely to
> be sufficient.
> 
> I'll try to come up with something tomorrow.

There is already TestLib::check_pg_config().  Shouldn't you leverage
that with PG_VERSION_NUM or equivalent?
--
Michael


signature.asc
Description: PGP signature

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 30, 2021, at 5:44 PM, Andrew Dunstan  wrote:
> 
> I'll try to come up with something tomorrow.

I hope the patch I sent is useful, at least as a starting point.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 3/30/21 6:22 PM, Alvaro Herrera wrote:
> On 2021-Mar-30, Mark Dilger wrote:
>
>> Once you have a node running, you can query the version using
>> safe_psql, but that clearly doesn't work soon enough, since we need
>> the information prior to running initdb.
> I was thinking something like examining some file in the install dir --
> say, include/server/pg_config.h, but that seems messier than just
> running pg_config.
>
>> One of the things I noticed while playing with this new toy (thanks,
>> Andrew!) 


(I'm really happy someone is playing with it so soon.)



>> is that if you pass a completely insane install_path, you
>> don't get any errors.  In fact, you get executables and libraries from
>> whatever PATH="/no/such/postgres:$PATH" gets you, probably the
>> executables and libraries of your latest development branch.  By
>> forcing get_new_node to call the pg_config of the path you pass in,
>> you'd fix that problem.  I didn't do that, mind you, but you could.  I
>> just executed pg_config, which means you'll still get the wrong
>> version owing to the PATH confusion.
> Hmm, I think it should complain if you give it a path that doesn't
> actually contain a valid installation.
>


Yeah, it should be validated. All things considered I think just calling
'pg_config --version' is probably the simplest validation, and likely to
be sufficient.


I'll try to come up with something tomorrow.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 30, 2021, at 3:22 PM, Alvaro Herrera  wrote:
> 
>> One of the things I noticed while playing with this new toy (thanks,
>> Andrew!) is that if you pass a completely insane install_path, you
>> don't get any errors.  In fact, you get executables and libraries from
>> whatever PATH="/no/such/postgres:$PATH" gets you, probably the
>> executables and libraries of your latest development branch.  By
>> forcing get_new_node to call the pg_config of the path you pass in,
>> you'd fix that problem.  I didn't do that, mind you, but you could.  I
>> just executed pg_config, which means you'll still get the wrong
>> version owing to the PATH confusion.
> 
> Hmm, I think it should complain if you give it a path that doesn't
> actually contain a valid installation.

I felt the same way, but wondered if Andrew had set path variables without 
sanity checking the install_path argument for some specific reason, and didn't 
want to break something he did intentionally.  If that wasn't intentional, then 
there are two open bugs/infelicities against master:

1) PostgresNode::init() doesn't work for older server versions

2) PostgresNode::get_new_node() doesn't reject invalid paths, resulting in 
confusion about which binaries subsequently get executed

I think this next version of the patch addresses both issues.  The first issue 
was already fixed in the previous patch.  The second issue is also now fixed by 
forcing the usage of the install_path qualified pg_config executable, rather 
than using whatever pg_config happens to be found in the path.

There is an existing issue that if you configure with 
--bindir=$SOMEWHERE_UNEXPECTED, PostgresNode won't work.  It inserts 
${install_path}/bin and ${install_path}/lib into the environment without regard 
for whether "bin" and "lib" are correct.  That's a pre-existing limitation, and 
I'm not complaining, but just commenting that I didn't do anything to fix it.

Keeping the WIP marking on the patch until we hear Andrew's opinion on all this.



v2-0001-Extending-PostgresNode-cross-version-functionalit.patch.WIP
Description: Binary data


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Mar-30, Mark Dilger wrote:

> Once you have a node running, you can query the version using
> safe_psql, but that clearly doesn't work soon enough, since we need
> the information prior to running initdb.

I was thinking something like examining some file in the install dir --
say, include/server/pg_config.h, but that seems messier than just
running pg_config.

> One of the things I noticed while playing with this new toy (thanks,
> Andrew!) is that if you pass a completely insane install_path, you
> don't get any errors.  In fact, you get executables and libraries from
> whatever PATH="/no/such/postgres:$PATH" gets you, probably the
> executables and libraries of your latest development branch.  By
> forcing get_new_node to call the pg_config of the path you pass in,
> you'd fix that problem.  I didn't do that, mind you, but you could.  I
> just executed pg_config, which means you'll still get the wrong
> version owing to the PATH confusion.

Hmm, I think it should complain if you give it a path that doesn't
actually contain a valid installation.

-- 
Álvaro Herrera   Valdivia, Chile
"But static content is just dynamic content that isn't moving!"
http://smylers.hates-software.com/2007/08/15/fe244d0c.html

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 30, 2021, at 3:12 PM, Alvaro Herrera  wrote:
> 
> On 2021-Mar-30, Mark Dilger wrote:
> 
>> The problem is clear enough; -N/--nosync was added in 9.3, and
>> PostgresNode::init is passing -N to initdb unconditionally. I wonder
>> if during PostgresNode::new a call should be made to pg_config and the
>> version information grep'd out so that version specific options to
>> various functions (init, backup, etc) could hinge on the version of
>> postgres being used?
> 
> Yeah, I think making it backwards-compatible would be good.  Is running
> pg_config to obtain the version the best way to do it?  I'm not sure --
> what other ways are there?  I can't of anything.  (Asking the user seems
> right out.)

Once you have a node running, you can query the version using safe_psql, but 
that clearly doesn't work soon enough, since we need the information prior to 
running initdb.

One of the things I noticed while playing with this new toy (thanks, Andrew!) 
is that if you pass a completely insane install_path, you don't get any errors. 
 In fact, you get executables and libraries from whatever 
PATH="/no/such/postgres:$PATH" gets you, probably the executables and libraries 
of your latest development branch.  By forcing get_new_node to call the 
pg_config of the path you pass in, you'd fix that problem.  I didn't do that, 
mind you, but you could.  I just executed pg_config, which means you'll still 
get the wrong version owing to the PATH confusion.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Mar-30, Mark Dilger wrote:

> The problem is clear enough; -N/--nosync was added in 9.3, and
> PostgresNode::init is passing -N to initdb unconditionally. I wonder
> if during PostgresNode::new a call should be made to pg_config and the
> version information grep'd out so that version specific options to
> various functions (init, backup, etc) could hinge on the version of
> postgres being used?

Yeah, I think making it backwards-compatible would be good.  Is running
pg_config to obtain the version the best way to do it?  I'm not sure --
what other ways are there?  I can't of anything.  (Asking the user seems
right out.)

-- 
Álvaro Herrera39°49'30"S 73°17'W
Officer Krupke, what are we to do?
Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 30, 2021, at 10:39 AM, Mark Dilger  
> wrote:
> 
> Andrew,
> 
> While developing some cross version tests, I noticed that PostgresNode::init 
> fails for postgres versions older than 9.3, like so:
> 
> # Checking port 52814
> # Found port 52814
> Name: 9.2.24
> Data directory: 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/pgdata
> Backup directory: 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/backup
> Archive directory: 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/archives
> Connection string: port=52814 
> host=/var/folders/6n/3f4vwbnn7fz5qk0xqhgbjrkwgp/T/L_A2w1x7qb
> Log file: 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/log/001_verify_paths_9.2.24.log
> # Running: initdb -D 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/pgdata
>  -A trust -N
> initdb: invalid option -- N
> Try "initdb --help" for more information.
> Bail out!  system initdb failed
> 
> The problem is clear enough; -N/--nosync was added in 9.3, and 
> PostgresNode::init is passing -N to initdb unconditionally. I wonder if 
> during PostgresNode::new a call should be made to pg_config and the version 
> information grep'd out so that version specific options to various functions 
> (init, backup, etc) could hinge on the version of postgres being used?
> 
> You could also just remove the -N option, but that would slow down all tests 
> for everybody, so I'm not keen to do that.  Or you could remove -N in cases 
> where $node->{_install_path} is defined, which would be far more acceptable.  
> I'm leaning towards using the output of pg_config, though, since this problem 
> is likely to come up again with other options/commands.
> 
> Thoughts?

I fixed this largely as outlined above, introducing a few new functions which 
ease test development and using one of them to condition the behavior of init() 
on the postgres version.

In the tests I have been developing (not included), the developer (or some 
buildfarm script) has to list all postgresql installations in a configuration 
file, like so:

/Users/mark.dilger/install/8.4
/Users/mark.dilger/install/9.0.23
/Users/mark.dilger/install/9.1.24
/Users/mark.dilger/install/9.2.24
/Users/mark.dilger/install/9.3.25
/Users/mark.dilger/install/9.4.26
/Users/mark.dilger/install/9.5.25
/Users/mark.dilger/install/9.6
/Users/mark.dilger/install/10
/Users/mark.dilger/install/11
/Users/mark.dilger/install/12
/Users/mark.dilger/install/13

The tests can't be hardcoded to know anything about which specific postgres 
versions will be installed, or what version of postgres exists in any 
particular install directory.  It makes the tests easier to maintain if they 
can do stuff like:

   $node{$_} = PostgresNode->get_new_node(...) for (@installed_versions);
   
   if ($node{a}->newer_than_version($node{b}))
   {
  # check that newer version A's whatever can connect to and work with 
older server B
 
   }

I therefore included functions of that sort in the patch along with the 
$node->at_least_version(version) function that the fix uses.



v1-0001-Extending-PostgresNode-cross-version-functionalit.patch.WIP
Description: Binary data


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company