Re: multi-install PostgresNode fails with older postgres versions

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