Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set

Hi,

On 2022-01-18 11:20:16 +0900, Michael Paquier wrote:
> +# required for 002_pg_upgrade.pl
> +REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
> +export REGRESS_SHLIB

It seems weird to propagate this into multiple places. Why don't we define
that centrally?

Although it's weird for this to use REGRESS_SHLIB, given it's just doing
dirname() on it. 027_stream_regress.pl has the "defense" of not wanting to
duplicate the variable with 017_shm.pl...

Not that I understand why 017_shm.pl and all the regression test source
fileseven need $(DLSUFFIX) - expand_dynamic_library_name() should take care of
it?

> +REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/bin/pg_upgrade
> +export REGRESS_OUTPUTDIR

I don't really understand why 027_stream_regress.pl is using this (and thus
not why it's used here). The tap tests create files all the time, why is this
different?

It's not like make / msvc put the data in different places:
src/test/recovery/Makefile:REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/test/recovery/tmp_check
src/tools/msvc/vcregress.pl: $ENV{REGRESS_OUTPUTDIR} = "$topdir/src/test/recovery/tmp_check";

> +
> +# From now on, the test of pg_upgrade consists in setting up an instance.

What does "from now on" mean?

> +# Default is the location of this source code for both nodes used with
> +# the upgrade.

Can't quite parse.

> +# Initialize a new node for the upgrade. This is done early so as it is
> +# possible to know with which node's PATH the initial dump needs to be
> +# taken.
> +my $newnode = PostgreSQL::Test::Cluster->new('new_node');
> +$newnode->init(extra => [ '--locale=C', '--encoding=LATIN1' ]);
> +my $newbindir = $newnode->config_data('--bindir');
> +my $oldbindir = $oldnode->config_data('--bindir');

Why C/LATIN?

Right now pg_upgrade test.sh uses --wal-segsize 1, and that has helped
identify several bugs. So I'd rather not give it up, even if it's a bit weird.

> + my @regress_command = [
> + $ENV{PG_REGRESS},
> + '--schedule', "$oldsrc/src/test/regress/parallel_schedule",
> + '--bindir', $oldnode->config_data('--bindir'),
> + '--dlpath', $dlpath,
> + '--port', $oldnode->port,
> + '--outputdir', $outputdir,
> + '--inputdir', $inputdir,
> + '--use-existing'
> + ];

I think this should use --host (c.f. 7340aceed72). Or is it intending to use
the host via env? If so, why is the port specified?

> + @regress_command = (@regress_command, @extra_opts);
> +
> + $oldnode->command_ok(@regress_command,
> + 'regression test run on old instance');

I also think this should take EXTRA_REGRESS_OPTS into account - test.sh did.

> +# After dumping, update references to the old source tree's regress.so
> +# to point to the new tree.
> +if (defined($ENV{oldinstall}))
> +{

Kinda asking for its own function...

> +
> +# Update the instance.
> +$oldnode->stop;
> +
> +# Time for the real run.

As opposed to the unreal one?

> +# pg_upgrade would complain if PGHOST, so as there are no attempts to
> +# connect to a different server than the upgraded ones.

"complain if PGHOST"?

> +# Take a second dump on the upgraded instance.

Sounds like you're taking to post-upgrade pg_dumps.

Greetings,

Andres Freund