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

From: Andres Freund <andres(at)anarazel(dot)de>
To: Michael Paquier <michael(at)paquier(dot)xyz>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Noah Misch <noah(at)leadboat(dot)com>
Cc: Julien Rouhaud <rjuju123(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
Date: 2022-02-13 04:50:41
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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

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. has the "defense" of not wanting to
duplicate the variable with

Not that I understand why and all the regression test source
fileseven need $(DLSUFFIX) - expand_dynamic_library_name() should take care of

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

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

It's not like make / msvc put the data in different places:
src/tools/msvc/ $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');


Right now pg_upgrade 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 = [
> + '--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 - did.

> +# After dumping, update references to the old source tree's
> +# 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.


Andres Freund

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2022-02-13 05:07:30 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
Previous Message Dilip Kumar 2022-02-13 04:42:38 Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints