multi-install PostgresNode fails with older postgres versions

On Mar 30, 2021, at 10:39 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

Andrew,

While developing some cross version tests, I noticed that PostgresNode::init fails for postgres versions older than 9.3, like so:

# Checking port 52814
# Found port 52814
Name: 9.2.24
Data directory: /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/pgdata
Backup directory: /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/backup
Archive directory: /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/archives
Connection string: port=52814 host=/var/folders/6n/3f4vwbnn7fz5qk0xqhgbjrkw0000gp/T/L_A2w1x7qb
Log file: /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/log/001_verify_paths_9.2.24.log
# Running: initdb -D /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/pgdata -A trust -N
initdb: invalid option -- N
Try "initdb --help" for more information.
Bail out! system initdb failed

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

You could also just remove the -N option, but that would slow down all tests for everybody, so I'm not keen to do that. Or you could remove -N in cases where $node->{_install_path} is defined, which would be far more acceptable. I'm leaning towards using the output of pg_config, though, since this problem is likely to come up again with other options/commands.

Thoughts?

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

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

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

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

$node{$_} = PostgresNode->get_new_node(...) for (@installed_versions);

if ($node{a}->newer_than_version($node{b}))
{
# check that newer version A's whatever can connect to and work with older server B
....
}

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

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, since we need the information prior to running initdb.

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

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

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 for some specific reason, and didn't want to break something he did intentionally. If that wasn't intentional, then there are two open bugs/infelicities against master:

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

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

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

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

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

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

On Mar 30, 2021, at 5:52 PM, Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Mar 30, 2021 at 08:44:26PM -0400, Andrew Dunstan wrote:

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

I'll try to come up with something tomorrow.

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

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

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

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

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

On Mar 30, 2021, at 5:41 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

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

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

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

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 condition is just:

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

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

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 condition is just:

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

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

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

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

On 3/31/21 10:28 PM, Mark Dilger wrote:

On Mar 31, 2021, at 1:07 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

On Mar 31, 2021, at 1:05 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 3/31/21 3:48 PM, Alvaro Herrera wrote:

On 2021-Mar-31, Mark Dilger wrote:

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

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

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

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

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

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

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

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

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

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

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

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

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

cheers

andrew

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

On Apr 3, 2021, at 11:01 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

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

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

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

Changed.

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

Changed.

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

Ah, good to know. Changed.

Other changes:

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

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

use PostgresNode;

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

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

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

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

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