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

From: Michael Paquier <[email-masked]>
Date: Fri, 26 Jan 2018 17:00:26 +0900

Hi all,

As promised on a recent thread, here is a second tentative to switch
pg_upgrade's test.sh into a TAP infrastructure.

This is a continuation of the following thread:
https://www.postgresql.org/message-id/CAB7nPqRdaN1A1YNjxNL9T1jUEWct8ttqq29dNv8W_o37%2Be8wfA%40mail.gmail.com

To begin with, compared to last version, I found and fixed a couple of
bugs, and I also implemented an interface which allows for tests of
pg_upgrade across major versions.  The patch set is as follows:
- 0001 refactors PostgresNode.pm so as a node's binary path is
registered when created and can be enforced by the test creating the
node via get_new_node.  That's useful for pg_upgrade because we want to
use different binary paths for the node to-be-upgraded and the new one.
This is also useful for test suites willing to work on cross-version
tests.
- 0002 begins the move to a TAP suite by introducing a base set of tests
for pg_upgrade.
- 0003 is the real meat, and removes test.sh in favor of a TAP script
aimed at supporting tests using the same version as well as
cross-version tests.

In order to do that, I have changed a bit the interface used for those
tests.  A use can enforce the following variables to run a test using a
past version, while the new version is the one of the tree where the TAP
script is kicked:
export oldsrc=...somewhere/postgresql (old version's source tree)
export oldbindir=...otherversion/bin  (old version's installed bin dir)
export oldlibdir=...otherversion/lib  (old version's installed lib dir)
make check

That is documented as well in TESTING in the patch.  While the patch is
working great when working on the same major version, there is still an
issue I am facing related to the loading of shared libraries in
src/test/regress/ for the old server's version after upgrading it. Even
by tweaking LD_LIBRARY_PATH I could not get the path load. The patch has
been tweaked so as the objects from regression tests that depend on
regress.so, autoinc.so and refint.so are not generated, allowing
pg_upgrade to work, which is definitely wrong.  I am sure that I missed
a trick here but it is Friday here ;p

Note also that the diff of the dump still needs to be eyeballed for two
reasons:
- hash index handling is different in v10~, as those exist now in the
dumps taken.
- PostgreSQL origin version string shows up.
We could apply as well some post-filters to allow the test to pass, but
that's one subject I would like to discuss on this thread.

Another topic that I would like to discuss is how this interface is fit
for the buildfarm code.  After hacking my stuff, I have looked at the
buildfarm code to notice that things like TestUpgradeXversion.pm do
*not* make use of test.sh, and that what I hacked is much similar to
what the buildfarm code is doing, which is itself roughly a copy of what
test.sh does.  Andrew, your feedback would be important here.

So, thoughts?
--
Michael
From 1294bb18426cea5c0764d0d07846fee291502b73 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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

---

From: Andres Freund <[email-masked]>
Date: Thu, 1 Mar 2018 22:08:06 -0800

Hi,

> test.sh does.  Andrew, your feedback would be important here.

Andrew, any comments?

> set of binaries during the upgrade processes.

Seems reasonable.

> +			or die "could not execute pg_config";

This isn't really required, we could just assume things are on PATH if
bindir isn't specified.  Don't have strong feelings about it.

> special knowledge of the pg_upgrade tests.

Not sure I see the point of keeping this separate, but whatever...

> +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl

s/_/_random_/?

> +# From now on,

Odd phrasing imo.

> +	# causes the regression tests to pass but pg_upgrade to fail afterwards.

Planning to fix it?

> +	unless (-e "$oldlibdir/regress.so");

Weird comment again.

> +# tests, similarly to what "make check" creates.

What does "using binaries of this instance" mean? And why?

> +	$oldnode->psql('postgres', $fix_sql);

I know you copied this, but what?

> +# that we need to use pg_dumpall from PATH here.

Whe do we need to?

Greetings,

Andres Freund

---

From: Andrew Dunstan <[email-masked]>
Date: Fri, 2 Mar 2018 18:33:55 +1030

>> test.sh does.  Andrew, your feedback would be important here.

I'll take a look.

Meanwhile:

TestUpgradeXversion.pm is different in a number of respects from the
inbuilt test regime. It doesn't run the normal regression suite to set
up a test. Rather, it saves out the installed binaries and data
directory of the buildfarm client, and then tries to upgrade those
saved data directorories from earlier branches or the current branch
to the target version being tested. It has to make a few adjustments
to the databases along the way. It does a compare of pre- and post-
pg_dumpall runs, allowing a fuzz factor if the versions are different.
The other upside to the scheme is that we're testing pg_dump against
earlier branches as part of testing pg_upgrade.

All this consumes quite a bit of disk space - currently 3.5Gb between
runs on crake. That could probably be reduced some by removing log as
we go.

I don't think a scheme like this is going to be terribly workable
outside some system such as the buildfarm that deals with multiple
branches.

One of the significant pluses to TestUpgradeXversion.pm is that it
tests upgrading quite a bit more than the standard regression
database. It also tests all the contrib databases and the isolation
and pl_regression databases. Several bugs have been found that way,
IIRC, and we should arguably do something along the same lines for our
builtin testing.

I'll post a follow up when I've had a chance to have a good look at
what Michael has actually done.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

---

From: Michael Paquier <[email-masked]>
Date: Fri, 2 Mar 2018 22:02:41 +0900

>> +	# causes the regression tests to pass but pg_upgrade to fail afterwards.

This is something I am not completely sure yet how to move on with.
Those areas would need adjustment once we ave a better idea what the
buildfarm can make use of.

> What does "using binaries of this instance" mean? And why?

This refers to the installation of the instance to upgrade, including
pg_regress installed in pgxs.

> I know you copied this, but what?

Doesn't it matter to be able to test cross upgrades where the instance
to upgrade is 8.4?

>> +# that we need to use pg_dumpall from PATH here.

Yeah, this should refer to the pg_dumpall command from the new
instance.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Fri, 2 Mar 2018 22:11:58 +0900

> earlier branches as part of testing pg_upgrade.

Thanks for the details.  test.sh also does a comparison of the output
of pg_dumpall between the instance before and after upgrade.  The
adjustments done are also close to what test.sh does, and what my patch
does.  This consists in update prosrc for defined functions (+alpha),
right?

> outside some system such as the buildfarm that deals with multiple

Yeah, you need the code tree of the past instance as well as the
regression tests need to be run on the instance to upgrade.

> IIRC, and we should arguably do something along the same lines for our

You have a point here.  I did not consider those parts.

> what Michael has actually done.

I was thinking about that a bit today, and a nice goal would be to come
up with an infrastructure that could be shared between the buildfarm and
the in-core code.  Roughly I think that what I developed could be
changed so as we have TestUpgradeXversion.pm, which the buildfarm could
use, and the in-core TAP tests would be a thin wrapper on top of it.

I doubt also a bit how much people actually use test.sh to run
cross-version upgrades..  5bab1985 is teaching this lesson.
--
Michael

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 2 Mar 2018 11:04:47 -0300

Andrew Dunstan wrote:

> and pl_regression databases.

I think we should definitely be including the src/bin/pg_dump/ test
database in this set of tests (if it isn't already), since that one is
supposed to include comprehensive object type coverage.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

---

From: Peter Eisentraut <[email-masked]>
Date: Sat, 3 Mar 2018 09:12:27 -0500

> pg_upgrade's test.sh into a TAP infrastructure.

AFAICT, this still has the same problem as the previous take, namely
that adding a TAP test suite to the pg_upgrade subdirectory will end up
with the build farm client running the pg_upgrade tests twice.  What we
likely need here is an update to the build farm client in conjunction
with this.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 4 Mar 2018 14:34:41 +1030

> likely need here is an update to the build farm client in conjunction

Something like this should do the trick.

diff --git a/PGBuild/Modules/TestUpgrade.pm b/PGBuild/Modules/TestUpgrade.pm
index 8406d27..05859f9 100644
--- a/PGBuild/Modules/TestUpgrade.pm
+++ b/PGBuild/Modules/TestUpgrade.pm
@@ -50,6 +50,11 @@ sub setup
 "overly long build root $buildroot will cause upgrade problems - try
something shorter than 46 chars"
       if (length($buildroot) > 46);

+   # don't run module if builtin test is enabled.
+    my $using_tap_tests = $conf->using_msvc ? $conf->{config}->{tap_tests} :
+     grep {$_ eq '--enable-tap-tests'} @{$conf->{config}} ;
+   return if $using_tap_tests && -d
"$buildroot/$branch/pgsql/src/bin/pg_upgrade/t";
+
     # could even set up several of these (e.g. for different branches)
     my $self  = {
         buildroot => $buildroot,

cheers

andrew

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 5 Mar 2018 07:39:30 +1030

> Something like this should do the trick.

[ tiny patch]

Pushed with a bug fix. See
<https://github.com/PGBuildFarm/client-code/commit/826d450eff05d15c8bb3c5b2728da5328634a588>

If you want to do this soon I can put out a Buildfarm Client release
fairly quickly.

cheers

andrew

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 6 Mar 2018 15:13:46 -0500

> If you want to do this soon I can put out a Buildfarm Client release

I think the dependency is mostly the other way around.  How quickly
would build farm owners install the upgrade?

---

From: Tom Lane <[email-masked]>
Date: Tue, 06 Mar 2018 16:12:44 -0500

> would build farm owners install the upgrade?

IIUC, the buildfarm script patch is only needed to avoid duplicate
tests.  So owners need only install it if they want to reduce wasted
cycles on their machines.  That being the case, it's only urgent to
the extent that the individual owner perceives it to be.  Some might
think it is so, so I'd like to see the BF release available before
we push the TAP test ... but we don't have to wait very long between.

			regards, tom lane

---

From: David Steele <[email-masked]>
Date: Wed, 21 Mar 2018 10:51:04 -0400

> we push the TAP test ... but we don't have to wait very long between.

It seems the consensus is that we'll need a build farm update before we
can move forward with the patch and that we don't need to wait long for
people to upgrade.

Andrew, do you have a date for the next release?

Thanks,
-- 
-David
[email-masked]

---

From: Michael Paquier <[email-masked]>
Date: Thu, 22 Mar 2018 10:39:57 +0900

> Andrew, do you have a date for the next release?

Even if a lighter version of this patch stripped from multi-version
support (Who uses that actually?) knowing that the buildfarm code has
its own set of modules to do the same may be a good step forward as per
the feedback of this thread people would like to unify the existing
scripts.

We could also try to figure later on how to merge the buildfarm code
into upstream and if it actually makes sense or not, which is something
I am not completely convinced is worth it.  This would remove the need
of any complicated refactoring in PostgresNode.pm to handle binary paths
for a single node, at the cost of of course removing cross-upgrade
checks from the tree.

Still I definitely agree that a release of the buildfarm client should
happen first.  9 days before the end of the commit, the window may be at
the end too short for v11.
--
Michael

---

From: Craig Ringer <[email-masked]>
Date: Thu, 22 Mar 2018 09:42:35 +0800

On 22 March 2018 at 09:39, Michael Paquier <[email-masked]> wrote:

> support (Who uses that actually?)

I'm super excited by the idea of multi-version support in TAP, if that's
what you mean.

Why? Because I use TAP heavily in extensions. Especially replication
extensions. Which like to talk across multiple versions. I currently need
external test frameworks and some hideous hacks to do this.

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

---

From: Michael Paquier <[email-masked]>
Date: Thu, 22 Mar 2018 10:49:57 +0900

> external test frameworks and some hideous hacks to do this.

Okay, in front of such enthusiasm we could keep at least the refactoring
part of PostgresNode.pm :)
--
Michael

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 2 Apr 2018 10:35:09 -0400

> part of PostgresNode.pm :)

I took a quick look at that part.  It appears to be quite invasive, more
than I would have hoped.  Basically, it imposes that from now on all
program invocations must observe the bindir setting, which someone is
surely going to forget.  The pg_upgrade tests aren't going to exercise
all possible paths where programs are called, so this is going to lead
to omissions and inconsistencies -- which will then possibly only be
found much later by the extensions that Craig was talking about.  I'd
like to see this more isolated, maybe via a path change, or something
inside system_or_bail or something less invasive and more maintainable.

---

From: Michael Paquier <[email-masked]>
Date: Tue, 3 Apr 2018 15:04:10 +0900

> inside system_or_bail or something less invasive and more maintainable.

Hm.  PostgresNode.pm uses now a couple of different ways to trigger
commands, which makes the problem harder than it seems:
- system_or_bail
- system_log
- IPC::Run::timeout
- IPC::Run::run

Because of that, and if one wants to minimize the number of places where
bindir is called, would be to modify the command strings themselves.
What I could think of would be to have a wrapper like build_bin_path
which adds $bindir on top of the binary, but that would still cause all
the callers to use this wrapper.  The use of this wrapper could still be
forgotten.

Enforcing PATH temporarily in a code path or another implies that the
previous value of PATH needs to be added back, which could be forgotten
as well.

At the end, it seems to me that what I am proposing is themost readable
approach, and with proper documentation things should be handled
finely...  Or there is an approach you have in mind I do not foresee?
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Mon, 9 Apr 2018 09:38:23 +0900

> finely...  Or there is an approach you have in mind I do not foresee?

With feature freeze which has been reached, I am marking this patch as
returned with feedback.  Please note that I have no plans to resubmit
again this patch set after this second attempt as I have no idea where
we want things to go.
--
Michael