Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

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