Обсуждение: Teach pg_upgrade test to honor NO_TEMP_INSTALL

On some machines (*cough* Mingw *cough*) installs are very slow. We've
ameliorated this by allowing temp installs to be reused, but the
pg_upgrade Makefile never got the message. Here's a patch that does
that. I'd like to backpatch it, at least to 9.5 where we switched the
pg_upgrade location. The risk seems appropriately low and it only
affects our test regime.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Saturday, March 30, 2019 9:42 PM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:

> On some machines (cough Mingw cough) installs are very slow. We've
> ameliorated this by allowing temp installs to be reused, but the
> pg_upgrade Makefile never got the message. Here's a patch that does
> that. I'd like to backpatch it, at least to 9.5 where we switched the
> pg_upgrade location. The risk seems appropriately low and it only
> affects our test regime.

While I haven't tried the patch (yet), reading it it makes sense, so +1
on the fix. Nice catch!

cheers ./daniel

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On some machines (*cough* Mingw *cough*) installs are very slow. We've
> ameliorated this by allowing temp installs to be reused, but the
> pg_upgrade Makefile never got the message. Here's a patch that does
> that. I'd like to backpatch it, at least to 9.5 where we switched the
> pg_upgrade location. The risk seems appropriately low and it only
> affects our test regime.

I haven't tested this, but it looks reasonable.

I suspect you need double-quotes around the path values, as in
the adjacent usage of EXTRA_REGRESS_OPTS.

            regards, tom lane

Hi Andrew,

On 2019-03-30 16:42:16 -0400, Andrew Dunstan wrote:
> On some machines (*cough* Mingw *cough*) installs are very slow. We've
> ameliorated this by allowing temp installs to be reused, but the
> pg_upgrade Makefile never got the message. Here's a patch that does
> that. I'd like to backpatch it, at least to 9.5 where we switched the
> pg_upgrade location. The risk seems appropriately low and it only
> affects our test regime.

I'm confused as to why this was done as a purely optional path, rather
than just ripping out the pg_upgrade specific install?

See also discussion around https://www.postgresql.org/message-id/21766.1558397960%40sss.pgh.pa.us

Greetings,

Andres Freund

On 5/20/19 9:58 PM, Andres Freund wrote:
> Hi Andrew,
>
> On 2019-03-30 16:42:16 -0400, Andrew Dunstan wrote:
>> On some machines (*cough* Mingw *cough*) installs are very slow. We've
>> ameliorated this by allowing temp installs to be reused, but the
>> pg_upgrade Makefile never got the message. Here's a patch that does
>> that. I'd like to backpatch it, at least to 9.5 where we switched the
>> pg_upgrade location. The risk seems appropriately low and it only
>> affects our test regime.
> I'm confused as to why this was done as a purely optional path, rather
> than just ripping out the pg_upgrade specific install?
>
> See also discussion around https://www.postgresql.org/message-id/21766.1558397960%40sss.pgh.pa.us
>

By specifying NO_TEMP_INSTALL you are in effect certifying that there is
already a suitable temp install available. But that might well not be
the case. In fact, there have been several iterations of code to get the
buildfarm client to check reasonable reliably that there is such an
install before it chooses to use the flag.


Note that the buildfarm doesn't run "make check-world" for reasons I
have explained in the past. NO_TEMP_INSTALL is particularly valuable in
saving time when running the TAP tests, especially on Mingw.


cheers


andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 5/20/19 9:58 PM, Andres Freund wrote:
>> I'm confused as to why this was done as a purely optional path, rather
>> than just ripping out the pg_upgrade specific install?

> By specifying NO_TEMP_INSTALL you are in effect certifying that there is
> already a suitable temp install available. But that might well not be
> the case. In fact, there have been several iterations of code to get the
> buildfarm client to check reasonable reliably that there is such an
> install before it chooses to use the flag.

Right.  Issuing "make check" in src/bin/pg_upgrade certainly shouldn't
skip making a new install.  But if we're recursing down from a top-level
check-world, we ought to be able to use the install it made.

            regards, tom lane

Hi,

On 2019-05-21 14:48:27 -0400, Andrew Dunstan wrote:
> On 5/20/19 9:58 PM, Andres Freund wrote:
> > Hi Andrew,
> >
> > On 2019-03-30 16:42:16 -0400, Andrew Dunstan wrote:
> >> On some machines (*cough* Mingw *cough*) installs are very slow. We've
> >> ameliorated this by allowing temp installs to be reused, but the
> >> pg_upgrade Makefile never got the message. Here's a patch that does
> >> that. I'd like to backpatch it, at least to 9.5 where we switched the
> >> pg_upgrade location. The risk seems appropriately low and it only
> >> affects our test regime.
> > I'm confused as to why this was done as a purely optional path, rather
> > than just ripping out the pg_upgrade specific install?
> >
> > See also discussion around https://www.postgresql.org/message-id/21766.1558397960%40sss.pgh.pa.us
> >
> 
> By specifying NO_TEMP_INSTALL you are in effect certifying that there is
> already a suitable temp install available. But that might well not be
> the case.

But all that takes is adding a dependency to temp-install in
src/bin/pg_upgrade/Makefile's check target? Like many other regression
test? And the temp-install rule already honors NO_TEMP_INSTALL:

temp-install: | submake-generated-headers
ifndef NO_TEMP_INSTALL
ifneq ($(abs_top_builddir),)
ifeq ($(MAKELEVEL),0)
    rm -rf '$(abs_top_builddir)'/tmp_install
    $(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
    $(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install
>'$(abs_top_builddir)'/tmp_install/log/install.log2>&1
 
    $(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep
>>'$(abs_top_builddir)'/tmp_install/log/install.log2>&1
 
endif
endif
endif

I'm not saying that you shouldn't have added NO_TEMP_INSTALL support or
something, I'm confused as to why the support for custom installations
inside test.sh was retained.

Roughly like in the attached?

Greetings,

Andres Freund

Hi,

On 2019-05-21 12:19:18 -0700, Andres Freund wrote:
> Roughly like in the attached?

> -check: test.sh all
> -    MAKE=$(MAKE) bindir="$(tbindir)" libdir="$(tlibdir)" EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $<
$(DOINST)
> +check: test.sh all temp-install
> +    MAKE=$(MAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) MAKE=$(MAKE)
EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)"$(SHELL) $< $(DOINST)
 

minus the duplicated MAKE=$(MAKE) of course.

Greetings,

Andres Freund

On Wed, May 22, 2019 at 7:41 AM Andres Freund <andres@anarazel.de> wrote:>
> On 2019-05-21 12:19:18 -0700, Andres Freund wrote:
> > Roughly like in the attached?
>
> > -check: test.sh all
> > -     MAKE=$(MAKE) bindir="$(tbindir)" libdir="$(tlibdir)" EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $<
$(DOINST)
> > +check: test.sh all temp-install
> > +     MAKE=$(MAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) MAKE=$(MAKE)
EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)"$(SHELL) $< $(DOINST)
 
>
> minus the duplicated MAKE=$(MAKE) of course.

After these commits (and Tom's commit "Un-break pg_upgrade regression
test."), cfbot broke:

(using postmaster on /tmp/pg_upgrade_check-YGuskp, port 54464)
============== dropping database "regression"         ==============
sh: 1: /usr/local/pgsql/bin/psql: not found
command failed: "/usr/local/pgsql/bin/psql" -X -c "DROP DATABASE IF
EXISTS \"regression\"" "postgres"
make[1]: *** [installcheck-parallel] Error 2
make[1]: Leaving directory
`/home/travis/build/postgresql-cfbot/postgresql/src/test/regress'

Before that it had been running happily like this:

./configure --enable-debug --enable-cassert --enable-tap-tests
--with-tcl --with-python --with-perl --with-ldap --with-openssl
--with-gssapi --with-icu && echo "COPT=-Wall -Werror" >
src/Makefile.custom && make -j4 all contrib docs && make check-world

I added --prefix=$HOME/something and added "make install" before "make
check-world", and now it's happy again.  Was that expected?

-- 
Thomas Munro
https://enterprisedb.com

Thomas Munro <thomas.munro@gmail.com> writes:
> After these commits (and Tom's commit "Un-break pg_upgrade regression
> test."), cfbot broke:

> sh: 1: /usr/local/pgsql/bin/psql: not found

I can confirm that here: check-world passes as long as I've done
"make install" beforehand ... but of course that should not be
necessary.  If I blow away the install tree, pg_upgrade's
check fails at

../../../src/test/regress/pg_regress --inputdir=. --bindir='/home/postgres/testversion/bin'    --port=54464 --dlpath=.
--max-concurrent-tests=20 --port=54464 --schedule=./parallel_schedule   
(using postmaster on /tmp/pg_upgrade_check-Nitf3h, port 54464)
============== dropping database "regression"         ==============
sh: /home/postgres/testversion/bin/psql: No such file or directory
command failed: "/home/postgres/testversion/bin/psql" -X -c "DROP DATABASE IF EXISTS \"regression\"" "postgres"

pg_regress is being told the wrong --bindir, ie
the final install location not the temp install.

(More generally, should we rearrange the buildfarm test
sequence so it doesn't run "make install" till after the
tests that aren't supposed to require an installed tree?)

            regards, tom lane

Hi,

On 2019-05-22 10:58:54 -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > After these commits (and Tom's commit "Un-break pg_upgrade regression
> > test."), cfbot broke:

I should just have finished working two hours earlier yesterday :(.


> > sh: 1: /usr/local/pgsql/bin/psql: not found
> 
> I can confirm that here: check-world passes as long as I've done
> "make install" beforehand ... but of course that should not be
> necessary.  If I blow away the install tree, pg_upgrade's
> check fails at

> ../../../src/test/regress/pg_regress --inputdir=. --bindir='/home/postgres/testversion/bin'    --port=54464
--dlpath=.--max-concurrent-tests=20  --port=54464 --schedule=./parallel_schedule  
 
> (using postmaster on /tmp/pg_upgrade_check-Nitf3h, port 54464)
> ============== dropping database "regression"         ==============
> sh: /home/postgres/testversion/bin/psql: No such file or directory
> command failed: "/home/postgres/testversion/bin/psql" -X -c "DROP DATABASE IF EXISTS \"regression\"" "postgres"
> 
> pg_regress is being told the wrong --bindir, ie
> the final install location not the temp install.

Yea, that's indeed the problem. I suspect that problem already exists in
the NO_TEMP_INSTALL solution committed in
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47b3c26642e6850e8dfa7afe01db78320b11549e
It's just that nobody noticed that due to:

> (More generally, should we rearrange the buildfarm test
> sequence so it doesn't run "make install" till after the
> tests that aren't supposed to require an installed tree?)

Seems what we need to fix the immediate issue is to ressurect:

# We need to make it use psql from our temporary installation,
# because otherwise the installcheck run below would try to
# use psql from the proper installation directory, which might
# be outdated or missing. But don't override anything else that's
# already in EXTRA_REGRESS_OPTS.
EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$bindir'"
export EXTRA_REGRESS_OPTS

and put that into global scope. While that'd be unnecessary when
invoking ./test.sh from commandline with explicitly already installed
binaries, it should be harmless there.

I wonder however, shouldn't the above stanza refer to $oldbindir?

I think we need to backpatch the move of the above outside the --install
path, because otherwise the buildfarm will break once we reorder the
buildfarm's scripts to do the make install later. Unless I miss
something?


> (More generally, should we rearrange the buildfarm test
> sequence so it doesn't run "make install" till after the
> tests that aren't supposed to require an installed tree?)

Seems like a good idea. On buildfarm's master the order is:

make_check() unless $delay_check;

# contrib is built under the standard build step for msvc
make_contrib() unless ($using_msvc);

make_testmodules()
  if (!$using_msvc && ($branch eq 'HEAD' || $branch ge 'REL9_5'));

make_doc() if (check_optional_step('build_docs'));

make_install();

# contrib is installed under standard install for msvc
make_contrib_install() unless ($using_msvc);

make_testmodules_install()
  if (!$using_msvc && ($branch eq 'HEAD' || $branch ge 'REL9_5'));

make_check() if $delay_check;

process_module_hooks('configure');

process_module_hooks('build');

process_module_hooks("check") unless $delay_check;

process_module_hooks('install');

process_module_hooks("check") if $delay_check;

run_bin_tests();

run_misc_tests();

... locale tests, ecpg, typedefs

Seems like we ought to at least move run_bin_tests, run_misc_tests up?


I'm not quite sure what the idea of $delay_check is. I found:
>  * a new --delay-check switch delays the check step until after
>    install. This helps work around a bug or lack of capacity w.r.t.
>    LD_LIBRARY_PATH on Alpine Linux

in an release announcement. But no further details.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Seems what we need to fix the immediate issue is to ressurect:

> # We need to make it use psql from our temporary installation,
> # because otherwise the installcheck run below would try to
> # use psql from the proper installation directory, which might
> # be outdated or missing. But don't override anything else that's
> # already in EXTRA_REGRESS_OPTS.
> EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$bindir'"
> export EXTRA_REGRESS_OPTS

> and put that into global scope.

Not sure about that last bit.  pg_upgrade has the issue of possibly
wanting to deal with 2 installations, unlike the rest of the tree,
so I'm not sure that fixing its problem means there's something we
need to change everywhere else.

(IOW, keep an eye on the cross-version-upgrade tests while
you mess with this...)

            regards, tom lane

Hi,

On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Seems what we need to fix the immediate issue is to ressurect:
> 
> > # We need to make it use psql from our temporary installation,
> > # because otherwise the installcheck run below would try to
> > # use psql from the proper installation directory, which might
> > # be outdated or missing. But don't override anything else that's
> > # already in EXTRA_REGRESS_OPTS.
> > EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$bindir'"
> > export EXTRA_REGRESS_OPTS
> 
> > and put that into global scope.
> 
> Not sure about that last bit.  pg_upgrade has the issue of possibly
> wanting to deal with 2 installations, unlike the rest of the tree,
> so I'm not sure that fixing its problem means there's something we
> need to change everywhere else.

I'm not quite following? We need to move it into global scope to fix the
issue at hand (namely that we currently need to make install first, just
to get psql).  And at which scope could it be in master, other than
global?

I do think we will have to move it to the global scope in the back
branches too, because NO_TEMP_INSTALL does indeed fail without a global
install first (rather than using the temp install, as intended):

On 11:

$ make -j16 -s uninstall
$ make -j16 -s temp-install
$ make -j16 -s -C src/bin/pg_upgrade/ check NO_TEMP_INSTALL=1
...
../../../src/test/regress/pg_regress --inputdir=/home/andres/src/postgresql-11/src/test/regress
--bindir='/home/andres/build/postgres/11-assert//install/bin'   --port=60851 --dlpath=. --max-concurrent-tests=20
--port=60851--schedule=/home/andres/src/postgresql-11/src/test/regress/serial_schedule 
 
(using postmaster on /tmp/pg_upgrade_check-uEwhDs, port 60851)
============== dropping database "regression"         ==============
sh: 1: /home/andres/build/postgres/11-assert//install/bin/psql: not found

$ make -j16 -s install
$ make -j16 -s -C src/bin/pg_upgrade/ check NO_TEMP_INSTALL=1 && echo success
...
success

As you can see it uses pg_regress etc from the temp installation, but
psql from the full installation.


> (IOW, keep an eye on the cross-version-upgrade tests while
> you mess with this...)

I will. If you refer to the buildfarm ones: As far as I can tell they
don't use test.sh at all. Which makes sense, as we need cleanup steps
inbetween the regression run and pg_upgrade, and test.sh doesn't allow
for that.

https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
>> Not sure about that last bit.  pg_upgrade has the issue of possibly
>> wanting to deal with 2 installations, unlike the rest of the tree,
>> so I'm not sure that fixing its problem means there's something we
>> need to change everywhere else.

> I'm not quite following? We need to move it into global scope to fix the
> issue at hand (namely that we currently need to make install first, just
> to get psql).  And at which scope could it be in master, other than
> global?

Maybe I misunderstood you --- I thought you were talking about something
like defining EXTRA_REGRESS_OPTS in Makefile.global.  If you mean
running this unconditionally within test.sh, I've got no objection
to that.

> I do think we will have to move it to the global scope in the back
> branches too, because NO_TEMP_INSTALL does indeed fail without a global
> install first (rather than using the temp install, as intended):

Agreed, we should fix it in all branches, because it seems like it's
probably testing the wrong thing, ie using the later branch's psql
to run the earlier branch's regression tests.

            regards, tom lane

Hi,

On 2019-05-22 14:27:51 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
> >> Not sure about that last bit.  pg_upgrade has the issue of possibly
> >> wanting to deal with 2 installations, unlike the rest of the tree,
> >> so I'm not sure that fixing its problem means there's something we
> >> need to change everywhere else.
> 
> > I'm not quite following? We need to move it into global scope to fix the
> > issue at hand (namely that we currently need to make install first, just
> > to get psql).  And at which scope could it be in master, other than
> > global?
> 
> Maybe I misunderstood you --- I thought you were talking about something
> like defining EXTRA_REGRESS_OPTS in Makefile.global.  If you mean
> running this unconditionally within test.sh, I've got no objection
> to that.

Oh, yes, that's what I meant.


> > I do think we will have to move it to the global scope in the back
> > branches too, because NO_TEMP_INSTALL does indeed fail without a global
> > install first (rather than using the temp install, as intended):
> 
> Agreed, we should fix it in all branches, because it seems like it's
> probably testing the wrong thing, ie using the later branch's psql
> to run the earlier branch's regression tests.

Ok, will do.

Greetings,

Andres Freund

On 5/22/19 2:42 PM, Andres Freund wrote:
> Hi,
>
> On 2019-05-22 14:27:51 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
>>>> Not sure about that last bit.  pg_upgrade has the issue of possibly
>>>> wanting to deal with 2 installations, unlike the rest of the tree,
>>>> so I'm not sure that fixing its problem means there's something we
>>>> need to change everywhere else.
>>> I'm not quite following? We need to move it into global scope to fix the
>>> issue at hand (namely that we currently need to make install first, just
>>> to get psql).  And at which scope could it be in master, other than
>>> global?
>> Maybe I misunderstood you --- I thought you were talking about something
>> like defining EXTRA_REGRESS_OPTS in Makefile.global.  If you mean
>> running this unconditionally within test.sh, I've got no objection
>> to that.
> Oh, yes, that's what I meant.
>
>
>>> I do think we will have to move it to the global scope in the back
>>> branches too, because NO_TEMP_INSTALL does indeed fail without a global
>>> install first (rather than using the temp install, as intended):
>> Agreed, we should fix it in all branches, because it seems like it's
>> probably testing the wrong thing, ie using the later branch's psql
>> to run the earlier branch's regression tests.



If I disable install, the buildfarm fails the upgrade check even when
not using NO_TEMP_INSTALL.


excerpts from the log:



rm -rf '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install
/bin/mkdir -p
'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log
make -C '../../..'
DESTDIR='/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install
install
>'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log
2>&1
make -j1  checkprep
>>'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log
2>&1
MAKE=make
PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin:$PATH"

LD_LIBRARY_PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/lib" 
bindir=/home/pgl/npgl/pg_h
ead/bfroot/HEAD/pgsql.build/tmp_install//home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin
EXTRA_REGRESS_OPTS="--port=5678" /bin/sh
/home/pgl/npgl/pg_head/src/bin/pg_upgrade/test.sh


rm -rf ./testtablespace
mkdir ./testtablespace
../../../src/test/regress/pg_regress
--inputdir=/home/pgl/npgl/pg_head/src/test/regress
--bindir='/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin'   --port=5678
--port=54464 --dlpath=. --max-concurrent-tests=20 --port=5678
--port=54464
--schedule=/home/pgl/npgl/pg_head/src/test/regress/parallel_schedule 
(using postmaster on /tmp/pg_upgrade_check-GCUkGu, port 54464)
============== dropping database "regression"         ==============
sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or
directory
command failed: "/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql" -X -c
"DROP DATABASE IF EXISTS \"regression\"" "postgres"
make[1]: *** [GNUmakefile:141: installcheck-parallel] Error 2
make[1]: Leaving directory
'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/src/test/regress'
make: *** [GNUmakefile:68: installcheck-parallel] Error 2
make: Leaving directory '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'
waiting for server to shut down.... done
server stopped
make: *** [Makefile:48: check] Error 1



It looks to me like the bindir needs to be passed to the make called by
test.sh (maybe LD_LIBRARY_PATH too?)


cheers


andrew


-- 

Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-05-22 16:04:34 -0400, Andrew Dunstan wrote:
> 
> On 5/22/19 2:42 PM, Andres Freund wrote:
> > Hi,
> >
> > On 2019-05-22 14:27:51 -0400, Tom Lane wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> >>> On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
> >>>> Not sure about that last bit.  pg_upgrade has the issue of possibly
> >>>> wanting to deal with 2 installations, unlike the rest of the tree,
> >>>> so I'm not sure that fixing its problem means there's something we
> >>>> need to change everywhere else.
> >>> I'm not quite following? We need to move it into global scope to fix the
> >>> issue at hand (namely that we currently need to make install first, just
> >>> to get psql).  And at which scope could it be in master, other than
> >>> global?
> >> Maybe I misunderstood you --- I thought you were talking about something
> >> like defining EXTRA_REGRESS_OPTS in Makefile.global.  If you mean
> >> running this unconditionally within test.sh, I've got no objection
> >> to that.
> > Oh, yes, that's what I meant.
> >
> >
> >>> I do think we will have to move it to the global scope in the back
> >>> branches too, because NO_TEMP_INSTALL does indeed fail without a global
> >>> install first (rather than using the temp install, as intended):
> >> Agreed, we should fix it in all branches, because it seems like it's
> >> probably testing the wrong thing, ie using the later branch's psql
> >> to run the earlier branch's regression tests.
> 
> 
> 
> If I disable install, the buildfarm fails the upgrade check even when
> not using NO_TEMP_INSTALL.
> 
> 
> excerpts from the log:
> 
> 
> 
> rm -rf '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install
> /bin/mkdir -p
> '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log
> make -C '../../..'
> DESTDIR='/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install
> install
> >'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log
> 2>&1
> make -j1  checkprep
> >>'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log
> 2>&1
> MAKE=make
> PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin:$PATH"
>
LD_LIBRARY_PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/lib" 
> bindir=/home/pgl/npgl/pg_h
> ead/bfroot/HEAD/pgsql.build/tmp_install//home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin
> EXTRA_REGRESS_OPTS="--port=5678" /bin/sh
> /home/pgl/npgl/pg_head/src/bin/pg_upgrade/test.sh
> 
> 
> rm -rf ./testtablespace
> mkdir ./testtablespace
> ../../../src/test/regress/pg_regress
> --inputdir=/home/pgl/npgl/pg_head/src/test/regress
> --bindir='/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin'   --port=5678
> --port=54464 --dlpath=. --max-concurrent-tests=20 --port=5678
> --port=54464
> --schedule=/home/pgl/npgl/pg_head/src/test/regress/parallel_schedule 
> (using postmaster on /tmp/pg_upgrade_check-GCUkGu, port 54464)
> ============== dropping database "regression"         ==============
> sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or
> directory
> command failed: "/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql" -X -c
> "DROP DATABASE IF EXISTS \"regression\"" "postgres"
> make[1]: *** [GNUmakefile:141: installcheck-parallel] Error 2
> make[1]: Leaving directory
> '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/src/test/regress'
> make: *** [GNUmakefile:68: installcheck-parallel] Error 2
> make: Leaving directory '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'
> waiting for server to shut down.... done
> server stopped
> make: *** [Makefile:48: check] Error 1
> 

That's the issue I was talking to Tom about above. Need to
unconditionally have

+# We need to make pg_regress use psql from the desired installation
+# (likely a temporary one), because otherwise the installcheck run
+# below would try to use psql from the proper installation directory,
+# which might be outdated or missing. But don't override anything else
+# that's already in EXTRA_REGRESS_OPTS.
+EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$oldbindir'"
+export EXTRA_REGRESS_OPTS

in all branches (i.e. ressurect in master, do it not just in the
--install case in the back branches, and reference $oldbindir rather
than $bindir in all branches).


> It looks to me like the bindir needs to be passed to the make called by
> test.sh (maybe LD_LIBRARY_PATH too?)

Think we don't need LD_LIBRARY_PATH, due to the $(with_temp_install)
logic in the makefile. In the back branches the --install branch
contains adjustments to LD_LIBRARY_PATH (but still references $bindir
rather than $oldbindr).

Greetings,

Andres Freund

Hi,

On 2019-05-22 13:08:41 -0700, Andres Freund wrote:
> On 2019-05-22 16:04:34 -0400, Andrew Dunstan wrote:
> > If I disable install, the buildfarm fails the upgrade check even when
> > not using NO_TEMP_INSTALL.
> > 
> > 
> > excerpts from the log:
> > sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or
> > directory
> 
> That's the issue I was talking to Tom about above. Need to
> unconditionally have
> ....

Andrew, after the latest set of changes, the reversed order should now
work reliably?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Andrew, after the latest set of changes, the reversed order should now
> work reliably?

Also, Thomas should be able to revert his cfbot hack ...

            regards, tom lane

On 5/24/19 11:22 AM, Andres Freund wrote:
> Hi,
>
> On 2019-05-22 13:08:41 -0700, Andres Freund wrote:
>> On 2019-05-22 16:04:34 -0400, Andrew Dunstan wrote:
>>> If I disable install, the buildfarm fails the upgrade check even when
>>> not using NO_TEMP_INSTALL.
>>>
>>>
>>> excerpts from the log:
>>> sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or
>>> directory
>> That's the issue I was talking to Tom about above. Need to
>> unconditionally have
>> ....
> Andrew, after the latest set of changes, the reversed order should now
> work reliably?
>


With the latest changes I don't get the above failure:


andrew@emma:pg_head (master)$ ~/bf/client-code/run_build.pl 
--skip-steps=install
master:HEAD          [19:21:45] creating vpath build dir
/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build ...
master:HEAD          [19:21:45] running configure ...
master:HEAD          [19:21:52] running make ...
master:HEAD          [19:23:42] running make check ...
master:HEAD          [19:24:37] running make contrib ...
master:HEAD          [19:24:45] running make testmodules ...
master:HEAD          [19:24:45] checking pg_upgrade
master:HEAD          [19:26:41] checking test-decoding
master:HEAD          [19:26:59] running make ecpg check ...
master:HEAD          [19:27:25] OK


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services