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

From: Michael Paquier <[email-masked]>
Date: Wed, 16 Feb 2022 13:58:10 +0900

On Tue, Feb 15, 2022 at 01:02:41PM +0900, Michael Paquier wrote:
> Hmm.  At the end of the day, I am wondering whether we should not give
> up entirely on the concept of running the regression tests on older
> branches in the TAP script of a newer branch.  pg_regress needs to
> come from the old source tree, meaning that we would most likely need
> to maintain a set of compatibility tweaks that would most probably
> rot over the time, and the buildfarm only cares about the possibility
> to set up old instances by loading dumps rather than running
> pg_regress.  This would also make the switch to TAP much easier (no
> need for the extra createdb or --locale AFAIK).  So attempting to
> maintain all that is going to be a PITA in the long term, and there is
> nothing running that automatically anyway.
> 
> There is also the extra requirement to adjust dump files, but that's
> independent of setting up the old instance to upgrade, and I don't
> really plan to tackle that as of this thread (note that the buildfarm
> client has extra tweaks regarding that).
> 
> Any thoughts about that?

I have been looking at how much simplicity this brings, and I have to
admit that it is tempting to just support the loading of dumps when
setting up the old instance to upgrade from.  We'd still need to do an
extra effort in terms of cleaning up the diffs for the dump of the old
instance with older versions once/if this is plugged into the
buildfarm, but that could be addressed later depending on the versions
that need to be covered.
--
Michael
From 7957ff3c2030a3a7df87ca0663a29deff69b385a Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 15 Feb 2022 12:05:28 +0900
Subject: [PATCH v9 1/2] Fix sanity check for PGHOST[ADDR] in pg_upgrade with
 Windows paths

The checks currently done at the startup of pg_upgrade for PGHOST and
PGHOSTADDR to avoid any attempt to access to an external cluster would
fail when attempting to use Windows paths, or even temporary paths
prefixed by '@'.  is_unixsock_path() is designed to detect such cases,
so use it rather than assuming that all valid paths are prefixed with a
slash.

Issue found while testing the tests of pg_upgrade through the CI on
Windows.

Based on an analysis from me and a solution from Andres Freund.
---
 src/bin/pg_upgrade/server.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 7878d233de..265137e86b 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -11,6 +11,7 @@
 
 #include "common/connect.h"
 #include "fe_utils/string_utils.h"
+#include "libpq/pqcomm.h"
 #include "pg_upgrade.h"
 
 static PGconn *get_db_conn(ClusterInfo *cluster, const char *db_name);
@@ -368,7 +369,7 @@ check_pghost_envvar(void)
 			if (value && strlen(value) > 0 &&
 			/* check for 'local' host values */
 				(strcmp(value, "localhost") != 0 && strcmp(value, "127.0.0.1") != 0 &&
-				 strcmp(value, "::1") != 0 && value[0] != '/'))
+				 strcmp(value, "::1") != 0 && !is_unixsock_path(value)))
 				pg_fatal("libpq environment variable %s has a non-local server value: %s\n",
 						 option->envvar, value);
 		}
-- 
2.34.1

From ab8860e4bbc93d71bec3c875128dffe121bd282e Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Wed, 16 Feb 2022 13:50:35 +0900
Subject: [PATCH v9 2/2] Switch tests of pg_upgrade to use TAP

---
 src/bin/pg_upgrade/Makefile            |  21 +-
 src/bin/pg_upgrade/TESTING             |  85 ++------
 src/bin/pg_upgrade/t/001_basic.pl      |   9 +
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 255 ++++++++++++++++++++++
 src/bin/pg_upgrade/test.sh             | 279 -------------------------
 src/tools/msvc/vcregress.pl            |  92 +-------
 6 files changed, 294 insertions(+), 447 deletions(-)
 create mode 100644 src/bin/pg_upgrade/t/001_basic.pl
 create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl
 delete mode 100644 src/bin/pg_upgrade/test.sh

diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 49b94f0ac7..1f5d757548 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -28,6 +28,10 @@ OBJS = \
 override CPPFLAGS := -DDLSUFFIX=\"$(DLSUFFIX)\" -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
 
+# required for 002_pg_upgrade.pl
+REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
+export REGRESS_SHLIB
+
 all: pg_upgrade
 
 pg_upgrade: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
@@ -47,17 +51,8 @@ clean distclean maintainer-clean:
 	rm -rf delete_old_cluster.sh log/ tmp_check/ \
 	       reindex_hash.sql
 
-# When $(MAKE) is present, make automatically infers that this is a
-# recursive make. which is not actually what we want here, as that
-# e.g. prevents output synchronization from working (as make thinks
-# that the subsidiary make knows how to deal with that itself, but
-# we're invoking a shell script that doesn't know). Referencing
-# $(MAKE) indirectly avoids that behaviour.
-# See https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html#MAKE-Variable
-NOTSUBMAKEMAKE=$(MAKE)
+check:
+	$(prove_check)
 
-check: test.sh all temp-install
-	MAKE=$(NOTSUBMAKEMAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $<
-
-# installcheck is not supported because there's no meaningful way to test
-# pg_upgrade against a single already-running server
+installcheck:
+	$(prove_installcheck)
diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING
index 78b9747908..3718483a1c 100644
--- a/src/bin/pg_upgrade/TESTING
+++ b/src/bin/pg_upgrade/TESTING
@@ -2,78 +2,23 @@ THE SHORT VERSION
 -----------------
 
 On non-Windows machines, you can execute the testing process
-described below by running
+described below by running the following command in this directory:
 	make check
-in this directory.  This will run the shell script test.sh, performing
-an upgrade from the version in this source tree to a new instance of
-the same version.
 
-To test an upgrade from a different version, you must have a built
-source tree for the old version as well as this version, and you
-must have done "make install" for both versions.  Then do:
+This will run the TAP tests to run pg_upgrade, performing an upgrade
+from the version in this source tree to a new instance of the same
+version.
 
-export oldsrc=...somewhere/postgresql	(old version's source tree)
-export oldbindir=...otherversion/bin	(old version's installed bin dir)
-export bindir=...thisversion/bin	(this version's installed bin dir)
-export libdir=...thisversion/lib	(this version's installed lib dir)
-sh test.sh
+Testing an upgrade from a different version requires a dump to set up
+the contents of this instance, with its set of binaries.  The following
+variables are available to control the test:
+export olddump=...somewhere/dump.sql	(old version's dump)
+export oldinstall=...otherversion/	(old version's install base path)
 
-In this case, you will have to manually eyeball the resulting dump
-diff for version-specific differences, as explained below.
+Finally, the tests can be done by running
+	make check
 
-
-DETAILS
--------
-
-The most effective way to test pg_upgrade, aside from testing on user
-data, is by upgrading the PostgreSQL regression database.
-
-This testing process first requires the creation of a valid regression
-database dump.  Such files contain most database features and are
-specific to each major version of Postgres.
-
-Here are the steps needed to create a regression database dump file:
-
-1)  Create and populate the regression database in the old cluster.
-    This database can be created by running 'make installcheck' from
-    src/test/regress.
-
-2)  Use pg_dump to dump out the regression database.  Use the new
-    cluster's pg_dump on the old database to minimize whitespace
-    differences in the diff.
-
-3)  Adjust the regression database dump file
-
-    a)  Perform the load/dump twice
-        This fixes problems with the ordering of COPY columns for
-        inherited tables.
-
-    b)  Change CREATE FUNCTION shared object paths to use '$libdir'
-        The old and new cluster will have different shared object paths.
-
-    c)  Fix any wrapping format differences
-        Commands like CREATE TRIGGER and ALTER TABLE sometimes have
-        differences.
-
-Once the dump is created, it can be repeatedly loaded into the old
-database, upgraded, and dumped out of the new database, and then
-compared to the original version. To test the dump file, perform these
-steps:
-
-1)  Create the old and new clusters in different directories.
-
-2)  Copy the regression shared object files into the appropriate /lib
-    directory for old and new clusters.
-
-3)  Create the regression database in the old server.
-
-4)  Load the dump file created above into the regression database;
-    check for errors while loading.
-
-5)  Upgrade the old database to the new major version, as outlined in
-    the pg_upgrade manual section.
-
-6)  Use pg_dump to dump out the regression database in the new cluster.
-
-7)  Diff the regression database dump file with the regression dump
-    file loaded into the old server.
+Using the test in this configuration is possible with any dump file.
+However, it is recommended to create a dump from the database "regression",
+created by running 'make installcheck' from src/test/regress as this covers
+most database features and are specific to each major version of Postgres.
diff --git a/src/bin/pg_upgrade/t/001_basic.pl b/src/bin/pg_upgrade/t/001_basic.pl
new file mode 100644
index 0000000000..75b0f98b08
--- /dev/null
+++ b/src/bin/pg_upgrade/t/001_basic.pl
@@ -0,0 +1,9 @@
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Utils;
+use Test::More tests => 8;
+
+program_help_ok('pg_upgrade');
+program_version_ok('pg_upgrade');
+program_options_handling_ok('pg_upgrade');
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
new file mode 100644
index 0000000000..7c314c9d26
--- /dev/null
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -0,0 +1,255 @@
+# Set of tests for pg_upgrade, including cross-version checks.
+use strict;
+use warnings;
+
+use Cwd qw(abs_path getcwd);
+use File::Basename qw(dirname);
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Generate a database with a name made of a range of ASCII characters.
+sub generate_db
+{
+	my ($node, $from_char, $to_char) = @_;
+
+	my $dbname = '';
+	for my $i ($from_char .. $to_char)
+	{
+		next if $i == 7 || $i == 10 || $i == 13;    # skip BEL, LF, and CR
+		$dbname = $dbname . sprintf('%c', $i);
+	}
+	$node->run_log(
+		[ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]
+	);
+}
+
+# The test of pg_upgrade consists in setting up an instance.  This is the
+# source instance used for the upgrade. Then a new and fresh instance is
+# created, and is used as the target instance for the upgrade.  Before
+# running an upgrade, a logical dump of the old instance is taken, and a
+# second logical dump of the new instance is taken after the upgrade.
+# The upgrade test passes if there are no differences in these two dumps.
+
+# Testing upgrades with an older instance of PostgreSQL requires setting up
+# two environment variables, as of:
+# - "olddump", to point to a dump file that will be used to set
+#   up the old instance to upgrade from, the dump being restored in the
+#   old cluster.
+# - "oldinstall", to point to the installation path of the old
+#   instance.
+if (   (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
+	|| (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
+{
+	# Not all variables are defined, so leave and die if test is
+	# done with an older installation.
+	die "olddump or oldinstall is undefined";
+}
+
+# Temporary location for the dumps taken
+my $tempdir = PostgreSQL::Test::Utils::tempdir;
+
+# Initialize node to upgrade
+my $oldnode = PostgreSQL::Test::Cluster->new('old_node',
+	install_path => $ENV{oldinstall});
+
+$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+$oldnode->start;
+
+# The default location of the source code is the root of this directory
+# for the new and old nodes.
+my $srcdir = abs_path("../../..");
+
+# Set up the data of the old instance with pg_regress or an old dump.
+if (defined($ENV{olddump}))
+{
+	# Use the dump specified.
+	my $olddumpfile = $ENV{olddump};
+	die "no dump file found!" unless -e $olddumpfile;
+
+	# Load the dump, and we are done here.
+	$oldnode->command_ok(
+		[
+			'psql',   '-X',           '-f',     $olddumpfile,
+			'--port', $oldnode->port, '--host', $oldnode->host,
+			'regression'
+		]);
+}
+else
+{
+	# Default is to just use pg_regress to set up the old instance
+	# Creating databases with names covering most ASCII bytes
+	generate_db($oldnode, 1,  45);
+	generate_db($oldnode, 46, 90);
+	generate_db($oldnode, 91, 127);
+
+	# Grab any regression options that may be passed down by caller.
+	my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
+	my @extra_opts     = split(/\s+/, $extra_opts_val);
+
+	# --dlpath is needed to be able to find the location of regress.so and
+	# any libraries the regression tests required.  This needs to point to
+	# the old instance when using it.  In the default case, fallback to
+	# what the caller provided for REGRESS_SHLIB.
+	my $dlpath =
+	  dirname(PostgreSQL::Test::Utils::perl2host($ENV{REGRESS_SHLIB}));
+
+	# --outputdir points to the path where to place the output files.
+	my $outputdir =
+	  PostgreSQL::Test::Utils::perl2host($PostgreSQL::Test::Utils::tmp_check);
+
+	# --inputdir points to the path of the input files.
+	my $inputdir =
+	  PostgreSQL::Test::Utils::perl2host("$srcdir/src/test/regress");
+
+	my @regress_command = [
+		$ENV{PG_REGRESS},
+		'--dlpath',
+		$dlpath,
+		'--max-concurrent-tests',
+		'20',
+		'--bindir',
+		$oldnode->config_data('--bindir'),
+		'--host',
+		$oldnode->host,
+		'--port',
+		$oldnode->port,
+		'--schedule',
+		"$srcdir/src/test/regress/parallel_schedule",
+		'--outputdir',
+		$outputdir,
+		'--inputdir',
+		$inputdir
+	];
+	@regress_command = (@regress_command, @extra_opts);
+
+	$oldnode->command_ok(@regress_command,
+		'regression test run on old instance');
+}
+
+# Before dumping, get rid of objects not existing or not supported in later
+# versions. This depends on the version of the old server used, and matters
+# only if different versions are used for the dump.
+if (defined($ENV{oldinstall}))
+{
+	# Note that upgrade_adapt.sql from the new version is used, to
+	# cope with an upgrade to this version.
+	$oldnode->run_log(
+		[
+			'psql', '-X', '-f',
+			PostgreSQL::Test::Utils::perl2host(
+				"$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql"),
+			'--port',
+			$oldnode->port,
+			'--host',
+			$oldnode->host,
+			'regression'
+		]);
+}
+
+# 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 => [ '--wal-segsize', '1', '--allow-group-access' ]);
+my $newbindir = $newnode->config_data('--bindir');
+my $oldbindir = $oldnode->config_data('--bindir');
+
+# Take a dump before performing the upgrade as a base comparison. Note
+# that we need to use pg_dumpall from the new node here.
+$newnode->command_ok(
+	[
+		'pg_dumpall', '--no-sync',
+		'-d',         $oldnode->connstr('postgres'),
+		'-f',         "$tempdir/dump1.sql"
+	],
+	'dump before running pg_upgrade');
+
+# After dumping, update references to the old source tree's regress.so
+# to point to the new tree.
+if (defined($ENV{oldinstall}))
+{
+	# First, fetch all the references to libraries that are not part
+	# of the default path $libdir.
+	my $output = $oldnode->safe_psql('regression',
+		"SELECT DISTINCT probin::text FROM pg_proc WHERE probin NOT LIKE '\$libdir%';"
+	);
+	chomp($output);
+	my @libpaths = split("\n", $output);
+
+	my $dump_data = slurp_file("$tempdir/dump1.sql");
+
+	my $newregresssrc = "$srcdir/src/test/regress";
+	foreach (@libpaths)
+	{
+		my $libpath = $_;
+		$libpath = dirname($libpath);
+		$dump_data =~ s/$libpath/$newregresssrc/g;
+	}
+
+	open my $fh, ">", "$tempdir/dump1.sql" or die "could not open dump file";
+	print $fh $dump_data;
+	close $fh;
+
+	# This replaces any references to the old tree's regress.so
+	# the new tree's regress.so.  Any references that do *not*
+	# match $libdir are switched so as this request does not
+	# depend on the path of the old source tree.  This is useful
+	# when using an old dump.  Do the operation on all the databases
+	# that allow connections so as this includes the regression
+	# database and anything the user has set up.
+	$output = $oldnode->safe_psql('postgres',
+		"SELECT datname FROM pg_database WHERE datallowconn;");
+	chomp($output);
+	my @datnames = split("\n", $output);
+	foreach (@datnames)
+	{
+		my $datname = $_;
+		$oldnode->safe_psql(
+			$datname, "UPDATE pg_proc SET probin =
+		  regexp_replace(probin, '.*/', '$newregresssrc/')
+		  WHERE probin NOT LIKE '\$libdir/%'");
+	}
+}
+
+# Upgrade the instance.
+$oldnode->stop;
+command_ok(
+	[
+		'pg_upgrade', '--no-sync',        '-d', $oldnode->data_dir,
+		'-D',         $newnode->data_dir, '-b', $oldbindir,
+		'-B',         $newbindir,         '-p', $oldnode->port,
+		'-P',         $newnode->port
+	],
+	'run of pg_upgrade for new instance');
+$newnode->start;
+
+# Check if there are any logs coming from pg_upgrade, that would only be
+# retained on failure.
+my $log_path = $newnode->data_dir . "/pg_upgrade_output.d/log";
+if (-d $log_path)
+{
+	foreach my $log (glob("$log_path/*"))
+	{
+		note "###########################";
+		note "Contents of log file $log";
+		note "###########################";
+		my $log_contents = slurp_file($log);
+		print "$log_contents\n";
+	}
+}
+
+# Second dump from the upgraded instance.
+$newnode->run_log(
+	[
+		'pg_dumpall', '--no-sync',
+		'-d',         $newnode->connstr('postgres'),
+		'-f',         "$tempdir/dump2.sql"
+	]);
+
+# Compare the two dumps, there should be no differences.
+command_ok([ 'diff', '-q', "$tempdir/dump1.sql", "$tempdir/dump2.sql" ],
+	'old and new dump match after pg_upgrade');
+
+done_testing();
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
deleted file mode 100644
index ef328b3062..0000000000
--- a/src/bin/pg_upgrade/test.sh
+++ /dev/null
@@ -1,279 +0,0 @@
-#!/bin/sh
-
-# src/bin/pg_upgrade/test.sh
-#
-# Test driver for pg_upgrade.  Initializes a new database cluster,
-# runs the regression tests (to put in some data), runs pg_dumpall,
-# runs pg_upgrade, runs pg_dumpall again, compares the dumps.
-#
-# Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
-# Portions Copyright (c) 1994, Regents of the University of California
-
-set -e
-
-: ${MAKE=make}
-
-# Guard against parallel make issues (see comments in pg_regress.c)
-unset MAKEFLAGS
-unset MAKELEVEL
-
-# Run a given "initdb" binary and overlay the regression testing
-# authentication configuration.
-standard_initdb() {
-	# To increase coverage of non-standard segment size and group access
-	# without increasing test runtime, run these tests with a custom setting.
-	# Also, specify "-A trust" explicitly to suppress initdb's warning.
-	# --allow-group-access and --wal-segsize have been added in v11.
-	"$1" -N --wal-segsize 1 --allow-group-access -A trust
-	if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ]
-	then
-		cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf"
-	fi
-	../../test/regress/pg_regress --config-auth "$PGDATA"
-}
-
-# What flavor of host are we on?
-# Treat MINGW* (msys1) and MSYS* (msys2) the same.
-testhost=`uname -s | sed 's/^MSYS/MINGW/'`
-
-# Establish how the server will listen for connections
-case $testhost in
-	MINGW*)
-		LISTEN_ADDRESSES="localhost"
-		PG_REGRESS_SOCKET_DIR=""
-		PGHOST=localhost
-		;;
-	*)
-		LISTEN_ADDRESSES=""
-		# Select a socket directory.  The algorithm is from the "configure"
-		# script; the outcome mimics pg_regress.c:make_temp_sockdir().
-		if [ x"$PG_REGRESS_SOCKET_DIR" = x ]; then
-			set +e
-			dir=`(umask 077 &&
-				  mktemp -d /tmp/pg_upgrade_check-XXXXXX) 2>/dev/null`
-			if [ ! -d "$dir" ]; then
-				dir=/tmp/pg_upgrade_check-$$-$RANDOM
-				(umask 077 && mkdir "$dir")
-				if [ ! -d "$dir" ]; then
-					echo "could not create socket temporary directory in \"/tmp\""
-					exit 1
-				fi
-			fi
-			set -e
-			PG_REGRESS_SOCKET_DIR=$dir
-			trap 'rm -rf "$PG_REGRESS_SOCKET_DIR"' 0
-			trap 'exit 3' 1 2 13 15
-		fi
-		PGHOST=$PG_REGRESS_SOCKET_DIR
-		;;
-esac
-
-POSTMASTER_OPTS="-F -c listen_addresses=\"$LISTEN_ADDRESSES\" -k \"$PG_REGRESS_SOCKET_DIR\""
-export PGHOST
-
-# don't rely on $PWD here, as old shells don't set it
-temp_root=`pwd`/tmp_check
-rm -rf "$temp_root"
-mkdir "$temp_root"
-
-: ${oldbindir=$bindir}
-
-: ${oldsrc=../../..}
-oldsrc=`cd "$oldsrc" && pwd`
-newsrc=`cd ../../.. && pwd`
-
-# 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
-# of the target version, which might be outdated or not exist. 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
-
-# While in normal cases this will already be set up, adding bindir to
-# path allows test.sh to be invoked with different versions as
-# described in ./TESTING
-PATH=$bindir:$PATH
-export PATH
-
-BASE_PGDATA="$temp_root/data"
-PGDATA="${BASE_PGDATA}.old"
-export PGDATA
-
-# Send installcheck outputs to a private directory.  This avoids conflict when
-# check-world runs pg_upgrade check concurrently with src/test/regress check.
-# To retrieve interesting files after a run, use pattern tmp_check/*/*.diffs.
-outputdir="$temp_root/regress"
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --outputdir=$outputdir"
-export EXTRA_REGRESS_OPTS
-mkdir "$outputdir"
-
-# pg_regress --make-tablespacedir would take care of that in 14~, but this is
-# still required for older versions where this option is not supported.
-if [ "$newsrc" != "$oldsrc" ]; then
-	mkdir "$outputdir"/testtablespace
-	mkdir "$outputdir"/sql
-	mkdir "$outputdir"/expected
-fi
-
-logdir=`pwd`/log
-rm -rf "$logdir"
-mkdir "$logdir"
-
-# Clear out any environment vars that might cause libpq to connect to
-# the wrong postmaster (cf pg_regress.c)
-#
-# Some shells, such as NetBSD's, return non-zero from unset if the variable
-# is already unset. Since we are operating under 'set -e', this causes the
-# script to fail. To guard against this, set them all to an empty string first.
-PGDATABASE="";        unset PGDATABASE
-PGUSER="";            unset PGUSER
-PGSERVICE="";         unset PGSERVICE
-PGSSLMODE="";         unset PGSSLMODE
-PGREQUIRESSL="";      unset PGREQUIRESSL
-PGCONNECT_TIMEOUT=""; unset PGCONNECT_TIMEOUT
-PGHOSTADDR="";        unset PGHOSTADDR
-
-# Select a non-conflicting port number, similarly to pg_regress.c
-PG_VERSION_NUM=`grep '#define PG_VERSION_NUM' "$newsrc"/src/include/pg_config.h | awk '{print $3}'`
-PGPORT=`expr $PG_VERSION_NUM % 16384 + 49152`
-export PGPORT
-
-i=0
-while psql -X postgres </dev/null 2>/dev/null
-do
-	i=`expr $i + 1`
-	if [ $i -eq 16 ]
-	then
-		echo port $PGPORT apparently in use
-		exit 1
-	fi
-	PGPORT=`expr $PGPORT + 1`
-	export PGPORT
-done
-
-# buildfarm may try to override port via EXTRA_REGRESS_OPTS ...
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --port=$PGPORT"
-export EXTRA_REGRESS_OPTS
-
-standard_initdb "$oldbindir"/initdb
-"$oldbindir"/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w
-
-# Create databases with names covering the ASCII bytes other than NUL, BEL,
-# LF, or CR.  BEL would ring the terminal bell in the course of this test, and
-# it is not otherwise a special case.  PostgreSQL doesn't support the rest.
-dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
-	if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null`
-# Exercise backslashes adjacent to double quotes, a Windows special case.
-dbname1='\"\'$dbname1'\\"\\\'
-dbname2=`awk 'BEGIN { for (i = 46; i <  91; i++) printf "%c", i }' </dev/null`
-dbname3=`awk 'BEGIN { for (i = 91; i < 128; i++) printf "%c", i }' </dev/null`
-createdb "regression$dbname1" || createdb_status=$?
-createdb "regression$dbname2" || createdb_status=$?
-createdb "regression$dbname3" || createdb_status=$?
-
-# Extra options to apply to the dump.  This may be changed later.
-extra_dump_options=""
-
-if "$MAKE" -C "$oldsrc" installcheck-parallel; then
-	oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
-
-	# Before dumping, tweak the database of the old instance depending
-	# on its version.
-	if [ "$newsrc" != "$oldsrc" ]; then
-		# This SQL script has its own idea of the cleanup that needs to be
-		# done on the cluster to-be-upgraded, and includes version checks.
-		# Note that this uses the script stored on the new branch.
-		psql -X -d regression -f "$newsrc/src/bin/pg_upgrade/upgrade_adapt.sql" \
-			|| psql_fix_sql_status=$?
-
-		# Handling of --extra-float-digits gets messy after v12.
-		# Note that this changes the dumps from the old and new
-		# instances if involving an old cluster of v11 or older.
-		if [ $oldpgversion -lt 120000 ]; then
-			extra_dump_options="--extra-float-digits=0"
-		fi
-	fi
-
-	pg_dumpall $extra_dump_options --no-sync \
-		-f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
-
-	if [ "$newsrc" != "$oldsrc" ]; then
-		# update references to old source tree's regress.so etc
-		fix_sql=""
-		case $oldpgversion in
-			*)
-				fix_sql="UPDATE pg_proc SET probin = replace(probin, '$oldsrc', '$newsrc') WHERE probin LIKE '$oldsrc%';"
-				;;
-		esac
-		psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=$?
-
-		mv "$temp_root"/dump1.sql "$temp_root"/dump1.sql.orig
-		sed "s;$oldsrc;$newsrc;g" "$temp_root"/dump1.sql.orig >"$temp_root"/dump1.sql
-	fi
-else
-	make_installcheck_status=$?
-fi
-"$oldbindir"/pg_ctl -m fast stop
-if [ -n "$createdb_status" ]; then
-	exit 1
-fi
-if [ -n "$make_installcheck_status" ]; then
-	exit 1
-fi
-if [ -n "$psql_fix_sql_status" ]; then
-	exit 1
-fi
-if [ -n "$pg_dumpall1_status" ]; then
-	echo "pg_dumpall of pre-upgrade database cluster failed"
-	exit 1
-fi
-
-PGDATA="$BASE_PGDATA"
-
-standard_initdb 'initdb'
-
-pg_upgrade $PG_UPGRADE_OPTS --no-sync -d "${PGDATA}.old" -D "$PGDATA" -b "$oldbindir" -p "$PGPORT" -P "$PGPORT"
-
-# make sure all directories and files have group permissions, on Unix hosts
-# Windows hosts don't support Unix-y permissions.
-case $testhost in
-	MINGW*|CYGWIN*) ;;
-	*)	if [ `find "$PGDATA" -type f ! -perm 640 | wc -l` -ne 0 ]; then
-			echo "files in PGDATA with permission != 640";
-			exit 1;
-		fi ;;
-esac
-
-case $testhost in
-	MINGW*|CYGWIN*) ;;
-	*)	if [ `find "$PGDATA" -type d ! -perm 750 | wc -l` -ne 0 ]; then
-			echo "directories in PGDATA with permission != 750";
-			exit 1;
-		fi ;;
-esac
-
-pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w
-
-pg_dumpall $extra_dump_options --no-sync \
-	-f "$temp_root"/dump2.sql || pg_dumpall2_status=$?
-pg_ctl -m fast stop
-
-if [ -n "$pg_dumpall2_status" ]; then
-	echo "pg_dumpall of post-upgrade database cluster failed"
-	exit 1
-fi
-
-case $testhost in
-	MINGW*)	MSYS2_ARG_CONV_EXCL=/c cmd /c delete_old_cluster.bat ;;
-	*)	    sh ./delete_old_cluster.sh ;;
-esac
-
-if diff "$temp_root"/dump1.sql "$temp_root"/dump2.sql >/dev/null; then
-	echo PASSED
-	exit 0
-else
-	echo "Files $temp_root/dump1.sql and $temp_root/dump2.sql differ"
-	echo "dumps were not identical"
-	exit 1
-fi
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index a994626239..39d1e49721 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -285,6 +285,10 @@ sub bincheck
 	foreach my $dir (@bin_dirs)
 	{
 		next unless -d "$dir/t";
+		# Do not consider pg_upgrade, as it is handled by
+		# upgradecheck.
+		next if ($dir =~ "/pg_upgrade/");
+
 		my $status = tap_check($dir);
 		$mstat ||= $status;
 	}
@@ -589,91 +593,9 @@ sub generate_db
 
 sub upgradecheck
 {
-	my $status;
-	my $cwd = getcwd();
-
-	# Much of this comes from the pg_upgrade test.sh script,
-	# but it only covers the --install case, and not the case
-	# where the old and new source or bin dirs are different.
-	# i.e. only this version to this version check. That's
-	# what pg_upgrade's "make check" does.
-
-	$ENV{PGHOST} = 'localhost';
-	$ENV{PGPORT} ||= 50432;
-	my $tmp_root = "$topdir/src/bin/pg_upgrade/tmp_check";
-	rmtree($tmp_root);
-	mkdir $tmp_root || die $!;
-	my $upg_tmp_install = "$tmp_root/install";    # unshared temp install
-	print "Setting up temp install\n\n";
-	Install($upg_tmp_install, "all", $config);
-
-	# Install does a chdir, so change back after that
-	chdir $cwd;
-	my ($bindir, $libdir, $oldsrc, $newsrc) =
-	  ("$upg_tmp_install/bin", "$upg_tmp_install/lib", $topdir, $topdir);
-	$ENV{PATH} = "$bindir;$ENV{PATH}";
-	my $data = "$tmp_root/data";
-	$ENV{PGDATA} = "$data.old";
-	my $outputdir          = "$tmp_root/regress";
-	my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");
-	mkdir "$outputdir" || die $!;
-
-	my $logdir = "$topdir/src/bin/pg_upgrade/log";
-	rmtree($logdir);
-	mkdir $logdir || die $!;
-	print "\nRunning initdb on old cluster\n\n";
-	standard_initdb() or exit 1;
-	print "\nStarting old cluster\n\n";
-	my @args = ('pg_ctl', 'start', '-l', "$logdir/postmaster1.log");
-	system(@args) == 0 or exit 1;
-
-	print "\nCreating databases with names covering most ASCII bytes\n\n";
-	generate_db("\\\"\\", 1,  45,  "\\\\\"\\\\\\");
-	generate_db('',       46, 90,  '');
-	generate_db('',       91, 127, '');
-
-	print "\nSetting up data for upgrading\n\n";
-	installcheck_internal('parallel', @EXTRA_REGRESS_OPTS);
-
-	# now we can chdir into the source dir
-	chdir "$topdir/src/bin/pg_upgrade";
-	print "\nDumping old cluster\n\n";
-	@args = ('pg_dumpall', '-f', "$tmp_root/dump1.sql");
-	system(@args) == 0 or exit 1;
-	print "\nStopping old cluster\n\n";
-	system("pg_ctl stop") == 0 or exit 1;
-	$ENV{PGDATA} = "$data";
-	print "\nSetting up new cluster\n\n";
-	standard_initdb() or exit 1;
-	print "\nRunning pg_upgrade\n\n";
-	@args = (
-		'pg_upgrade', '-d', "$data.old", '-D', $data, '-b', $bindir,
-		'--no-sync');
-	system(@args) == 0 or exit 1;
-	print "\nStarting new cluster\n\n";
-	@args = ('pg_ctl', '-l', "$logdir/postmaster2.log", 'start');
-	system(@args) == 0 or exit 1;
-	print "\nDumping new cluster\n\n";
-	@args = ('pg_dumpall', '-f', "$tmp_root/dump2.sql");
-	system(@args) == 0 or exit 1;
-	print "\nStopping new cluster\n\n";
-	system("pg_ctl stop") == 0 or exit 1;
-	print "\nDeleting old cluster\n\n";
-	system(".\\delete_old_cluster.bat") == 0 or exit 1;
-	print "\nComparing old and new cluster dumps\n\n";
-
-	@args = ('diff', '-q', "$tmp_root/dump1.sql", "$tmp_root/dump2.sql");
-	system(@args);
-	$status = $?;
-	if (!$status)
-	{
-		print "PASSED\n";
-	}
-	else
-	{
-		print "dumps not identical!\n";
-		exit(1);
-	}
+	InstallTemp();
+	my $mstat = tap_check("$topdir/src/bin/pg_upgrade");
+	exit $mstat if $mstat;
 	return;
 }
 
-- 
2.34.1

---

From: Michael Paquier <[email-masked]>
Date: Wed, 2 Mar 2022 15:57:23 +0900

> buildfarm, but that could be addressed later depending on the versions

The bug related to the detection of Windows and temporary paths for
pg_upgrade's server.c has been fixed as of dc57366, so attached is the
remaining rebased piece as perl2host has been recently removed.

Do others have an opinion about a backpatch of the bugfix?  Nobody has
complained about that since pg_upgrade exists, so I have just done the
change on HEAD.
--
Michael
From 66b6961866d215344aa836963e5bdbfb237ed606 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Wed, 2 Mar 2022 15:55:32 +0900
Subject: [PATCH v10] Switch tests of pg_upgrade to use TAP

---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 251 ++++++++++++++++++++++
 6 files changed, 290 insertions(+), 447 deletions(-)

 

 

 
 
-
-DETAILS
--------
-
index 0000000000..7e8b48497b
@@ -0,0 +1,251 @@
+	my $dlpath = dirname($ENV{REGRESS_SHLIB});
+	my $outputdir = $PostgreSQL::Test::Utils::tmp_check;
+	my $inputdir = "$srcdir/src/test/regress";
+			"$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
index e2b0db0879..a5f39f02d3 100644
@@ -286,6 +286,10 @@ sub bincheck
@@ -590,91 +594,9 @@ sub generate_db
 
 sub upgradecheck
 {
-	my $status;
-	my $cwd = getcwd();
-
 
-- 
2.35.1

---

From: Andres Freund <[email-masked]>
Date: Wed, 2 Mar 2022 00:07:29 -0800

Hi,

> complained about that since pg_upgrade exists, so I have just done the

WFM.

> +++ b/src/bin/pg_upgrade/t/001_basic.pl

Outdated.

> +program_options_handling_ok('pg_upgrade');

Unrelated. But I kinda wish we'd do this in a saner manner than copying this
test into every binary. E.g. by ensuring that all tools installed in the temp
install are tested or such.

> +# created, and is used as the target instance for the upgrade.

This seems a bit repetitive. Lots of "instance".

> Before

Odd indentation. Spaces between parens?

> +$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);

I'd copy the comments from test.sh wrt --wal-segsize,
--allow-group-access.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Thu, 3 Mar 2022 14:03:38 +0900

>> +use Test::More tests => 8;

Fixed.

> install are tested or such.

Perhaps.  I am sticking with the existing style for now.

> This seems a bit repetitive. Lots of "instance".

Indeed.  I have reworked the whole, rather than just those three
sentences.

> Odd indentation. Spaces between parens?

Well, perltidy tells me that this is right.

> I'd copy the comments from test.sh wrt --wal-segsize,

Done.
--
Michael
From ba2cf7a7adf9cf50406d8bbcaa12167137ba29c3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Thu, 3 Mar 2022 14:01:56 +0900
Subject: [PATCH v11] Switch tests of pg_upgrade to use TAP

---
 src/bin/pg_upgrade/Makefile            |  21 +-
 src/bin/pg_upgrade/TESTING             |  85 ++------
 src/bin/pg_upgrade/t/001_basic.pl      |  11 +
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 248 ++++++++++++++++++++++
 src/bin/pg_upgrade/test.sh             | 279 -------------------------
 src/tools/msvc/vcregress.pl            |  92 +-------
 6 files changed, 289 insertions(+), 447 deletions(-)
 create mode 100644 src/bin/pg_upgrade/t/001_basic.pl
 create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl
 delete mode 100644 src/bin/pg_upgrade/test.sh

 

 

 
 
-
-DETAILS
--------
-
index 0000000000..40458f10b6
@@ -0,0 +1,11 @@
index 0000000000..7a256844da
@@ -0,0 +1,248 @@
+# The test of pg_upgrade consists requires two clusters, an old one and
+# and a new one, that gets upgraded.  Before running the upgrade, a logical
+# dump of the old cluster is taken, and a second logical dump of the new one
+# is taken after the upgrade.  The upgrade test passes if there are no
+# differences in these two dumps.
+# Testing upgrades with an older version of PostgreSQL requires setting up
+# - "olddump", to point to a dump file that will be used to set up the old
+#   instance to upgrade from, the dump being restored in the old cluster.
+# - "oldinstall", to point to the installation path of the old cluster.
+# To increase coverage of non-standard segment size and group access without
+# increasing test runtime, run these tests with a custom setting.
+# --allow-group-access and --wal-segsize have been added in v11.
+# The default location of the source code is the root of this directory.
+# Set up the data of the old instance with a dump or pg_regress.
+	# the old cluster when using it.  In the default case, fallback to
+		@extra_opts,
+			'psql',   '-X',
+			'-f',     "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
+			'--port', $oldnode->port,
+			'--host', $oldnode->host,
 
 sub upgradecheck
 {
-	my $status;
-	my $cwd = getcwd();
-
 
-- 
2.35.1

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 3 Mar 2022 08:16:13 -0500

> Well, perltidy tells me that this is right.

Yeah, I haven't found a way to make it stop doing that :-(

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Michael Paquier <[email-masked]>
Date: Thu, 31 Mar 2022 13:10:43 +0900

> Indeed.  I have reworked the whole, rather than just those three

So, any particular feelings about this patch?  This has been around
for a couple of months/years now, so it could be a good time to do the
switch now rather than wait an extra year, or even the beginning of
the next release cycle.  And the buildfarm is already able to handle
that in its code based on the last release, by skipping the upgrade
check if it finds a pg_upgrade/t/ subdirectory.
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Thu, 31 Mar 2022 01:00:14 -0400

> check if it finds a pg_upgrade/t/ subdirectory.

There's still about a third of the buildfarm running older
client releases --- I count

      2 REL_8
      2 REL_10
     13 REL_11
      6 REL_12
     16 REL_13.1
     89 REL_14

How well does this patch work with pre-14 buildfarm clients?

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Thu, 31 Mar 2022 01:25:17 -0400

>      89 REL_14

Wait a minute ... actually, what's most relevant here is
the population running TAP tests, which seems to be

      2 REL_8
      4 REL_11
      1 REL_12
      7 REL_13.1
     53 REL_14

So there are still some people we'd have to nag if it doesn't
work pre-v14, but fewer than I thought --- specifically,
the owners of

butterflyfish
copperhead
eelpout
elver
halibut
kittiwake
mantid
marabou
massasauga
myna
snakefly
snapper
spurfowl
tadarida

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 30 Mar 2022 22:36:16 -0700

> How well does this patch work with pre-14 buildfarm clients?

Looks to me like it'll just run the test twice, once via TestUpgrade, once via
taptest. It's possible that there could be trouble somehow due to duplicated
log files or something?

---

From: Michael Paquier <[email-masked]>
Date: Thu, 31 Mar 2022 16:56:36 +0900

> taptest. It's possible that there could be trouble somehow due to duplicated

Hmm.  TestUpgrade.pm also uses tmp_check/, and the TAP tests would
remove this path before running.  Still, all the contents of the logs
would be printed out before moving to the next tests at the end of
check-pg_upgrade.  It does not seem like this double run is going to
be an issue on this side. 
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Thu, 31 Mar 2022 09:49:50 -0400

> be an issue on this side. 

Well, let's go ahead with it and see what happens.  If it's too
much of a mess we can always revert.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Fri, 1 Apr 2022 10:16:48 +0900

> much of a mess we can always revert.

Okay, done after an extra round of self-review.  I have finished by
tweaking a couple of comments, and adjusted further TESTING to explain
what needs to be done to have a dump compatible with the test.  Let's
now see what goes wrong.
--
Michael

---

From: Noah Misch <[email-masked]>
Date: Thu, 31 Mar 2022 20:42:41 -0700

> what needs to be done to have a dump compatible with the test.  Let's

The REL_14 buildfarm client did not grab logs from the first failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse&dt=2022-04-01%2001%3A39%3A04

The failure looked like this:

# Running: diff -q /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_lPFv/dump1.sql /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_lPFv/dump2.sql
/usr/bin/diff: illegal option -- q
usage: diff [-bitw] [-c | -e | -f | -h | -n | -u] file1 file2
       diff [-bitw] [-C number | -U number] file1 file2
       diff [-bitw] [-D string] file1 file2
       diff [-bitw] [-c | -e | -f | -h | -n | -u] [-l] [-r] [-s] [-S name] directory1 directory2
not ok 4 - old and new dump match after pg_upgrade

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 31 Mar 2022 22:51:59 -0500

> not ok 4 - old and new dump match after pg_upgrade

Is diff -q defined somewhere ?  I can't find it in postgres sources nor in
sources for bf client.

Maybe your bf member could use git diff --exit-code --quiet ?

-- 
Justin

---

From: Michael Paquier <[email-masked]>
Date: Fri, 1 Apr 2022 12:54:32 +0900

> what needs to be done to have a dump compatible with the test.  Let's

So, the first reports are published, and the buildfarm is rather cool
on the matter.  wrasse is the only buildfarm member that has reported
a failure, complaining that the dumps generated do not match.  I am
not completely sure what's going on there, so I have applied an extra
patch to get more information from the logs on failures, and switched
the test to use File::Compare::compare() to check if the dumps match.
This last part feels safer in the long run, anyway.  There should be a
diff command as previous runs used test.sh, so perhaps this is an
issue with its perl.  The next report should tell more.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Fri, 1 Apr 2022 13:00:20 +0900

> not ok 4 - old and new dump match after pg_upgrade

Ah, thanks for the information!  So the problem was that the first
commit of the patch took the diff command from the MSVC scripts, and
there is no -q on Solaris 11.3.  Using File::Compare should be enough
to fix the problem, then.  Hopefully.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Fri, 1 Apr 2022 15:01:38 +0900

> Is diff -q defined somewhere ?  I can't find it in postgres sources nor in

322becb has added such a call, at the end of 002_pg_upgrade.pl.
vcregress.pl also has one before this commit.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Fri, 1 Apr 2022 20:53:10 +0900

> vcregress.pl also has one before this commit.

The Windows animals seem to be in good shape, except hamerkop that
dies on "vcregress upgradecheck" when the TAP tests are disabled
causing the buildfarm client to stop.  My idea to use upgradecheck
leads to more code than just moving the test to bincheck so let's
reuse the suggestion from Andres upthread and disable completely
upgradecheck, keeping the target around only for compatibility.  The
attached does that, and the test of pg_upgrade would go through
bincheck instead.

It is late here, I'll try to get that patched up tomorrow.
--
Michael
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 43d05bde4e..18101e7a70 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -470,7 +470,6 @@ $ENV{CONFIG}="Debug";
 <userinput>vcregress isolationcheck</userinput>
 <userinput>vcregress bincheck</userinput>
 <userinput>vcregress recoverycheck</userinput>
-<userinput>vcregress upgradecheck</userinput>
 </screen>
 
    To change the schedule used (default is parallel), append it to the
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index e896820ac5..fbfc781508 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -106,7 +106,7 @@ my %command = (
 	ISOLATIONCHECK => \&isolationcheck,
 	BINCHECK       => \&bincheck,
 	RECOVERYCHECK  => \&recoverycheck,
-	UPGRADECHECK   => \&upgradecheck,
+	UPGRADECHECK   => \&upgradecheck, # no-op
 	TAPTEST        => \&taptest,);
 
 my $proc = $command{$what};
@@ -286,9 +286,6 @@ sub bincheck
 	foreach my $dir (@bin_dirs)
 	{
 		next unless -d "$dir/t";
-		# Do not consider pg_upgrade, as it is handled by
-		# upgradecheck.
-		next if ($dir =~ "/pg_upgrade/");
 
 		my $status = tap_check($dir);
 		$mstat ||= $status;
@@ -520,9 +517,9 @@ sub generate_db
 
 sub upgradecheck
 {
-	InstallTemp();
-	my $mstat = tap_check("$topdir/src/bin/pg_upgrade");
-	exit $mstat if $mstat;
+	# pg_upgrade is now handled by bincheck, but keep this
+	# target for backward compatibility.
+	print "upgradecheck disabled\n";
 	return;
 }

---

From: Noah Misch <[email-masked]>
Date: Sun, 1 May 2022 21:27:18 -0700

On Fri, Apr 01, 2022 at 10:16:48AM +0900, Michael Paquier wrote:
> Okay, done after an extra round of self-review.

> +		[ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]

Nothing checks the command result, so the test file passes even if each of
these createdb calls fails.  Other run_log() calls in this file have the same
problem.  This particular one should be command_ok() or similar.

--host and --port are redundant in a PostgreSQL::Test::Cluster::run_log call,
because that call puts equivalent configuration in the environment.  Other
calls in the file have the same redundant operands.  (No other test file has
redundant --host or --port.)

> +	my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";

Typo: s/_OPT/_OPTS/

> +	my @extra_opts     = split(/\s+/, $extra_opts_val);

src/test/recovery/t/027_stream_regress.pl and the makefiles treat
EXTRA_REGRESS_OPTS as a shell fragment.  To be compatible, use the
src/test/recovery/t/027_stream_regress.pl approach.  Affected usage patetrns
are not very important, but since the tree has code for it, you may as well
borrow that code.  These examples witness the difference:

EXTRA_REGRESS_OPTS='--nosuc" h"' MAKEFLAGS= make -C src/bin/pg_upgrade check PROVE_TESTS=t/002_pg_upgrade.pl
# log has: /home/nm/src/pg/postgresql/src/bin/pg_upgrade/../../../src/test/regress/pg_regress: unrecognized option '--nosuc"'
EXTRA_REGRESS_OPTS='--nosuc" h"' MAKEFLAGS= make -C src/test/recovery check PROVE_TESTS=t/027_stream_regress.pl
# log has: /home/nm/src/pg/postgresql/src/test/recovery/../../../src/test/regress/pg_regress: unrecognized option '--nosuc h'

> -dbname1='\"\'$dbname1'\\"\\\'

This rewrite dropped the exercise of backslashes adjacent to double quotes.

---

From: Michael Paquier <[email-masked]>
Date: Mon, 2 May 2022 16:00:53 +0900

> commit 322becb wrote:

Thanks, Noah.  I am out this week, but I should be able to address all
your points at the beginning of next week.  I have added an open item
for now.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Mon, 9 May 2022 12:18:39 +0900

> problem.  This particular one should be command_ok() or similar.

All of them could rely on command_ok(), as they should never fail, so
switched this way.

> redundant --host or --port.)

Right.  Removed all that.

>> +	my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";

Oops, fixed.

> borrow that code.  These examples witness the difference:

So the pattern of EXTRA_REGRESS_OPTS being used in the Makefiles is
the decision point here.  Makes sense.

> This rewrite dropped the exercise of backslashes adjacent to double quotes.

Damn, thanks.  If I am reading that right, this could be done with the
following addition in generate_db(), adding double quotes surrounded
by backslashes before and after the database name: 
$dbname = '\\"\\' . $dbname . '\\"\\';

All these fixes lead me to the attached patch.  Does that look fine to
you?

Thanks,
--
Michael
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 05bf161843..4002982dc0 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -21,9 +21,11 @@ sub generate_db
 		next if $i == 7 || $i == 10 || $i == 13;    # skip BEL, LF, and CR
 		$dbname = $dbname . sprintf('%c', $i);
 	}
-	$node->run_log(
-		[ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]
-	);
+
+	# Exercise backslashes adjacent to double quotes, a Windows special
+	# case.
+	$dbname = '\\"\\' . $dbname . '\\"\\';
+	$node->command_ok([ 'createdb', $dbname ]);
 }
 
 # The test of pg_upgrade requires two clusters, an old one and a new one
@@ -70,12 +72,7 @@ if (defined($ENV{olddump}))
 
 	# Load the dump using the "postgres" database as "regression" does
 	# not exist yet, and we are done here.
-	$oldnode->command_ok(
-		[
-			'psql',   '-X',           '-f',     $olddumpfile,
-			'--port', $oldnode->port, '--host', $oldnode->host,
-			'postgres'
-		]);
+	$oldnode->command_ok([ 'psql', '-X', '-f', $olddumpfile, 'postgres' ]);
 }
 else
 {
@@ -87,8 +84,7 @@ else
 	generate_db($oldnode, 91, 127);
 
 	# Grab any regression options that may be passed down by caller.
-	my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
-	my @extra_opts     = split(/\s+/, $extra_opts_val);
+	my $extra_opts = $ENV{EXTRA_REGRESS_OPTS} || "";
 
 	# --dlpath is needed to be able to find the location of regress.so
 	# and any libraries the regression tests require.
@@ -100,19 +96,19 @@ else
 	# --inputdir points to the path of the input files.
 	my $inputdir = "$srcdir/src/test/regress";
 
-	my @regress_command = [
-		$ENV{PG_REGRESS},                             @extra_opts,
-		'--dlpath',                                   $dlpath,
-		'--max-concurrent-tests',                     '20',
-		'--bindir=',                                  '--host',
-		$oldnode->host,                               '--port',
-		$oldnode->port,                               '--schedule',
-		"$srcdir/src/test/regress/parallel_schedule", '--outputdir',
-		$outputdir,                                   '--inputdir',
-		$inputdir
-	];
-
-	my $rc = run_log(@regress_command);
+	my $rc =
+	  system($ENV{PG_REGRESS}
+		  . "$extra_opts "
+		  . "--dlpath=\"$dlpath\" "
+		  . "--bindir= "
+		  . "--host="
+		  . $oldnode->host . " "
+		  . "--port="
+		  . $oldnode->port . " "
+		  . "--schedule=$srcdir/src/test/regress/parallel_schedule "
+		  . "--max-concurrent-tests=20 "
+		  . "--inputdir=\"$inputdir\" "
+		  . "--outputdir=\"$outputdir\"");
 	if ($rc != 0)
 	{
 		# Dump out the regression diffs file, if there is one
@@ -133,12 +129,10 @@ if (defined($ENV{oldinstall}))
 {
 	# Note that upgrade_adapt.sql from the new version is used, to
 	# cope with an upgrade to this version.
-	$oldnode->run_log(
+	$oldnode->command_ok(
 		[
-			'psql',   '-X',
-			'-f',     "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
-			'--port', $oldnode->port,
-			'--host', $oldnode->host,
+			'psql', '-X',
+			'-f',   "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
 			'regression'
 		]);
 }
@@ -232,7 +226,7 @@ if (-d $log_path)
 }
 
 # Second dump from the upgraded instance.
-$newnode->run_log(
+$newnode->command_ok(
 	[
 		'pg_dumpall', '--no-sync',
 		'-d',         $newnode->connstr('postgres'),

---

From: Michael Paquier <[email-masked]>
Date: Wed, 11 May 2022 10:29:44 +0900

On Mon, May 09, 2022 at 12:18:39PM +0900, Michael Paquier wrote:
> All these fixes lead me to the attached patch.

I have applied this stuff as of 7dd3ee5, in time for beta1, and closed
the open item.  One difference is that I've added one backslash
surrounding the double quote at the beginning *and* the end of the
database name in the patch.  However, the original case was different,
with:
- At the beginning of the database name, one backslash before and
after the double quote.
- At the end of the database name, two backslaces before the double
quote and three after the double quote.
--
Michael

---

From: Noah Misch <[email-masked]>
Date: Tue, 10 May 2022 22:32:55 -0700

> quote and three after the double quote.

Why did you discontinue testing the longstanding test database name?

---

From: Michael Paquier <[email-masked]>
Date: Thu, 12 May 2022 14:27:30 +0900

> Why did you discontinue testing the longstanding test database name?

I am not sure what you mean here.  Are you saying that the test should
be changed to prefix each database name by "regression", as it was the
case in test.sh?  Or do you mean that the backslash/double-quote
business should only apply to the first database name and not the
other two, implying that the new generate_db() in 002_pg_upgrade.pl
had better have a $prefix and a $suffix like it was originally
written?
--
Michael

---

From: Noah Misch <[email-masked]>
Date: Sat, 14 May 2022 01:27:28 -0700

> > > quote and three after the double quote.

Here, you describe differences between test.sh and your rewrite of test.sh.

> I am not sure what you mean here.

Here, I requested the rationale for the differences you had just described.
You made a choice to stop testing one list of database names and start testing
a different list of database names.  Why?

> Are you saying that the test should

No, I wasn't saying any of those.  (Later, I may say one or more of those.)

---

From: Michael Paquier <[email-masked]>
Date: Mon, 16 May 2022 14:30:00 +0900

> a different list of database names.  Why?

Because the shape of the new names does not change the test coverage
("regression" prefix or the addition of the double quotes with
backslashes for all the database names), while keeping the code a bit
simpler.  If you think that the older names are more adapted, I have
no objections to use them, FWIW, which is something like the patch
attached would achieve.

This uses the same convention as vcregress.pl before 322becb, but not
the one of test.sh where "regression" was appended to the database
names.
--
Michael
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 8372a85e6e..33a75991d8 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -13,18 +13,16 @@ use Test::More;
 # Generate a database with a name made of a range of ASCII characters.
 sub generate_db
 {
-	my ($node, $from_char, $to_char) = @_;
+	my ($node, $prefix, $from_char, $to_char, $suffix) = @_;
 
-	my $dbname = '';
+	my $dbname = $prefix;
 	for my $i ($from_char .. $to_char)
 	{
 		next if $i == 7 || $i == 10 || $i == 13;    # skip BEL, LF, and CR
 		$dbname = $dbname . sprintf('%c', $i);
 	}
 
-	# Exercise backslashes adjacent to double quotes, a Windows special
-	# case.
-	$dbname = '\\"\\' . $dbname . '\\\\"\\\\\\';
+	$dbname .= $suffix;
 	$node->command_ok([ 'createdb', $dbname ]);
 }
 
@@ -79,10 +77,12 @@ else
 {
 	# Default is to use pg_regress to set up the old instance.
 
-	# Create databases with names covering most ASCII bytes
-	generate_db($oldnode, 1,  45);
-	generate_db($oldnode, 46, 90);
-	generate_db($oldnode, 91, 127);
+	# Create databases with names covering most ASCII bytes.  The
+	# first name exercises backslashes adjacent to double quotes, a
+	# Windows special case.
+	generate_db($oldnode, "\\\"\\", 1,  45,  "\\\\\"\\\\\\");
+	generate_db($oldnode, '',       46, 90,  '');
+	generate_db($oldnode, '',       91, 127, '');
 
 	# Grab any regression options that may be passed down by caller.
 	my $extra_opts = $ENV{EXTRA_REGRESS_OPTS} || "";

---

From: Noah Misch <[email-masked]>
Date: Wed, 18 May 2022 01:03:15 -0700

> the one of test.sh where "regression" was appended to the database

I would have picked the test.sh names, both because test.sh was the senior
implementation and because doing so avoids warnings under
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS.  See the warnings here:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2022-05-18%2000%3A59%3A35&stg=pg_upgrade-check

More-notable line from that same log:
sh: /Users/buildfarm/bf-data/HEAD/pgsql.build/src/bin/pg_upgrade/../../../src/test/regress/pg_regress--port=5678: No such file or directory

Commit 7dd3ee5 adopted much of the 027_stream_regress.pl approach to running
pg_regress, but it didn't grab the "is($rc, 0, 'regression tests pass')"
needed to make defects like that report a failure.

> +	generate_db($oldnode, '',       91, 127, '');

Does this pass on Windows?  I'm 65% confident that released IPC::Run can't
handle this input due to https://github.com/toddr/IPC-Run/issues/142.  If it's
passing for you on Windows, then disregard.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 18 May 2022 18:20:08 +0900

> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2022-05-18%2000%3A59%3A35&stg=pg_upgrade-check

Yes, I saw that.  This did not bother me much as the TAP tests run in
isolation, but I am fine to stick to your option and silence these.

> sh: /Users/buildfarm/bf-data/HEAD/pgsql.build/src/bin/pg_upgrade/../../../src/test/regress/pg_regress--port=5678: No such file or directory

So you are using EXTRA_REGRESS_OPTS, then, and a space is missing from
the first argument of the command used to make that work properly.

> needed to make defects like that report a failure.

Okay, added this one.

> passing for you on Windows, then disregard.

Hmm.  The CI has been passing for me with this name pattern in place,
as of https://github.com/michaelpq/postgres/tree/upgrade_tap_fixes.

Attached is an updated patch to address your concerns.
--
Michael
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 8372a85e6e..86fe1b4d09 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -13,18 +13,16 @@ use Test::More;
 # Generate a database with a name made of a range of ASCII characters.
 sub generate_db
 {
-	my ($node, $from_char, $to_char) = @_;
+	my ($node, $prefix, $from_char, $to_char, $suffix) = @_;
 
-	my $dbname = '';
+	my $dbname = $prefix;

@@ -79,10 +77,12 @@ else
 {
 
-	# Create databases with names covering most ASCII bytes
-	generate_db($oldnode, 1,  45);
-	generate_db($oldnode, 46, 90);
-	generate_db($oldnode, 91, 127);
+	# Create databases with names covering most ASCII bytes.  The
+	# first name exercises backslashes adjacent to double quotes, a
+	# Windows special case.
+	generate_db($oldnode, 'regression\\"\\', 1,  45,   '\\\\"\\\\\\');
+	generate_db($oldnode, 'regression',      46, 90,  '');
+	generate_db($oldnode, 'regression',      91, 127, '');
 
@@ -99,7 +99,7 @@ else
 
 	my $rc =
 	  system($ENV{PG_REGRESS}
-		  . "$extra_opts "
+		  . " $extra_opts "
 		  . "--dlpath=\"$dlpath\" "
 		  . "--bindir= "
 		  . "--host="
@@ -121,6 +121,7 @@ else
 			print "=== EOF ===\n";
 		}
 	}
+	is($rc, 0, 'regression tests pass');
 }
 
 # Before dumping, get rid of objects not existing or not supported in later

---

From: Noah Misch <[email-masked]>
Date: Fri, 20 May 2022 18:28:01 -0700

> Attached is an updated patch to address your concerns.

Looks successful.

---

From: Michael Paquier <[email-masked]>
Date: Sat, 21 May 2022 12:03:55 +0900

> Looks successful.

Thanks a lot for confirming.  I have applied that on HEAD, then.
--
Michael