Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set - Mailing list pgsql-hackers

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