Rewriting the test of pg_upgrade as a TAP test - take two

From 1294bb18426cea5c0764d0d07846fee291502b73 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 24 Jan 2018 16:19:53 +0900
Subject: [PATCH 1/3] Refactor PostgresNode.pm so as nodes can use custom
 binary path

This is a requirement for having a proper TAP test suite for pg_upgrade
where users have the possibility to manipulate nodes which use different
set of binaries during the upgrade processes.
---
 src/test/perl/PostgresNode.pm | 80 +++++++++++++++++++++++++++++++++----------
 1 file changed, 62 insertions(+), 18 deletions(-)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 1d5ac4ee35..c0892fb8a0 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -133,7 +133,7 @@ INIT
 
 =over
 
-=item PostgresNode::new($class, $name, $pghost, $pgport)
+=item PostgresNode::new($class, $name, $pghost, $pgport, $bindir)
 
 Create a new PostgresNode instance. Does not initdb or start it.
 
@@ -144,13 +144,14 @@ of finding port numbers, registering instances for cleanup, etc.
 
 sub new
 {
-	my ($class, $name, $pghost, $pgport) = @_;
+	my ($class, $name, $pghost, $pgport, $bindir) = @_;
 	my $testname = basename($0);
 	$testname =~ s/\.[^.]+$//;
 	my $self = {
 		_port    => $pgport,
 		_host    => $pghost,
 		_basedir => "$TestLib::tmp_check/t_${testname}_${name}_data",
+		_bindir  => $bindir,
 		_name    => $name,
 		_logfile => "$TestLib::log_path/${testname}_${name}.log" };
 
@@ -284,6 +285,20 @@ sub data_dir
 
 =pod
 
+=item $node->bin_dir()
+
+Returns the path to the binary directory used by this node.
+
+=cut
+
+sub bin_dir
+{
+	my ($self) = @_;
+	return $self->{_bindir};
+}
+
+=pod
+
 =item $node->archive_dir()
 
 If archiving is enabled, WAL files go here.
@@ -328,6 +343,7 @@ sub info
 	open my $fh, '>', \$_info or die;
 	print $fh "Name: " . $self->name . "\n";
 	print $fh "Data directory: " . $self->data_dir . "\n";
+	print $fh "Binary directory: " . $self->bin_dir . "\n";
 	print $fh "Backup directory: " . $self->backup_dir . "\n";
 	print $fh "Archive directory: " . $self->archive_dir . "\n";
 	print $fh "Connection string: " . $self->connstr . "\n";
@@ -401,6 +417,7 @@ sub init
 	my ($self, %params) = @_;
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
+	my $bindir = $self->bin_dir;
 	my $host   = $self->host;
 
 	$params{allows_streaming} = 0 unless defined $params{allows_streaming};
@@ -409,8 +426,8 @@ sub init
 	mkdir $self->backup_dir;
 	mkdir $self->archive_dir;
 
-	TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
-		@{ $params{extra} });
+	TestLib::system_or_bail("$bindir/initdb", '-D', $pgdata, '-A', 'trust',
+		'-N', @{ $params{extra} });
 	TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata);
 
 	open my $conf, '>>', "$pgdata/postgresql.conf";
@@ -502,12 +519,13 @@ sub backup
 {
 	my ($self, $backup_name) = @_;
 	my $backup_path = $self->backup_dir . '/' . $backup_name;
+	my $bindir      = $self->bin_dir;
 	my $port        = $self->port;
 	my $name        = $self->name;
 
 	print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
-	TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p', $port,
-		'--no-sync');
+	TestLib::system_or_bail("$bindir/pg_basebackup", '-D', $backup_path,
+		'-p', $port, '--no-sync');
 	print "# Backup finished\n";
 }
 
@@ -660,11 +678,12 @@ sub start
 	my ($self) = @_;
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
+	my $bindir = $self->bin_dir;
 	my $name   = $self->name;
 	BAIL_OUT("node \"$name\" is already running") if defined $self->{_pid};
 	print("### Starting node \"$name\"\n");
-	my $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
-		$self->logfile, 'start');
+	my $ret = TestLib::system_log("$bindir/pg_ctl", '-D', $self->data_dir,
+		'-l', $self->logfile, 'start');
 
 	if ($ret != 0)
 	{
@@ -693,11 +712,13 @@ sub stop
 	my ($self, $mode) = @_;
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
+	my $bindir = $self->bin_dir;
 	my $name   = $self->name;
 	$mode = 'fast' unless defined $mode;
 	return unless defined $self->{_pid};
 	print "### Stopping node \"$name\" using mode $mode\n";
-	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-m', $mode, 'stop');
+	TestLib::system_or_bail("$bindir/pg_ctl", '-D', $pgdata, '-m', $mode,
+		'stop');
 	$self->_update_pid(0);
 }
 
@@ -714,9 +735,10 @@ sub reload
 	my ($self) = @_;
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
+	my $bindir = $self->bin_dir;
 	my $name   = $self->name;
 	print "### Reloading node \"$name\"\n";
-	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, 'reload');
+	TestLib::system_or_bail("$bindir/pg_ctl", '-D', $pgdata, 'reload');
 }
 
 =pod
@@ -732,10 +754,11 @@ sub restart
 	my ($self)  = @_;
 	my $port    = $self->port;
 	my $pgdata  = $self->data_dir;
+	my $bindir  = $self->bin_dir;
 	my $logfile = $self->logfile;
 	my $name    = $self->name;
 	print "### Restarting node \"$name\"\n";
-	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+	TestLib::system_or_bail("$bindir/pg_ctl", '-D', $pgdata, '-l', $logfile,
 		'restart');
 	$self->_update_pid(1);
 }
@@ -753,10 +776,11 @@ sub promote
 	my ($self)  = @_;
 	my $port    = $self->port;
 	my $pgdata  = $self->data_dir;
+	my $bindir  = $self->bin_dir;
 	my $logfile = $self->logfile;
 	my $name    = $self->name;
 	print "### Promoting node \"$name\"\n";
-	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+	TestLib::system_or_bail("$bindir/pg_ctl", '-D', $pgdata, '-l', $logfile,
 		'promote');
 }
 
@@ -860,13 +884,16 @@ sub _update_pid
 
 =pod
 
-=item PostgresNode->get_new_node(node_name)
+=item PostgresNode->get_new_node(node_name [, $bindir ])
 
 Build a new object of class C<PostgresNode> (or of a subclass, if you have
 one), assigning a free port number.  Remembers the node, to prevent its port
 number from being reused for another node, and to ensure that it gets
 shut down when the test script exits.
 
+<$bindir> can be optionally defined to enforce from where this node should
+use the set of binaries controlling it in the tests.
+
 You should generally use this instead of C<PostgresNode::new(...)>.
 
 For backwards compatibility, it is also exported as a standalone function,
@@ -879,6 +906,7 @@ sub get_new_node
 	my $class = 'PostgresNode';
 	$class = shift if 1 < scalar @_;
 	my $name  = shift;
+	my $bindir = shift;
 	my $found = 0;
 	my $port  = $last_port_assigned;
 
@@ -921,8 +949,21 @@ sub get_new_node
 
 	print "# Found free port $port\n";
 
+	# Find binary directory for this new node.
+	if (!defined($bindir))
+	{
+		# If caller has not defined the binary directory, find it
+		# from pg_config defined in this environment's PATH.
+		my ($stdout, $stderr);
+		my $result = IPC::Run::run [ 'pg_config', '--bindir' ], '>',
+			\$stdout, '2>', \$stderr
+			or die "could not execute pg_config";
+		chomp($stdout);
+		$bindir = $stdout;
+	}
+
 	# Lock port number found by creating a new node
-	my $node = $class->new($name, $test_pghost, $port);
+	my $node = $class->new($name, $test_pghost, $port, $bindir);
 
 	# Add node to list of nodes
 	push(@all_nodes, $node);
@@ -1127,10 +1168,11 @@ sub psql
 
 	my $stdout            = $params{stdout};
 	my $stderr            = $params{stderr};
+	my $bindir            = $self->bin_dir;
 	my $timeout           = undef;
 	my $timeout_exception = 'psql timed out';
 	my @psql_params =
-	  ('psql', '-XAtq', '-d', $self->connstr($dbname), '-f', '-');
+	  ("$bindir/psql", '-XAtq', '-d', $self->connstr($dbname), '-f', '-');
 
 	# If the caller wants an array and hasn't passed stdout/stderr
 	# references, allocate temporary ones to capture them so we
@@ -1275,8 +1317,9 @@ sub poll_query_until
 	my ($self, $dbname, $query, $expected) = @_;
 
 	$expected = 't' unless defined($expected);    # default value
-
-	my $cmd = [ 'psql', '-XAt', '-c', $query, '-d', $self->connstr($dbname) ];
+	my $bindir = $self->bin_dir;
+	my $cmd = [ "$bindir/psql", '-XAt', '-c', $query, '-d',
+				$self->connstr($dbname) ];
 	my ($stdout, $stderr);
 	my $max_attempts = 180 * 10;
 	my $attempts     = 0;
@@ -1666,8 +1709,9 @@ sub pg_recvlogical_upto
 	die 'slot name must be specified' unless defined($slot_name);
 	die 'endpos must be specified'    unless defined($endpos);
 
+	my $bindir = $self->bin_dir;
 	my @cmd = (
-		'pg_recvlogical', '-S', $slot_name, '--dbname',
+		"$bindir/pg_recvlogical", '-S', $slot_name, '--dbname',
 		$self->connstr($dbname));
 	push @cmd, '--endpos', $endpos;
 	push @cmd, '-f', '-', '--no-loop', '--start';
-- 
2.15.1

From df74a70b886bb998ac37195b4d3067c41a012738 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 24 Jan 2018 16:22:00 +0900
Subject: [PATCH 2/3] Add basic TAP test setup for pg_upgrade

The plan is to convert the current pg_upgrade test to the TAP
framework.  This commit just puts a basic TAP test in place so that we
can see how the build farm behaves, since the build farm client has some
special knowledge of the pg_upgrade tests.
---
 src/bin/pg_upgrade/Makefile       | 7 ++++---
 src/bin/pg_upgrade/t/001_basic.pl | 9 +++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 src/bin/pg_upgrade/t/001_basic.pl

diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 1d6ee702c6..e5c98596a1 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -36,8 +36,9 @@ clean distclean maintainer-clean:
 	       pg_upgrade_dump_globals.sql \
 	       pg_upgrade_dump_*.custom pg_upgrade_*.log
 
-check: test.sh all
+check: test.sh
+	$(prove_check)
 	MAKE=$(MAKE) bindir=$(bindir) libdir=$(libdir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< --install
 
-# 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/t/001_basic.pl b/src/bin/pg_upgrade/t/001_basic.pl
new file mode 100644
index 0000000000..605a7f622f
--- /dev/null
+++ b/src/bin/pg_upgrade/t/001_basic.pl
@@ -0,0 +1,9 @@
+use strict;
+use warnings;
+
+use TestLib;
+use Test::More tests => 8;
+
+program_help_ok('pg_upgrade');
+program_version_ok('pg_upgrade');
+program_options_handling_ok('pg_upgrade');
-- 
2.15.1

From f903e01bbb65f1e38cd74b01ee99c4526e4c3b7f Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 26 Jan 2018 16:37:42 +0900
Subject: [PATCH 3/3] Replace pg_upgrade's test.sh by a TAP test

This replacement still allows tests across major versions, though the
interface to reach that has been changed a bit. See TESTING for more
details on the matter.
---
 doc/src/sgml/install-windows.sgml      |   1 -
 src/bin/pg_upgrade/Makefile            |   3 +-
 src/bin/pg_upgrade/TESTING             |   8 +-
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 213 ++++++++++++++++++++++++++
 src/bin/pg_upgrade/test.sh             | 264 ---------------------------------
 src/test/perl/PostgresNode.pm          |   7 +-
 src/tools/msvc/vcregress.pl            | 124 +---------------
 7 files changed, 222 insertions(+), 398 deletions(-)
 create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl
 delete mode 100644 src/bin/pg_upgrade/test.sh

diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 99e9c7b78e..62fc45e517 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -447,7 +447,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/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index e5c98596a1..bccab1d723 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -36,9 +36,8 @@ clean distclean maintainer-clean:
 	       pg_upgrade_dump_globals.sql \
 	       pg_upgrade_dump_*.custom pg_upgrade_*.log
 
-check: test.sh
+check:
 	$(prove_check)
-	MAKE=$(MAKE) bindir=$(bindir) libdir=$(libdir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< --install
 
 installcheck:
 	$(prove_installcheck)
diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING
index 6831f679f6..ee45f6a80f 100644
--- a/src/bin/pg_upgrade/TESTING
+++ b/src/bin/pg_upgrade/TESTING
@@ -9,14 +9,12 @@ 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:
+source tree for the old version as well as this version.  Then do:
 
 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
+export oldlibdir=...otherversion/lib	(old version's installed lib dir)
+make check
 
 In this case, you will have to manually eyeball the resulting dump
 diff for version-specific differences, as explained below.
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..f2c45be284
--- /dev/null
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -0,0 +1,213 @@
+# Set of tests for pg_upgrade.
+use strict;
+use warnings;
+use Cwd;
+use Config;
+use File::Basename;
+use File::Copy;
+use IPC::Run;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 4;
+
+# 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', '--port', $node->port, $dbname ]);
+}
+
+my $startdir = getcwd();
+
+# From now on, the test of pg_upgrade consists in setting up an instance
+# on which regression tests are run. This is the source instance used
+# for the upgrade. Then a new, 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 after running pg_upgrade.
+
+# Determine the version set to use depending on what the caller wants.
+# There are a couple of environment variables to scan for:
+# 1) oldsrc, which points to the code tree of the old instance's binaries
+#    which gets upgraded.
+# 2) oldbindir, which points to the binaries of the old instance to
+#    upgrade.
+# 2) oldlibdir, which points to the libraries of the old instance to
+#    upgrade.
+# If the caller has not defined any of them, default values are used,
+# pointing to the source tree where this script is located.
+my $regress_bin = undef;
+my $newsrc = "../../..";	# top of this code repository
+my $newlibdir;
+my $oldsrc = $newsrc;
+my $oldlibdir = undef;
+my $oldbindir = undef;
+my $dlpath = undef;
+
+my ($stdout, $stderr);
+my $result = IPC::Run::run [ 'pg_config', '--libdir' ], '>',
+	\$stdout, '2>', \$stderr
+	or die "could not execute pg_config";
+chomp($stdout);
+$newlibdir = $stdout;
+
+if (!defined($ENV{oldsrc}) &&
+	!defined($ENV{oldlibdir}) &&
+	!defined($ENV{oldbindir}))
+{
+	# No variables defined, so run the test using this version's
+	# tree for both the new and old instances.
+	$regress_bin = $ENV{PG_REGRESS};
+
+	# Calculate this build's old library directory
+	$oldlibdir = $newlibdir;
+
+	# And this build's old binary directory
+	$result = IPC::Run::run [ 'pg_config', '--bindir' ], '>',
+		\$stdout, '2>', \$stderr
+		or die "could not execute pg_config";
+	chomp($stdout);
+	$oldbindir = $stdout;
+
+	# In this case download path is in src/test/regress.
+	$dlpath = ".";
+}
+elsif (defined($ENV{oldsrc}) &&
+	defined($ENV{oldbindir}) &&
+	defined($ENV{oldlibdir}))
+{
+	# A run is wanted on an old version as base.
+	$oldsrc = $ENV{oldsrc};
+	$oldbindir = $ENV{oldbindir};
+	$oldlibdir = $ENV{oldlibdir};
+	# FIXME: this needs better tuning. Using "." or "$oldlibdir/postgresql"
+	# causes the regression tests to pass but pg_upgrade to fail afterwards.
+	$dlpath = "$oldlibdir";
+	$regress_bin = "$oldsrc/src/test/regress/pg_regress";
+}
+else
+{
+	# Not all variables are defined, so leave and die.
+	die "not all variables in oldsrc, oldlibdir and oldbindir are defined";
+}
+
+# Make sure the installed libraries come first in dynamic load paths.
+$ENV{LD_LIBRARY_PATH}="$newlibdir/postgresql";
+$ENV{DYLD_LIBRARY_PATH}="$newlibdir/postgresql";
+
+# Temporary location for dumps taken
+my $tempdir = TestLib::tempdir;
+
+# Initialize node to upgrade
+my $oldnode = get_new_node('old_node', $oldbindir);
+$oldbindir = $oldnode->bin_dir;	# could be default value
+$oldnode->init(extra => [ '--locale=C', '--encoding=LATIN1' ],
+			   pg_regress => $regress_bin);
+$oldnode->start;
+
+# Creating databases with names covering most ASCII bytes
+generate_db($oldnode, 1,  45);
+generate_db($oldnode, 46, 90);
+generate_db($oldnode, 91, 127);
+
+# Install manually regress.so, this got forgotten in the process.
+copy "$oldsrc/src/test/regress/regress.so",
+	"$oldlibdir/postgresql/regress.so"
+	unless (-e "$oldlibdir/regress.so");
+
+# Run regression tests on the old instance, using the binaries of this
+# instance. At the same time create a tablespace path needed for the
+# tests, similarly to what "make check" creates.
+chdir dirname($regress_bin);
+rmdir "testtablespace";
+mkdir "testtablespace";
+$oldnode->run_log([ "$oldbindir/createdb", '--port', $oldnode->port,
+					'regression' ]);
+
+$oldnode->command_ok([$regress_bin, '--schedule', './serial_schedule',
+		'--dlpath', "$dlpath", '--bindir', $oldnode->bin_dir,
+		'--use-existing', '--port', $oldnode->port],
+		'regression test run on old instance');
+
+# Before dumping, get rid of objects not existing in later versions. This
+# depends on the version of the old server used, and matters only if the
+# old and new source paths
+my $oldpgversion;
+($result, $oldpgversion, $stderr) =
+	$oldnode->psql('postgres', qq[SHOW server_version_num;]);
+my $fix_sql;
+if ($newsrc ne $oldsrc)
+{
+	if ($oldpgversion <= 80400)
+	{
+		$fix_sql = "DROP FUNCTION public.myfunc(integer); DROP FUNCTION public.oldstyle_length(integer, text);";
+	}
+	else
+	{
+		$fix_sql = "DROP FUNCTION public.oldstyle_length(integer, text);";
+	}
+	$oldnode->psql('postgres', $fix_sql);
+}
+
+# Take a dump before performing the upgrade as a base comparison. Note
+# that we need to use pg_dumpall from PATH here.
+$oldnode->command_ok(['pg_dumpall', '--no-sync', '--port', $oldnode->port,
+					  '-f', "$tempdir/dump1.sql"],
+					 'dump before running pg_upgrade');
+
+# After dumping, update references to the old source tree's regress.so and
+# such.
+if ($newsrc ne $oldsrc)
+{
+	if ($oldpgversion <= 80400)
+	{
+		$fix_sql = "UPDATE pg_proc SET probin = replace(probin::text, '$oldsrc', '$newsrc')::bytea WHERE probin LIKE '$oldsrc%';";
+	}
+	else
+	{
+		$fix_sql = "UPDATE pg_proc SET probin = replace(probin, '$oldsrc', '$newsrc') WHERE probin LIKE '$oldsrc%';";
+	}
+	$oldnode->psql('postgres', $fix_sql);
+
+	my $dump_data = slurp_file("$tempdir/dump1.sql");
+	$dump_data =~ s/$oldsrc/$newsrc/g;
+
+	open my $fh, ">", "$tempdir/dump1.sql" or die "could not open dump file";
+	print $fh $dump_data;
+	close $fh;
+}
+
+# Move back to current directory, all logs generated need to be located
+# at the origin.
+chdir $startdir;
+
+# Update the instance.
+$oldnode->stop;
+
+# Initialize a new node for the upgrade.
+my $newnode = get_new_node('new_node');
+$newnode->init(extra => [ '--locale=C', '--encoding=LATIN1' ]);
+
+# Time for the real run.
+chdir "$newsrc/src/test/regress";
+my $newbindir = $newnode->bin_dir;
+command_ok(['pg_upgrade', '-d', $oldnode->data_dir, '-D', $newnode->data_dir,
+	'-b', $oldnode->bin_dir, '-B', $newnode->bin_dir, '-p', $oldnode->port,
+	'-P', $newnode->port], 'run of pg_upgrade for new instance');
+$newnode->start;
+
+# Take a second dump on the upgraded instance.
+run_log(['pg_dumpall', '--no-sync', '--port', $newnode->port,
+		 '-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 checks after pg_upgrade');
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
deleted file mode 100644
index 39983abea1..0000000000
--- a/src/bin/pg_upgrade/test.sh
+++ /dev/null
@@ -1,264 +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-2018, 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 without
-	# increase test runtime, run these tests with a lower setting.
-	"$1" -N --wal-segsize 1
-	if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ]
-	then
-		cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf"
-	fi
-	../../test/regress/pg_regress --config-auth "$PGDATA"
-}
-
-# Establish how the server will listen for connections
-testhost=`uname -s`
-
-case $testhost in
-	MINGW*)
-		LISTEN_ADDRESSES="localhost"
-		PGHOST=localhost
-		;;
-	*)
-		LISTEN_ADDRESSES=""
-		# Select a socket directory.  The algorithm is from the "configure"
-		# script; the outcome mimics pg_regress.c:make_temp_sockdir().
-		PGHOST=$PG_REGRESS_SOCK_DIR
-		if [ "x$PGHOST" = x ]; then
-			{
-				dir=`(umask 077 &&
-					  mktemp -d /tmp/pg_upgrade_check-XXXXXX) 2>/dev/null` &&
-				[ -d "$dir" ]
-			} ||
-			{
-				dir=/tmp/pg_upgrade_check-$$-$RANDOM
-				(umask 077 && mkdir "$dir")
-			} ||
-			{
-				echo "could not create socket temporary directory in \"/tmp\""
-				exit 1
-			}
-
-			PGHOST=$dir
-			trap 'rm -rf "$PGHOST"' 0
-			trap 'exit 3' 1 2 13 15
-		fi
-		;;
-esac
-
-POSTMASTER_OPTS="-F -c listen_addresses=$LISTEN_ADDRESSES -k \"$PGHOST\""
-export PGHOST
-
-# don't rely on $PWD here, as old shells don't set it
-temp_root=`pwd`/tmp_check
-
-if [ "$1" = '--install' ]; then
-	temp_install=$temp_root/install
-	bindir=$temp_install/$bindir
-	libdir=$temp_install/$libdir
-
-	"$MAKE" -s -C ../.. install DESTDIR="$temp_install"
-
-	# platform-specific magic to find the shared libraries; see pg_regress.c
-	LD_LIBRARY_PATH=$libdir:$LD_LIBRARY_PATH
-	export LD_LIBRARY_PATH
-	DYLD_LIBRARY_PATH=$libdir:$DYLD_LIBRARY_PATH
-	export DYLD_LIBRARY_PATH
-	LIBPATH=$libdir:$LIBPATH
-	export LIBPATH
-	SHLIB_PATH=$libdir:$SHLIB_PATH
-	export SHLIB_PATH
-	PATH=$libdir:$PATH
-
-	# We need to make it use psql from our temporary installation,
-	# 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='$bindir'"
-	export EXTRA_REGRESS_OPTS
-fi
-
-: ${oldbindir=$bindir}
-
-: ${oldsrc=../../..}
-oldsrc=`cd "$oldsrc" && pwd`
-newsrc=`cd ../../.. && pwd`
-
-PATH=$bindir:$PATH
-export PATH
-
-BASE_PGDATA=$temp_root/data
-PGDATA="$BASE_PGDATA.old"
-export PGDATA
-rm -rf "$BASE_PGDATA" "$PGDATA"
-
-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
-
-# enable echo so the user can see what is being executed
-set -x
-
-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 "$dbname1" || createdb_status=$?
-createdb "$dbname2" || createdb_status=$?
-createdb "$dbname3" || createdb_status=$?
-
-if "$MAKE" -C "$oldsrc" installcheck; then
-	oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
-
-	# before dumping, get rid of objects not existing in later versions
-	if [ "$newsrc" != "$oldsrc" ]; then
-		fix_sql=""
-		case $oldpgversion in
-			804??)
-				fix_sql="DROP FUNCTION public.myfunc(integer); DROP FUNCTION public.oldstyle_length(integer, text);"
-				;;
-			*)
-				fix_sql="DROP FUNCTION public.oldstyle_length(integer, text);"
-				;;
-		esac
-		psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=$?
-	fi
-
-	pg_dumpall --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
-			804??)
-				fix_sql="UPDATE pg_proc SET probin = replace(probin::text, '$oldsrc', '$newsrc')::bytea WHERE probin LIKE '$oldsrc%';"
-				;;
-			*)
-				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 -d "${PGDATA}.old" -D "${PGDATA}" -b "$oldbindir" -B "$bindir" -p "$PGPORT" -P "$PGPORT"
-
-pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w
-
-case $testhost in
-	MINGW*)	cmd /c analyze_new_cluster.bat ;;
-	*)		sh ./analyze_new_cluster.sh ;;
-esac
-
-pg_dumpall --no-sync -f "$temp_root"/dump2.sql || pg_dumpall2_status=$?
-pg_ctl -m fast stop
-
-# no need to echo commands anymore
-set +x
-echo
-
-if [ -n "$pg_dumpall2_status" ]; then
-	echo "pg_dumpall of post-upgrade database cluster failed"
-	exit 1
-fi
-
-case $testhost in
-	MINGW*)	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/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index c0892fb8a0..2e957965ba 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -422,13 +422,14 @@ sub init
 
 	$params{allows_streaming} = 0 unless defined $params{allows_streaming};
 	$params{has_archiving}    = 0 unless defined $params{has_archiving};
+	$params{pg_regress} = $ENV{PG_REGRESS} unless defined $params{pg_regress};
 
 	mkdir $self->backup_dir;
 	mkdir $self->archive_dir;
 
 	TestLib::system_or_bail("$bindir/initdb", '-D', $pgdata, '-A', 'trust',
 		'-N', @{ $params{extra} });
-	TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata);
+	TestLib::system_or_bail($params{pg_regress}, '--config-auth', $pgdata);
 
 	open my $conf, '>>', "$pgdata/postgresql.conf";
 	print $conf "\n# Added by PostgresNode.pm\n";
@@ -683,7 +684,7 @@ sub start
 	BAIL_OUT("node \"$name\" is already running") if defined $self->{_pid};
 	print("### Starting node \"$name\"\n");
 	my $ret = TestLib::system_log("$bindir/pg_ctl", '-D', $self->data_dir,
-		'-l', $self->logfile, 'start');
+		'-l', $self->logfile, '-w', 'start');
 
 	if ($ret != 0)
 	{
@@ -904,7 +905,7 @@ which can only create objects of class C<PostgresNode>.
 sub get_new_node
 {
 	my $class = 'PostgresNode';
-	$class = shift if 1 < scalar @_;
+	$class = shift if 2 < scalar @_;
 	my $name  = shift;
 	my $bindir = shift;
 	my $found = 0;
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 314f2c37d2..27dc9dd5c5 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -34,7 +34,7 @@ if (-e "src/tools/msvc/buildenv.pl")
 
 my $what = shift || "";
 if ($what =~
-/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|upgradecheck|bincheck|recoverycheck|taptest)$/i
+/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|bincheck|recoverycheck|taptest)$/i
   )
 {
 	$what = uc $what;
@@ -83,7 +83,6 @@ my %command = (
 	ISOLATIONCHECK => \&isolationcheck,
 	BINCHECK       => \&bincheck,
 	RECOVERYCHECK  => \&recoverycheck,
-	UPGRADECHECK   => \&upgradecheck,
 	TAPTEST        => \&taptest,);
 
 my $proc = $command{$what};
@@ -418,126 +417,6 @@ sub standard_initdb
 			$ENV{PGDATA}) == 0);
 }
 
-# This is similar to appendShellString().  Perl system(@args) bypasses
-# cmd.exe, so omit the caret escape layer.
-sub quote_system_arg
-{
-	my $arg = shift;
-
-	# Change N >= 0 backslashes before a double quote to 2N+1 backslashes.
-	$arg =~ s/(\\*)"/${\($1 . $1)}\\"/gs;
-
-	# Change N >= 1 backslashes at end of argument to 2N backslashes.
-	$arg =~ s/(\\+)$/${\($1 . $1)}/gs;
-
-	# Wrap the whole thing in unescaped double quotes.
-	return "\"$arg\"";
-}
-
-# Generate a database with a name made of a range of ASCII characters, useful
-# for testing pg_upgrade.
-sub generate_db
-{
-	my ($prefix, $from_char, $to_char, $suffix) = @_;
-
-	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);
-	}
-	$dbname .= $suffix;
-
-	system('createdb', quote_system_arg($dbname));
-	my $status = $? >> 8;
-	exit $status if $status;
-}
-
-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";
-	(mkdir $tmp_root || die $!) unless -d $tmp_root;
-	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 $logdir = "$topdir/src/bin/pg_upgrade/log";
-	(mkdir $logdir || die $!) unless -d $logdir;
-	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();
-
-	# 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,      '-B', $bindir);
-	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 "\nSetting up stats on new cluster\n\n";
-	system(".\\analyze_new_cluster.bat") == 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);
-	}
-}
-
 sub fetchRegressOpts
 {
 	my $handle;
@@ -647,7 +526,6 @@ sub usage
 	  "  plcheck        run tests of PL languages\n",
 	  "  recoverycheck  run recovery test suite\n",
 	  "  taptest        run an arbitrary TAP test set\n",
-	  "  upgradecheck   run tests of pg_upgrade\n",
 	  "\nOptions for <arg>: (used by check and installcheck)\n",
 	  "  serial         serial mode\n",
 	  "  parallel       parallel mode\n",
-- 
2.15.1