On Sun, Oct 02, 2022 at 10:02:37AM -0700, Andres Freund wrote:
> This fails tests widely, and has so for a while:
> https://cirrus-ci.com/build/4862820121575424
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3649
> 
> Note that it causes timeouts, which end up chewing up a cfbot "slot" for an
> hour...

Sorry for kicking the can down the road for a too-long time.  Attached
is an updated patch that I have strimmed down to the minimum that
addresses all the issues I want fixed as of the scope of this thread:
- The filtering of the dumps is reduced to a minimum, removing only
comments and empty lines.
- The configuration of the old and new nodes is tweaked so as it is
abvle to handle upgrade from nodes older than 10.
- pg_dumpall is tweaked to use --extra-float-digits=0 for the old
nodes older than 11 to minimize the amount of diffs generated.

That's quite nice in itself, as it becomes possible to use much more
dump patterns loaded as part of the tests.  More filtering rules could
be used, like the part about procedures and functions that I have sent
in the previous versions, but what I have here is enough to make the
test complete with all the versions supported by Cluster.pm.  I am
thinking to get this stuff applied soon before moving on to the PATH
issue.

There is still one issue reported upthread by Justin about the fact
that we can use unexpected commands depending on the node involved.
For example, something like that would build a PATH so as psql from
the old node is used, not from the new node:
$newnode->command_ok(['psql', '-X', '-f', 'blah.sql']);

So with the current Cluster.pm, we could fail upgrade_adapt.sql if the
version upgraded from does not support psql's \if, and I don't think
that we should use a full path to the binary either.  I am not
completely done analyzing that and this deserves a separate thread, as
it impacts all the commands used in TAP tests manipulating nodes from
multiple versions.
--
Michael
From cfbf5133835d2de963156231521993900f963dc1 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 3 Oct 2022 10:51:24 +0900
Subject: [PATCH v4] Add more filtering capabilities in the dumps.

This adds support for some filtering capabilities, for some tests done
down to v9.5.  This allows the tests to pass with v14, while v9.5~13
still generate a few diffs, but these are minimal compared to what
happened before this commit.
---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 100 +++++++++++++++++--------
 1 file changed, 69 insertions(+), 31 deletions(-)

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl 
b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 2f9b13bf0a..ac03ef5734 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -30,6 +30,27 @@ sub generate_db
                "created database with ASCII characters from $from_char to 
$to_char");
 }
 
+# Filter the contents of a dump before its use in a content comparison.
+# This returns the path to the filtered dump.
+sub filter_dump
+{
+       my ($node, $dump_file) = @_;
+       my $dump_contents = slurp_file($dump_file);
+
+       # Remove the comments.
+       $dump_contents =~ s/^\-\-.*//mgx;
+       # Remove empty lines.
+       $dump_contents =~ s/^\n//mgx;
+
+       my $dump_file_filtered = "${dump_file}_filtered";
+       open(my $dh, '>', $dump_file_filtered)
+         || die "opening $dump_file_filtered";
+       print $dh $dump_contents;
+       close($dh);
+
+       return $dump_file_filtered;
+}
+
 # The test of pg_upgrade requires two clusters, an old one and a new one
 # that gets upgraded.  Before running the upgrade, a logical dump of the
 # old cluster is taken, and a second logical dump of the new one is taken
@@ -49,8 +70,10 @@ if (   (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
        die "olddump or oldinstall is undefined";
 }
 
-# Temporary location for the dumps taken
+# Paths to the dumps taken during the tests.
 my $tempdir = PostgreSQL::Test::Utils::tempdir;
+my $dump1_file = "$tempdir/dump1.sql";
+my $dump2_file = "$tempdir/dump2.sql";
 
 # Initialize node to upgrade
 my $oldnode =
@@ -60,7 +83,10 @@ my $oldnode =
 # To increase coverage of non-standard segment size and group access without
 # increasing test runtime, run these tests with a custom setting.
 # --allow-group-access and --wal-segsize have been added in v11.
-$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+my %node_params = ();
+$node_params{extra} = [ '--wal-segsize', '1', '--allow-group-access' ]
+    if $oldnode->pg_version >= 11;
+$oldnode->init(%node_params);
 $oldnode->start;
 
 # The default location of the source code is the root of this directory.
@@ -129,37 +155,39 @@ else
        is($rc, 0, 'regression tests pass');
 }
 
+# Initialize a new node for the upgrade.
+# Initialize a new node for the upgrade.
+my $newnode = PostgreSQL::Test::Cluster->new('new_node');
+$newnode->init(%node_params);
+
+my $newbindir = $newnode->config_data('--bindir');
+my $oldbindir = $oldnode->config_data('--bindir');
+
 # Before dumping, get rid of objects not existing or not supported in later
 # versions. This depends on the version of the old server used, and matters
 # only if different major versions are used for the dump.
 if (defined($ENV{oldinstall}))
 {
-       # Note that upgrade_adapt.sql from the new version is used, to
-       # cope with an upgrade to this version.
-       $oldnode->command_ok(
+       # Note that upgrade_adapt.sql and psql from the new version are used,
+       # to cope with an upgrade to this version.
+       $newnode->command_ok(
                [
                        'psql', '-X',
                        '-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
-                       'regression'
+                       '-d', $oldnode->connstr('regression'),
                ],
                'ran adapt script');
 }
 
-# Initialize a new node for the upgrade.
-my $newnode = PostgreSQL::Test::Cluster->new('new_node');
-$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
-my $newbindir = $newnode->config_data('--bindir');
-my $oldbindir = $oldnode->config_data('--bindir');
-
 # Take a dump before performing the upgrade as a base comparison. Note
 # that we need to use pg_dumpall from the new node here.
-$newnode->command_ok(
-       [
-               'pg_dumpall', '--no-sync',
-               '-d',         $oldnode->connstr('postgres'),
-               '-f',         "$tempdir/dump1.sql"
-       ],
-       'dump before running pg_upgrade');
+my @dump_command = (
+       'pg_dumpall', '--no-sync', '-d', $oldnode->connstr('postgres'),
+       '-f',         $dump1_file);
+# --extra-float-digits is needed when upgrading from a version older than 11.
+push(@dump_command, '--extra-float-digits', '0')
+  if ($oldnode->pg_version < 12);
+$newnode->command_ok(\@dump_command, 'dump before running pg_upgrade');
 
 # After dumping, update references to the old source tree's regress.so
 # to point to the new tree.
@@ -173,7 +201,7 @@ if (defined($ENV{oldinstall}))
        chomp($output);
        my @libpaths = split("\n", $output);
 
-       my $dump_data = slurp_file("$tempdir/dump1.sql");
+       my $dump_data = slurp_file($dump1_file);
 
        my $newregresssrc = "$srcdir/src/test/regress";
        foreach (@libpaths)
@@ -183,7 +211,7 @@ if (defined($ENV{oldinstall}))
                $dump_data =~ s/$libpath/$newregresssrc/g;
        }
 
-       open my $fh, ">", "$tempdir/dump1.sql" or die "could not open dump 
file";
+       open my $fh, ">", $dump1_file or die "could not open dump file";
        print $fh $dump_data;
        close $fh;
 
@@ -284,24 +312,34 @@ if (-d $log_path)
 }
 
 # Second dump from the upgraded instance.
-$newnode->command_ok(
-       [
-               'pg_dumpall', '--no-sync',
-               '-d',         $newnode->connstr('postgres'),
-               '-f',         "$tempdir/dump2.sql"
-       ],
-       'dump after running pg_upgrade');
+@dump_command = (
+       'pg_dumpall', '--no-sync', '-d', $newnode->connstr('postgres'),
+       '-f',         $dump2_file);
+# --extra-float-digits is needed when upgrading from a version older than 11.
+push(@dump_command, '--extra-float-digits', '0')
+  if ($oldnode->pg_version < 12);
+$newnode->command_ok(\@dump_command, 'dump after running pg_upgrade');
+
+# No need to apply filters on the dumps if working on the same version
+# for the old and new nodes.
+my $dump1_filtered = $dump1_file;
+my $dump2_filtered = $dump2_file;
+if ($oldnode->pg_version != $newnode->pg_version)
+{
+       $dump1_filtered = filter_dump($oldnode, $dump1_file);
+       $dump2_filtered = filter_dump($newnode, $dump2_file);
+}
 
 # Compare the two dumps, there should be no differences.
-my $compare_res = compare("$tempdir/dump1.sql", "$tempdir/dump2.sql");
+my $compare_res = compare($dump1_filtered, $dump2_filtered);
 is($compare_res, 0, 'old and new dumps match after pg_upgrade');
 
 # Provide more context if the dumps do not match.
 if ($compare_res != 0)
 {
        my ($stdout, $stderr) =
-         run_command([ 'diff', "$tempdir/dump1.sql", "$tempdir/dump2.sql" ]);
-       print "=== diff of $tempdir/dump1.sql and $tempdir/dump2.sql\n";
+         run_command([ 'diff', $dump1_filtered, $dump2_filtered ]);
+       print "=== diff of $dump1_filtered and $dump2_filtered\n";
        print "=== stdout ===\n";
        print $stdout;
        print "=== stderr ===\n";
-- 
2.37.3

Attachment: signature.asc
Description: PGP signature

Reply via email to