Horton

Thread

Commits

  1. Teach PostgresVersion all the ways to mark non-release code

  2. Make PostgresNode version aware

  3. Avoid unfortunate IPC::Run path caching in PostgresNode

  4. Change pg_ctl to detect server-ready by watching status in postmaster.pid.

  1. multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-03-30T17:39:56Z

    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/3f4vwbnn7fz5qk0xqhgbjrkw0000gp/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?

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

  2. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-03-30T21:33:36Z

    
> On Mar 30, 2021, at 10:39 AM, Mark Dilger <mark.dilger@enterprisedb.com> 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/3f4vwbnn7fz5qk0xqhgbjrkw0000gp/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.

  3. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-30T22:12:48Z

    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 Herrera                            39°49'30"S 73°17'W
Officer Krupke, what are we to do?
Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")

  4. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-03-30T22:17:26Z

    
> On Mar 30, 2021, at 3:12 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  5. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-30T22:22:01Z

    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

  6. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-03-31T00:41:56Z

    
> On Mar 30, 2021, at 3:22 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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.

  7. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-31T00:44:26Z

    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

  8. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-03-31T00:50:14Z

    
> On Mar 30, 2021, at 5:44 PM, Andrew Dunstan <andrew@dunslane.net> 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

  9. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-03-31T00:52:35Z

    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

  10. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-03-31T00:59:08Z

    
> On Mar 30, 2021, at 5:52 PM, Michael Paquier <michael@paquier.xyz> 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

  11. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-31T02:06:55Z

    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 Herrera                            39°49'30"S 73°17'W

  12. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-03-31T03:08:25Z

    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

  13. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-03-31T19:34:41Z

    
> On Mar 30, 2021, at 5:41 PM, Mark Dilger <mark.dilger@enterprisedb.com> 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

  14. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-31T19:48:17Z

    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

  15. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-31T20:05:23Z

    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

  16. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-03-31T20:07:48Z

    
> On Mar 31, 2021, at 1:05 PM, Andrew Dunstan <andrew@dunslane.net> 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

  17. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-31T20:45:32Z

    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

  18. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-01T02:28:56Z

    
> On Mar 31, 2021, at 1:07 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> 
> 
>> On Mar 31, 2021, at 1:05 PM, Andrew Dunstan <andrew@dunslane.net> 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.

  19. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-03T18:01:10Z

    On 3/31/21 10:28 PM, Mark Dilger wrote:
>
>> On Mar 31, 2021, at 1:07 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>>
>>
>>
>>> On Mar 31, 2021, at 1:05 PM, Andrew Dunstan <andrew@dunslane.net> 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

  20. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-07T05:03:50Z

    
> On Apr 3, 2021, at 11:01 AM, Andrew Dunstan <andrew@dunslane.net> 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.

  21. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-07T14:37:12Z

    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¢

  22. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-07T15:43:41Z

    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

  23. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-07T15:54:36Z

    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

  24. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-07T16:08:31Z

    
> On Apr 7, 2021, at 7:37 AM, Jehan-Guillaume de Rorthais <jgdr@dalibo.com> 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

  25. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-07T16:21:06Z

    On Wed, 7 Apr 2021 11:54:36 -0400
Andrew Dunstan <andrew@dunslane.net> 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,

  26. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-07T16:26:57Z

    On Wed, 7 Apr 2021 09:08:31 -0700
Mark Dilger <mark.dilger@enterprisedb.com> wrote:

> > On Apr 7, 2021, at 7:37 AM, Jehan-Guillaume de Rorthais <jgdr@dalibo.com>

> > 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,

  27. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-07T16:51:55Z

    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

  28. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-07T17:07:07Z

    
> On Apr 7, 2021, at 9:26 AM, Jehan-Guillaume de Rorthais <jgdr@dalibo.com> wrote:
> 
> On Wed, 7 Apr 2021 09:08:31 -0700
> Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
>>> On Apr 7, 2021, at 7:37 AM, Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
> 
>>> 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

  29. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-07T17:19:21Z

    On Wed, 7 Apr 2021 12:51:55 -0400
Alvaro Herrera <alvherre@alvh.no-ip.org> 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,

  30. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-07T17:36:31Z

    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

  31. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-07T17:38:39Z

    On 4/7/21 1:19 PM, Jehan-Guillaume de Rorthais wrote:
> On Wed, 7 Apr 2021 12:51:55 -0400
> Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  32. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-07T17:50:26Z

    
> On Apr 7, 2021, at 10:36 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  33. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-07T18:07:41Z

    On Wed, 7 Apr 2021 13:36:31 -0400
Alvaro Herrera <alvherre@alvh.no-ip.org> 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,

  34. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-07T18:15:40Z

    On Wed, 7 Apr 2021 10:50:26 -0700
Mark Dilger <mark.dilger@enterprisedb.com> wrote:

> > On Apr 7, 2021, at 10:36 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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,

  35. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-07T18:35:40Z

    On Wed, 7 Apr 2021 13:38:39 -0400
Andrew Dunstan <andrew@dunslane.net> wrote:

> On 4/7/21 1:19 PM, Jehan-Guillaume de Rorthais wrote:
> > On Wed, 7 Apr 2021 12:51:55 -0400
> > Alvaro Herrera <alvherre@alvh.no-ip.org> 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,

  36. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-07T18:40:05Z

    
> On Apr 7, 2021, at 11:35 AM, Jehan-Guillaume de Rorthais <jgdr@dalibo.com> wrote:
> 
> On Wed, 7 Apr 2021 13:38:39 -0400
> Andrew Dunstan <andrew@dunslane.net> wrote:
> 
>> On 4/7/21 1:19 PM, Jehan-Guillaume de Rorthais wrote:
>>> On Wed, 7 Apr 2021 12:51:55 -0400
>>> Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  37. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-07T19:09:24Z

    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 Herrera                            39°49'30"S 73°17'W
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

  38. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-07T19:13:14Z

    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 Herrera                            39°49'30"S 73°17'W
"Crear es tan difícil como ser libre" (Elsa Triolet)

  39. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-07T19:20:50Z

    
> On Apr 7, 2021, at 12:13 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  40. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-07T20:13:21Z

    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

  41. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-07T20:19:35Z

    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

  42. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-07T20:28:50Z

    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 Herrera                            39°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".

  43. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-07T20:36:45Z

    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

  44. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-07T20:48:44Z

    
> On Apr 7, 2021, at 1:28 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  45. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-07T21:02:20Z

    On 4/7/21 4:48 PM, Mark Dilger wrote:
>
>> On Apr 7, 2021, at 1:28 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  46. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-07T21:04:21Z

    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

  47. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-07T21:06:19Z

    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 Herrera                            39°49'30"S 73°17'W
"El sudor es la mejor cura para un pensamiento enfermo" (Bardia)

  48. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-07T21:34:41Z

    
> On Apr 7, 2021, at 2:04 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  49. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-08T19:07:27Z

    
> On Apr 7, 2021, at 8:43 AM, Andrew Dunstan <andrew@dunslane.net> 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.

  50. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-11T17:01:18Z

    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

  51. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-12T12:59:18Z

    Hi,

On Wed, 7 Apr 2021 20:07:41 +0200
Jehan-Guillaume de Rorthais <jgdr@dalibo.com> 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,

  52. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-12T13:52:24Z

    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 <jgdr@dalibo.com> 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

  53. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-12T14:57:12Z

    On Mon, 12 Apr 2021 09:52:24 -0400
Andrew Dunstan <andrew@dunslane.net> 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 <jgdr@dalibo.com> 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,

  54. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-17T16:31:11Z

    On 4/12/21 10:57 AM, Jehan-Guillaume de Rorthais wrote:
> On Mon, 12 Apr 2021 09:52:24 -0400
> Andrew Dunstan <andrew@dunslane.net> 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 <jgdr@dalibo.com> 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

  55. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-17T16:35:45Z

    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 <andrew@dunslane.net> 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 <jgdr@dalibo.com> 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:
<https://gitlab.com/adunstan/pg-old-bin> along with some saved by my
buildfarm animal.


cheers


andrew


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

  56. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-19T12:11:01Z

    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:
> <https://gitlab.com/adunstan/pg-old-bin> 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

  57. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-04-19T12:32:01Z

    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

  58. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-19T13:54:47Z

    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
<https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm>


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

  59. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-19T14:43:58Z

    
> On Apr 19, 2021, at 5:11 AM, Andrew Dunstan <andrew@dunslane.net> 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

  60. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-19T16:37:08Z

    On 4/19/21 10:43 AM, Mark Dilger wrote:
>
>> On Apr 19, 2021, at 5:11 AM, Andrew Dunstan <andrew@dunslane.net> 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

  61. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-19T17:02:14Z

    On Mon, 19 Apr 2021 07:43:58 -0700
Mark Dilger <mark.dilger@enterprisedb.com> wrote:

> > On Apr 19, 2021, at 5:11 AM, Andrew Dunstan <andrew@dunslane.net> 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.

  62. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-19T17:25:44Z

    On Mon, 19 Apr 2021 12:37:08 -0400
Andrew Dunstan <andrew@dunslane.net> wrote:

> 
> On 4/19/21 10:43 AM, Mark Dilger wrote:
> >
> >> On Apr 19, 2021, at 5:11 AM, Andrew Dunstan <andrew@dunslane.net> 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?

  63. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-19T17:35:39Z

    
> On Apr 19, 2021, at 10:25 AM, Jehan-Guillaume de Rorthais <jgdr@dalibo.com> wrote:
> 
> On Mon, 19 Apr 2021 12:37:08 -0400
> Andrew Dunstan <andrew@dunslane.net> wrote:
> 
>> 
>> On 4/19/21 10:43 AM, Mark Dilger wrote:
>>> 
>>>> On Apr 19, 2021, at 5:11 AM, Andrew Dunstan <andrew@dunslane.net> 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

  64. Re: multi-install PostgresNode fails with older postgres versions

    Jehan-Guillaume de Rorthais <jgdr@dalibo.com> — 2021-04-19T17:50:43Z

    On Mon, 19 Apr 2021 10:35:39 -0700
Mark Dilger <mark.dilger@enterprisedb.com> wrote:

> > On Apr 19, 2021, at 10:25 AM, Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
> > wrote:
> > 
> > On Mon, 19 Apr 2021 12:37:08 -0400
> > Andrew Dunstan <andrew@dunslane.net> wrote:
> >   
> >> 
> >> On 4/19/21 10:43 AM, Mark Dilger wrote:  
> >>>   
> >>>> On Apr 19, 2021, at 5:11 AM, Andrew Dunstan <andrew@dunslane.net> 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

  65. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-19T18:17:11Z

    
> On Apr 19, 2021, at 10:50 AM, Jehan-Guillaume de Rorthais <jgdr@dalibo.com> 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

  66. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-20T17:11:59Z

    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 <andrew@dunslane.net> 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

  67. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-04-21T05:13:07Z

    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

  68. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-21T14:04:40Z

    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

  69. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-04-22T06:52:28Z

    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

  70. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-22T12:53:52Z

    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

  71. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-22T15:09:54Z

    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 Herrera                            39°49'30"S 73°17'W

  72. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-22T15:13:20Z

    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

  73. Re: multi-install PostgresNode fails with older postgres versions

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-22T15:46:09Z

    
> On Apr 22, 2021, at 8:09 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  74. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-22T16:42:33Z

    On 4/22/21 11:46 AM, Mark Dilger wrote:
>
>> On Apr 22, 2021, at 8:09 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  75. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-22T17:58:18Z

    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

  76. Re: multi-install PostgresNode fails with older postgres versions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-22T18:35:13Z

    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 Herrera                            39°49'30"S 73°17'W

  77. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-04-22T21:08:19Z

    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

  78. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-23T00:43:10Z

    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

  79. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-04-23T04:41:26Z

    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

  80. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-23T12:10:01Z

    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

  81. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-04-24T05:54:54Z

    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

  82. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-24T13:49:07Z

    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

  83. Re: multi-install PostgresNode fails with older postgres versions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-20T08:36:07Z

    On Thu, Apr 22, 2021 at 8:43 PM Andrew Dunstan <andrew@dunslane.net> 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.

  84. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-05-20T11:05:05Z

    On 5/20/21 4:36 AM, Amit Kapila wrote:
> On Thu, Apr 22, 2021 at 8:43 PM Andrew Dunstan <andrew@dunslane.net> 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: <https://perldoc.perl.org/perl5220delta>


cheers


andrew


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

  85. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-05-20T11:13:18Z

    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 <andrew@dunslane.net> 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: <https://perldoc.perl.org/perl5220delta>
>
>

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

  86. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-05-20T11:15:27Z

    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: <https://perldoc.perl.org/perl5220delta>

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

  87. Re: multi-install PostgresNode fails with older postgres versions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-20T11:16:44Z

    On Thu, May 20, 2021 at 4:43 PM Andrew Dunstan <andrew@dunslane.net> 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: <https://perldoc.perl.org/perl5220delta>
> >
> >
>
> 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.

  88. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-05-20T12:25:45Z

    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: <https://perldoc.perl.org/perl5220delta>
> 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

  89. Re: multi-install PostgresNode fails with older postgres versions

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-20T13:53:44Z

    Michael Paquier <michael@paquier.xyz> 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: <https://perldoc.perl.org/perl5220delta>

> 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

  90. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-05-20T14:51:46Z

    On 5/20/21 9:53 AM, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> 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: <https://perldoc.perl.org/perl5220delta>
>> 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

  91. Re: multi-install PostgresNode fails with older postgres versions

    Michael Paquier <michael@paquier.xyz> — 2021-05-21T01:49:45Z

    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

  92. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-05-21T01:59:33Z

    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

  93. Re: multi-install PostgresNode fails with older postgres versions

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-21T03:04:51Z

    Andrew Dunstan <andrew@dunslane.net> 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

  94. Re: multi-install PostgresNode fails with older postgres versions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-21T03:33:13Z

    On Thu, May 20, 2021 at 5:55 PM Andrew Dunstan <andrew@dunslane.net> 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: <https://perldoc.perl.org/perl5220delta>
> > 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.

  95. Re: multi-install PostgresNode fails with older postgres versions

    Andrew Dunstan <andrew@dunslane.net> — 2021-05-21T12:29:14Z

    On 5/20/21 11:04 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> 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