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

Hi,

On 2018-01-26 17:00:26 +0900, Michael Paquier wrote:

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.

Andrew, any comments?

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.

Seems reasonable.

+	# 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;
+	}
+

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.

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.

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

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.

+++ 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
+{

s/_/_random_/?

+# From now on,

Odd phrasing imo.

+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.

Planning to fix it?

+# 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");

Weird comment again.

+# 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.

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

+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);
+}

I know you copied this, but what?

+# Take a dump before performing the upgrade as a base comparison. Note
+# that we need to use pg_dumpall from PATH here.

Whe do we need to?

Greetings,

Andres Freund

On Fri, Mar 2, 2018 at 4:38 PM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2018-01-26 17:00:26 +0900, Michael Paquier wrote:

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.

Andrew, any comments?

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

On Thu, Mar 01, 2018 at 10:08:06PM -0800, Andres Freund wrote:

On 2018-01-26 17:00:26 +0900, Michael Paquier wrote:

+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.

Planning to fix it?

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.

+# 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.

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.

+# 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);
+}

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?

+# Take a dump before performing the upgrade as a base comparison. Note
+# that we need to use pg_dumpall from PATH here.

Whe do we need to?

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

On Fri, Mar 02, 2018 at 06:33:55PM +1030, Andrew Dunstan wrote:

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.

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?

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.

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.

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.

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

I'll post a follow up when I've had a chance to have a good look at
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

Andrew Dunstan wrote:

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.

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

On 1/26/18 03:00, Michael Paquier wrote:

As promised on a recent thread, here is a second tentative to switch
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

On Sun, Mar 4, 2018 at 12:42 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 1/26/18 03:00, Michael Paquier wrote:

As promised on a recent thread, here is a second tentative to switch
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

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

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

On Sun, Mar 4, 2018 at 2:34 PM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On Sun, Mar 4, 2018 at 12:42 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 1/26/18 03:00, Michael Paquier wrote:

As promised on a recent thread, here is a second tentative to switch
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

Something like this should do the trick.

[ tiny patch]

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

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

cheers

andrew

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

On 3/4/18 16:09, Andrew Dunstan wrote:

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.

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

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

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

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 3/4/18 16:09, Andrew Dunstan wrote:

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

I think the dependency is mostly the other way around. How quickly
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

On 3/6/18 4:12 PM, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 3/4/18 16:09, Andrew Dunstan wrote:

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

I think the dependency is mostly the other way around. How quickly
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.

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
david@pgmasters.net

On Wed, Mar 21, 2018 at 10:51:04AM -0400, David Steele wrote:

On 3/6/18 4:12 PM, Tom Lane wrote:
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?

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

On 22 March 2018 at 09:39, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Mar 21, 2018 at 10:51:04AM -0400, David Steele wrote:

On 3/6/18 4:12 PM, Tom Lane wrote:
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?

Even if a lighter version of this patch stripped from multi-version
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

On Thu, Mar 22, 2018 at 09:42:35AM +0800, Craig Ringer wrote:

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.

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

On 3/21/18 21:49, Michael Paquier wrote:

On Thu, Mar 22, 2018 at 09:42:35AM +0800, Craig Ringer wrote:

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.

Okay, in front of such enthusiasm we could keep at least the refactoring
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.

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

On Mon, Apr 02, 2018 at 10:35:09AM -0400, Peter Eisentraut wrote:

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.

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

On Tue, Apr 03, 2018 at 03:04:10PM +0900, Michael Paquier wrote:

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?

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