Thread: multi-install PostgresNode fails with older postgres versions
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
-Atrust -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'dout so that version specific options to various functions (init, backup, etc) could hinge on the version of
postgresbeing 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
leaningtowards 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
> 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
-Atrust -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'dout so that version specific options to various functions (init, backup, etc) could hinge on the version of
postgresbeing 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
leaningtowards 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
themto 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
installationsin 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
ofpostgres 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
thefix uses.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
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")
> 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, sincewe 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 forcingget_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, butyou 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
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
> 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
forsome specific reason, and didn't want to break something he did intentionally. If that wasn't intentional, then
thereare 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
getexecuted
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
usingwhatever 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
fixit.
Keeping the WIP marking on the patch until we hear Andrew's opinion on all this.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
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
> 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
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
Attachment
> 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, relyingon the environment's path variables thereafter is safe. But that means the initial pg_config execution is uniquein not being able to rely on the path. There really isn't enough motivation for changing TestLib, I don't think, becausesubsequent 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
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
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
Attachment
> 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 thepostmaster.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 theinteraction with PostgresNode would be. I'll keep looking. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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
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
> 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
conditionis 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
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
> 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
conditionis 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.
Attachedis a WIP patch set with those changes, and with a framework that exercises PostgresNode and can be extended to
checkother 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.
Versions9.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
and9.0 fail on the start(). I had trouble getting versions of postgres older than 8.4 to compile on my laptop. I
haven'tdug 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.datwith the names and locations of those installations. (I committed the
patchwith my local settings, so you can easily compare and edit.) That should get you to the point where you can run
'makecheck' in the test_cross_version directory.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
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
conditionis 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.
Attachedis a WIP patch set with those changes, and with a framework that exercises PostgresNode and can be extended to
checkother 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.
Versions9.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
and9.0 fail on the start(). I had trouble getting versions of postgres older than 8.4 to compile on my laptop. I
haven'tdug 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.datwith the names and locations of those installations. (I committed the
patchwith my local settings, so you can easily compare and edit.) That should get you to the point where you can run
'makecheck' 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
> 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.
Subsequentcalls to IPC::Run::run(X, ...) use the cached path, without respecting changes to $ENV{PATH}. In practice,
thismeans 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
appearto 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
bugsin regression tests. The fix in v2-0001 works for me, as demonstrated by v2-0002, but whether the fix in the
attachedv2 patch set gets used or not, I think something needs to be done to fix this.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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¢
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.
Subsequentcalls to IPC::Run::run(X, ...) use the cached path, without respecting changes to $ENV{PATH}. In practice,
thismeans 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
appearto 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
bugsin regression tests. The fix in v2-0001 works for me, as demonstrated by v2-0002, but whether the fix in the
attachedv2 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
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
> 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
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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,
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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,
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
> 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. Itis hard to quickly review your changes, as they are all mixed together with a mass of copied code from the main project. I'll look some more for parts that I could reuse. Thanks — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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,
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
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
> 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 versionsof postgres on that same development branch have been patched. If you only ever spin up the latest version, youcan'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 doesthat help? If you have ten different versions of "10" in your test, they all look the same? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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,
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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,
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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,
> 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
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.
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)
> 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
usedas 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
is10.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
derivedclasses. It would make more sense to me if the version method worked the way I implemented it, where it just
returnsthe version obtained from pg_config
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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
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
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".
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
> 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
andhas 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
implementeddifferently in one class than another. I think the subclassing solution is cleaner if the number of version
testsis 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
functionsthat work a certain way, but cannot work that same way with older versions of postgres that don't have the
necessarysupport. 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
avoidedit. 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.
Priorto Andrew's recent introduction of support for alternate installation paths, it made sense to have
restart_after_crashbe 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
wantit to do*?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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
limitationsand 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
implementeddifferently in one class than another. I think the subclassing solution is cleaner if the number of version
testsis 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
functionsthat work a certain way, but cannot work that same way with older versions of postgres that don't have the
necessarysupport. 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
avoidedit. 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_crashbe 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
wantit 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
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
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)
> 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.
Myproblems 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.
Ofcourse the behaviors change to the extent that postgres itself changed between versions. I mean changes because
PostgresNodebehaves 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
> 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.
Subsequentcalls to IPC::Run::run(X, ...) use the cached path, without respecting changes to $ENV{PATH}. In practice,
thismeans 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
appearto 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
diagnosebugs in regression tests. The fix in v2-0001 works for me, as demonstrated by v2-0002, but whether the fix in
theattached 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
versionof 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,
whichare 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
boilerplatefrom elsewhere. The second patch has turned into something possibly worth keeping in its own right, and
havingthe 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
construedas an argument against their request. It's just not handled in this particular patch.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
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
Attachment
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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,
Attachment
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
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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,
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
Attachment
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
Attachment
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
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
Attachment
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
> 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 olderservers, create data that cannot be created on newer servers (such as heap tuples with HEAP_MOVED_IN or HEAP_MOVED_OFFbits set), upgrade, and test that new code handles the old data correctly. I think this is not only usefulfor our test suites as a community, but is also useful for companies providing support services who need to reproduceproblems 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
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 olderservers, create data that cannot be created on newer servers (such as heap tuples with HEAP_MOVED_IN or HEAP_MOVED_OFFbits set), upgrade, and test that new code handles the old data correctly. I think this is not only usefulfor our test suites as a community, but is also useful for companies providing support services who need to reproduceproblems 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
Attachment
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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.
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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?
> 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 bugsthat can only be triggered by cross version upgrades, the scripts need a PostgresNode implementation which can work acrossversions. Likewise for bugs that can only be triggered when client applications connect to servers of a differentversion. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: multi-install PostgresNode fails with older postgres versions
From
Jehan-Guillaume de Rorthais
Date:
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
> 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
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
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
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
> 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) ",
andstrip 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
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) ",
andstrip 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
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
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
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
Attachment
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
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
Attachment
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
Attachment
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
Attachment
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
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.
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
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
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
Attachment
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.
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
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
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
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
Attachment
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
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
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.
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