On 4/7/21 1:03 AM, Mark Dilger wrote:
> The v1 patch supported postgres versions back to 8.4, but v2 pushes that back
> to 8.1.
>
> The version of PostgresNode currently committed relies on IPC::Run in a way
> that is subtly wrong. The first time IPC::Run::run(X, ...) is called, it
> uses the PATH as it exists at that time, resolves the path for X, and caches
> it. Subsequent calls to IPC::Run::run(X, ...) use the cached path, without
> respecting changes to $ENV{PATH}. In practice, this means that:
>
> use PostgresNode;
>
> my $a = PostgresNode->get_new_node('a', install_path => '/my/install/8.4');
> my $b = PostgresNode->get_new_node('b', install_path => '/my/install/9.0');
>
> $a->safe_psql(...) # <=== Resolves and caches 'psql' as
> /my/install/8.4/bin/psql
>
> $b->safe_psql(...) # <=== Executes /my/install/8.4/bin/psql, not
> /my/install/9.0/bin/psql as one might expect
>
> PostgresNode::safe_psql() and PostgresNode::psql() both suffer from this, and
> similarly PostgresNode::pg_recvlogical_upto() because the path to
> pg_recvlogical gets cached. Calls to initdb and pg_ctl do not appear to
> suffer this problem, as they are ultimately handled by perl's system() call,
> not by IPC::Run::run.
>
> Since postgres commands work fairly similarly from one release to another,
> this can cause subtle and hard to diagnose bugs in regression tests. The fix
> in v2-0001 works for me, as demonstrated by v2-0002, but whether the fix in
> the attached v2 patch set gets used or not, I think something needs to be
> done to fix this.
>
>
Awesome work. The IPC::Run behaviour is darned unfriendly, and AFAICS
completely undocumented. It can't even be easily modified by a client
because the cache is stashed in a lexical variable. You fix looks good.
other notes:
. needs a perltidy run, some lines are too long (see
src/tools/pgindent/perltidyrc)
. Please use an explicit return here:
+ # Return an array reference
+ [ @result ];
. I'm not sure the computation in _pg_version_cmp is right. What if the
number of elements differ? As I read it you would return 0 for a
comparison of '1.2' and '1.2.3'. Is that what's intended?
. The second patch has a bunch of stuff it doesn't need. The control
file should be unnecessary as should all the lines above 'ifdef
USE_PGXS' in the Makefile except 'TAP_TESTS = 1'
. the test script should have a way of passing a non-default version
file to CrossVersion::nodes(). Possible get it from an environment variable?
. I'm somewhat inclined to say that CrossVersion should just return a
{name => path} map, and let the client script do the node creation. Then
crossVersion doesn't need to know anything much about the
infrastructure. But I could possibly be persuaded otherwise. Also, maybe
it belongs in src/test/perl.
. This line appears deundant, the variable is not referenced:
+ my $path = $ENV{PATH};
Also these lines at the bottom of CrossVersion.pm are redundant:
+use strict;
+use warnings;
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
