Re: multi-install PostgresNode fails with older postgres versions

> On Apr 7, 2021, at 8:43 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>
> On 4/7/21 1:03 AM, Mark Dilger wrote:
>> The v1 patch supported postgres versions back to 8.4, but v2 pushes that back to 8.1.
>>
>> The version of PostgresNode currently committed relies on IPC::Run in a way that is subtly wrong. The first time IPC::Run::run(X, ...) is called, it uses the PATH as it exists at that time, resolves the path for X, and caches it. Subsequent calls to IPC::Run::run(X, ...) use the cached path, without respecting changes to $ENV{PATH}. In practice, this means that:
>>
>> use PostgresNode;
>>
>> my $a = PostgresNode->get_new_node('a', install_path => '/my/install/8.4');
>> my $b = PostgresNode->get_new_node('b', install_path => '/my/install/9.0');
>>
>> $a->safe_psql(...) # <=== Resolves and caches 'psql' as /my/install/8.4/bin/psql
>>
>> $b->safe_psql(...) # <=== Executes /my/install/8.4/bin/psql, not /my/install/9.0/bin/psql as one might expect
>>
>> PostgresNode::safe_psql() and PostgresNode::psql() both suffer from this, and similarly PostgresNode::pg_recvlogical_upto() because the path to pg_recvlogical gets cached. Calls to initdb and pg_ctl do not appear to suffer this problem, as they are ultimately handled by perl's system() call, not by IPC::Run::run.
>>
>> Since postgres commands work fairly similarly from one release to another, this can cause subtle and hard to diagnose bugs in regression tests. The fix in v2-0001 works for me, as demonstrated by v2-0002, but whether the fix in the attached v2 patch set gets used or not, I think something needs to be done to fix this.
>>
>>
>
> Awesome work. The IPC::Run behaviour is darned unfriendly, and AFAICS
> completely undocumented. It can't even be easily modified by a client
> because the cache is stashed in a lexical variable.

Yes, I noticed that, too. Even if we could get a patch accepted into IPC::Run, we'd need to be compatible with older versions. So there doesn't seem to be any option but to work around the issue.

> You fix looks good.

Thanks for reviewing!

> other notes:
>
>
> . needs a perltidy run, some lines are too long (see
> src/tools/pgindent/perltidyrc)
>
>
> . Please use an explicit return here:
>
>
> + # Return an array reference
> + [ @result ];

Done.

> . I'm not sure the computation in _pg_version_cmp is right. What if the
> number of elements differ? As I read it you would return 0 for a
> comparison of '1.2' and '1.2.3'. Is that what's intended?

Yes, that is intended. '1.2' and '1.2.0' are not the same. '1.2' means "some unspecified micro release or development version of 1.2", whereas '1.2.0' does not.

This is useful for things like $node->at_least_version("13") returning true for development versions of version 13, which are otherwise less than (not equal to) 13.0

> . The second patch has a bunch of stuff it doesn't need. The control
> file should be unnecessary as should all the lines above 'ifdef
> USE_PGXS' in the Makefile except 'TAP_TESTS = 1'

Done.

Yeah, I started the second patch as simply a means of testing the first and didn't clean it up after copying boilerplate from elsewhere. The second patch has turned into something possibly worth keeping in its own right, and having the build farm execute on a regular basis.

> . the test script should have a way of passing a non-default version
> file to CrossVersion::nodes(). Possible get it from an environment variable?

Good idea. I changed it to use $ENV{PG_TEST_VERSIONS_FILE}. I'm open to other names for this variable.

> . I'm somewhat inclined to say that CrossVersion should just return a
> {name => path} map, and let the client script do the node creation. Then
> crossVersion doesn't need to know anything much about the
> infrastructure. But I could possibly be persuaded otherwise. Also, maybe
> it belongs in src/test/perl.

Hmm. That's a good thought. I've moved it to src/test/perl with the change you suggest.

> . This line appears deundant, the variable is not referenced:
>
>
> + my $path = $ENV{PATH};

Removed.

> Also these lines at the bottom of CrossVersion.pm are redundant:
>
>
> +use strict;
> +use warnings;

Yeah, those are silly. Removed.

This patch has none of the Object Oriented changes Alvaro and Jehan-Guillaume requested, but that should not be construed as an argument against their request. It's just not handled in this particular patch.