Re: multi-install PostgresNode fails with older postgres versions
От | Andrew Dunstan |
---|---|
Тема | Re: multi-install PostgresNode fails with older postgres versions |
Дата | |
Msg-id | 1ff0f372-503d-074a-6a09-912c8cf20bb3@dunslane.net обсуждение исходный текст |
Ответ на | Re: multi-install PostgresNode fails with older postgres versions (Mark Dilger <mark.dilger@enterprisedb.com>) |
Ответы |
Re: multi-install PostgresNode fails with older postgres versions
(Mark Dilger <mark.dilger@enterprisedb.com>)
|
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления:
Следующее
От: Peter GeogheganДата:
Сообщение: Re: New IndexAM API controlling index vacuum strategies