On Wed, Dec 18, 2024 at 7:39 PM Daniel Gustafsson <dan...@yesql.se> wrote:
>
> > On 18 Dec 2024, at 12:28, Ashutosh Bapat <ashutosh.bapat....@gmail.com>
> > wrote:
>
> In general I think it's fine to have such an expensive test gated behind a
> PG_TEST_EXTRA flag, and since it's only run on demand we might as well run it
> for all formats while at it. If this ran just once per week in the buildfarm
> it would still allow us to catch things in time at fairly low overall cost.
>
> > I have rebased my patches on the current HEAD. The test now passes and
> > does not show any new diff or bug.
>
> A few comments on this version of the patch:
>
> + regression run. Not enabled by default because it is time consuming.
> Since this test consumes both time and to some degree diskspace (the
> dumpfiles)
> I wonder if this should be "time and resource consuming".
Done.
>
>
> + if ( $ENV{PG_TEST_EXTRA}
> + && $ENV{PG_TEST_EXTRA} =~ /\bregress_dump_test\b/)
> Should this also test that $oldnode and $newnode have matching pg_version to
> keep this from running in a cross-version upgrade test? While it can be
> argued
> that running this in a cross-version upgrade is breaking it and getting to
> keep
> both pieces, it's also not ideal to run a resource intensive test we know will
> fail. (It can't be done at this exact callsite, just picked to illustrate.)
>
You already wrote it in parenthesis. At the exact callsite $oldnode
and $newnode can not be of different versions. In fact newnode is yet
to be created at this point. But $oldnode has the same version as the
server run from the code. In a cross-version upgrade this test will
not be executed. I am confused as to what this comment is about.
>
> -sub filter_dump
> +sub filter_dump_for_upgrade
> What is the reason for the rename? filter_dump() is perhaps generic but it's
> also local to the upgrade test so it's also not too unclear.
>
In one of the earlier versions of the patch, there was
filter_dump_for_regress or some such function which was used to filter
the dump from the regression database. Name was changed to
differentiate between the two functions. But the new function is now
named as adjust_regress_dumpfile() so this name change is not required
anymore. Reverting it. I have left the comment change since the test
file now has tests for both upgrade and dump/restore.
>
> + my $format_spec = substr($format, 0, 1);
> This doesn't seem great for readability, how about storing the formats and
> specfiers in an array of Perl hashes which can be iterated over with
> descriptive names, like $format{'name'} and $format{'spec'}?
>
Instead of an array of hashes, I used a single hash with format
description as key and format spec as value. Hope that's acceptable.
>
> + || die "opening $dump_adjusted ";
> Please include the errno in the message using ": $!" appended to the error
> message, it could help in debugging.
>
I didn't see this being used with other open calls in the file. For
that matter we are not using $! with open() in many test files. But it
seems useful. Done
> +compare the results of dump and retore tests
> s/retore/restore/
>
Thanks for pointing out. Fixed.
>
> + else
> + {
> + note('first dump file: ' . $dump1);
> + note('second dump file: ' . $dump2);
> + }
> +
> This doesn't seem particularly helpful, if the tests don't fail then printing
> the names won't bring any needed information. What we could do here is to add
> an is() test in compare_dump()s to ensure the filenames differ to catch any
> programmer error in passing in the same file twice.
Good suggestion. Done.
0001 - same as 0001 from previous version
0002 - addresses above comments
--
Best Wishes,
Ashutosh Bapat
From 5ab6dd99438dbb1a77151f5faa0a4104aec5ce74 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com>
Date: Thu, 27 Jun 2024 10:03:53 +0530
Subject: [PATCH 1/2] Test pg_dump/restore of regression objects
002_pg_upgrade.pl tests pg_upgrade of the regression database left
behind by regression run. Modify it to test dump and restore of the
regression database as well.
Regression database created by regression run contains almost all the
database objects supported by PostgreSQL in various states. Hence the
new testcase covers dump and restore scenarios not covered by individual
dump/restore cases. Many regression tests mention tht they leave objects
behind for dump/restore testing. But till now 002_pg_upgrade only tested
dump/restore through pg_upgrade which is different from dump/restore
through pg_dump. Adding the new testcase closes that gap.
Testing dump and restore of regression database makes this test run
longer for a relatively smaller benefit. Hence run it only when
explicitly requested by user by specifying "regress_dump_test" in
PG_TEST_EXTRA.
Note For the reviewer:
The new test has uncovered two bugs so far in one year.
1. Introduced by 14e87ffa5c54. Fixed in fd41ba93e4630921a72ed5127cd0d552a8f3f8fc.
2. Introduced by 0413a556990ba628a3de8a0b58be020fd9a14ed0. Reverted in 74563f6b90216180fc13649725179fc119dddeb5.
Multiple tests compare pg_dump outputs taken from two clusters in plain
format as a way to compare the contents of those clusters. Add
PostreSQL::Test::Utils::compare_dumps() to standardize and modularize
the comparison.
Author: Ashutosh Bapat
Reviewed by: Michael Pacquire, Tom Lane
Discussion: https://www.postgresql.org/message-id/CAExHW5uF5V=Cjecx3_Z=7xfh4rg2wf61pt+hfquzjbqourz...@mail.gmail.com
---
doc/src/sgml/regress.sgml | 11 ++
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 145 +++++++++++++++++---
src/test/perl/Makefile | 2 +
src/test/perl/PostgreSQL/Test/AdjustDump.pm | 122 ++++++++++++++++
src/test/perl/PostgreSQL/Test/Utils.pm | 48 +++++++
src/test/perl/meson.build | 1 +
src/test/recovery/t/027_stream_regress.pl | 14 +-
7 files changed, 315 insertions(+), 28 deletions(-)
create mode 100644 src/test/perl/PostgreSQL/Test/AdjustDump.pm
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index f4cef9e80f7..4be5d2d7d52 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -336,6 +336,17 @@ make check-world PG_TEST_EXTRA='kerberos ldap ssl load_balance libpq_encryption'
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><literal>regress_dump_test</literal></term>
+ <listitem>
+ <para>
+ When enabled, <filename>src/bin/pg_upgrade/t/002_pg_upgrade.pl</filename>
+ tests dump and restore of regression database left behind by the
+ regression run. Not enabled by default because it is time consuming.
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
Tests for features that are not supported by the current build
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 82a82a1841a..42b68527146 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -6,13 +6,13 @@ use warnings FATAL => 'all';
use Cwd qw(abs_path);
use File::Basename qw(dirname);
-use File::Compare;
-use File::Find qw(find);
-use File::Path qw(rmtree);
+use File::Find qw(find);
+use File::Path qw(rmtree);
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use PostgreSQL::Test::AdjustUpgrade;
+use PostgreSQL::Test::AdjustDump;
use Test::More;
# Can be changed to test the other modes.
@@ -36,9 +36,9 @@ 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
+# Filter the contents of a dump before its use in a content comparison for
+# upgrade testing. This returns the path to the filtered dump.
+sub filter_dump_for_upgrade
{
my ($is_old, $old_version, $dump_file) = @_;
my $dump_contents = slurp_file($dump_file);
@@ -262,6 +262,20 @@ else
}
}
is($rc, 0, 'regression tests pass');
+
+ # Test dump/restore of the objects left behind by regression. Ideally it
+ # should be done in a separate TAP test, but doing it here saves us one full
+ # regression run.
+ #
+ # This step takes several extra seconds. Do it only when requested so as to
+ # avoid spending those extra seconds in every check-world run.
+ #
+ # Do this while the old cluster is running before the upgrade.
+ if ( $ENV{PG_TEST_EXTRA}
+ && $ENV{PG_TEST_EXTRA} =~ /\bregress_dump_test\b/)
+ {
+ test_regression_dump_restore($oldnode, %node_params);
+ }
}
# Initialize a new node for the upgrade.
@@ -511,24 +525,115 @@ push(@dump_command, '--extra-float-digits', '0')
$newnode->command_ok(\@dump_command, 'dump after running pg_upgrade');
# Filter the contents of the dumps.
-my $dump1_filtered = filter_dump(1, $oldnode->pg_version, $dump1_file);
-my $dump2_filtered = filter_dump(0, $oldnode->pg_version, $dump2_file);
+my $dump1_filtered =
+ filter_dump_for_upgrade(1, $oldnode->pg_version, $dump1_file);
+my $dump2_filtered =
+ filter_dump_for_upgrade(0, $oldnode->pg_version, $dump2_file);
# Compare the two dumps, there should be no differences.
-my $compare_res = compare($dump1_filtered, $dump2_filtered);
-is($compare_res, 0, 'old and new dumps match after pg_upgrade');
+compare_dumps($dump1_filtered, $dump2_filtered,
+ 'old and new dumps match after pg_upgrade');
+
+# Test dump and restore of objects left behind by the regression run.
+#
+# It is expected that regression tests, which create `regression` database, are
+# run on `src_node`, which in turn is left in running state. The dump is
+# restored on a fresh node created using given `node_params`. Plain dumps from
+# both the nodes are compared to make sure that all the dumped objects are
+# restored faithfully.
+sub test_regression_dump_restore
+{
+ my ($src_node, %node_params) = @_;
+ my $dst_node = PostgreSQL::Test::Cluster->new('dst_node');
+
+ # Dump the original database for comparison later.
+ my $src_dump = get_dump_for_comparison($src_node->connstr('regression'),
+ 'src_dump', 1);
+
+ # Setup destination database
+ $dst_node->init(%node_params);
+ $dst_node->start;
+
+ for my $format ('plain', 'tar', 'directory', 'custom')
+ {
+ my $dump_file = "$tempdir/regression_dump.$format";
+ my $format_spec = substr($format, 0, 1);
+ my $restored_db = 'regression_' . $format;
+
+ # Even though we compare only schema from the original and the restored
+ # database (See get_dump_for_comparison() for details.), we dump and
+ # restore data as well to catch any errors while doing so.
+ command_ok(
+ [
+ 'pg_dump', "-F$format_spec", '--no-sync',
+ '-d', $src_node->connstr('regression'),
+ '-f', $dump_file
+ ],
+ "pg_dump on source instance in $format format");
+
+ $dst_node->command_ok([ 'createdb', $restored_db ],
+ "created destination database '$restored_db'");
+
+ # Restore into destination database.
+ my @restore_command;
+ if ($format eq 'plain')
+ {
+ # Restore dump in "plain" format using `psql`.
+ @restore_command = [
+ 'psql', '-d', $dst_node->connstr($restored_db),
+ '-f', $dump_file
+ ];
+ }
+ else
+ {
+ @restore_command = [
+ 'pg_restore', '-d',
+ $dst_node->connstr($restored_db), $dump_file
+ ];
+ }
+ command_ok(@restore_command,
+ "restore dump taken in $format format on destination instance");
+
+ # Dump restored database for comparison
+ my $dst_dump =
+ get_dump_for_comparison($dst_node->connstr($restored_db),
+ 'dest_dump.' . $format, 0);
+
+ compare_dumps($src_dump, $dst_dump,
+ "dump outputs of original and restored regression database, using $format format match"
+ );
+ }
+}
-# Provide more context if the dumps do not match.
-if ($compare_res != 0)
+# Dump database pointed by given connection string in plain format and adjust it
+# to compare dumps from original and restored database.
+#
+# file_prefix is used to create unique names for all dump files, so that they
+# remain available for debugging in case the test fails.
+#
+# The name of the file containting adjusted dump is returned.
+sub get_dump_for_comparison
{
- my ($stdout, $stderr) =
- run_command([ 'diff', '-u', $dump1_filtered, $dump2_filtered ]);
- print "=== diff of $dump1_filtered and $dump2_filtered\n";
- print "=== stdout ===\n";
- print $stdout;
- print "=== stderr ===\n";
- print $stderr;
- print "=== EOF ===\n";
+ my ($connstr, $file_prefix, $adjust_child_columns) = @_;
+
+ my $dumpfile = $tempdir . '/' . $file_prefix . '.sql';
+ my $dump_adjusted = "${dumpfile}_adjusted";
+
+
+ # The order of columns in COPY statements dumped from the original database
+ # and that from the restored database differs. These differences are hard to
+ # adjust. Hence we compare only schema dumps for now.
+ command_ok(
+ [ 'pg_dump', '-s', '--no-sync', '-d', $connstr, '-f', $dumpfile ],
+ 'dump for comparison succeeded');
+
+ open(my $dh, '>', $dump_adjusted)
+ || die "opening $dump_adjusted ";
+ print $dh adjust_regress_dumpfile(slurp_file($dumpfile),
+ $adjust_child_columns);
+ close($dh);
+
+ return $dump_adjusted;
}
done_testing();
diff --git a/src/test/perl/Makefile b/src/test/perl/Makefile
index c02f18454e3..91235204c7a 100644
--- a/src/test/perl/Makefile
+++ b/src/test/perl/Makefile
@@ -26,6 +26,7 @@ install: all installdirs
$(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/Cluster.pm '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/Cluster.pm'
$(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/BackgroundPsql.pm '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/BackgroundPsql.pm'
$(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/AdjustUpgrade.pm '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/AdjustUpgrade.pm'
+ $(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/AdjustDump.pm '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/AdjustDump.pm'
$(INSTALL_DATA) $(srcdir)/PostgreSQL/Version.pm '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
uninstall:
@@ -36,6 +37,7 @@ uninstall:
rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/Cluster.pm'
rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/BackgroundPsql.pm'
rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/AdjustUpgrade.pm'
+ rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/AdjustDump.pm'
rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
endif
diff --git a/src/test/perl/PostgreSQL/Test/AdjustDump.pm b/src/test/perl/PostgreSQL/Test/AdjustDump.pm
new file mode 100644
index 00000000000..0b0abb0cefc
--- /dev/null
+++ b/src/test/perl/PostgreSQL/Test/AdjustDump.pm
@@ -0,0 +1,122 @@
+
+# Copyright (c) 2024-2025, PostgreSQL Global Development Group
+
+=pod
+
+=head1 NAME
+
+PostgreSQL::Test::AdjustDump - helper module for dump and restore tests
+
+=head1 SYNOPSIS
+
+ use PostgreSQL::Test::AdjustDump;
+
+ # Adjust contents of dump output file so that dump output from original
+ # regression database and that from the restored regression database match
+ $dump = adjust_regress_dumpfile($dump, $original);
+
+=head1 DESCRIPTION
+
+C<PostgreSQL::Test::AdjustDump> encapsulates various hacks needed to
+compare the results of dump and retore tests
+
+=cut
+
+package PostgreSQL::Test::AdjustDump;
+
+use strict;
+use warnings FATAL => 'all';
+
+use Exporter 'import';
+use Test::More;
+
+our @EXPORT = qw(
+ adjust_regress_dumpfile
+);
+
+=pod
+
+=head1 ROUTINES
+
+=over
+
+=item $dump = adjust_regress_dumpfile($dump, $original)
+
+If we take dump of the regression database left behind after running regression
+tests, restore the dump, and take dump of the restored regression database, the
+outputs of both the dumps differ. Some regression tests purposefully create
+some child tables in such a way that their column orders differ from column
+orders of their respective parents. In the restored database, however, their
+column orders are same as that of their respective parents. Thus the column
+orders of these child tables in the original database and those in the restored
+database differ, causing difference in the dump outputs. See MergeAttributes()
+and dumpTableSchema() for details.
+
+This routine rearranges the column declarations in these C<CREATE TABLE ... INHERITS>
+statements in the dump file from original database to match that from the
+restored database.
+
+Additionally it adjusts blank and new lines to avoid noise.
+
+Arguments:
+
+=over
+
+=item C<dump>: Contents of dump file
+
+=item C<adjust_child_columns>: 1 indicates that the given dump file requires
+adjusting columns in the child tables; usually when the dump is from original
+database. 0 indicates no such adjustment is needed; usually when the dump is
+from restored database.
+
+=back
+
+Returns the adjusted dump text.
+
+=cut
+
+sub adjust_regress_dumpfile
+{
+ my ($dump, $adjust_child_columns) = @_;
+
+ # use Unix newlines
+ $dump =~ s/\r\n/\n/g;
+ # Suppress blank lines, as some places in pg_dump emit more or fewer.
+ $dump =~ s/\n\n+/\n/g;
+
+ # Adjust the CREATE TABLE ... INHERITS statements.
+ if ($adjust_child_columns)
+ {
+ my $saved_dump = $dump;
+
+ $dump =~ s/(^CREATE\sTABLE\sgenerated_stored_tests\.gtestxx_4\s\()
+ (\n\s+b\sinteger),
+ (\n\s+a\sinteger\sNOT\sNULL)/$1$3,$2/mgx;
+
+ ok($saved_dump ne $dump, 'applied gtestxx_4 adjustments');
+
+ $dump =~ s/(^CREATE\sTABLE\spublic\.test_type_diff2_c1\s\()
+ (\n\s+int_four\sbigint),
+ (\n\s+int_eight\sbigint),
+ (\n\s+int_two\ssmallint)/$1$4,$2,$3/mgx;
+
+ ok($saved_dump ne $dump, 'applied test_type_diff2_c1 adjustments');
+
+ $dump =~ s/(CREATE\sTABLE\spublic\.test_type_diff2_c2\s\()
+ (\n\s+int_eight\sbigint),
+ (\n\s+int_two\ssmallint),
+ (\n\s+int_four\sbigint)/$1$3,$4,$2/mgx;
+
+ ok($saved_dump ne $dump, 'applied test_type_diff2_c2 adjustments');
+ }
+
+ return $dump;
+}
+
+=pod
+
+=back
+
+=cut
+
+1;
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 022b44ba22b..6efe5faf77d 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -50,6 +50,7 @@ use Cwd;
use Exporter 'import';
use Fcntl qw(:mode :seek);
use File::Basename;
+use File::Compare;
use File::Find;
use File::Spec;
use File::stat qw(stat);
@@ -89,6 +90,8 @@ our @EXPORT = qw(
command_fails_like
command_checks_all
+ compare_dumps
+
$windows_os
$is_msys2
$use_unix_sockets
@@ -1081,6 +1084,51 @@ sub command_checks_all
=pod
+=item compare_dumps(dump1, dump2, testname)
+
+Test that the given two files match. The files usually contain pg_dump output in
+"plain" format. Output the difference if any.
+
+=over
+
+=item C<dump1> and C<dump2>: Dump files to compare
+
+=item C<testname>: test name
+
+=back
+
+=cut
+
+sub compare_dumps
+{
+ my ($dump1, $dump2, $testname) = @_;
+
+ my $compare_res = compare($dump1, $dump2);
+ is($compare_res, 0, $testname);
+
+ # Provide more context
+ if ($compare_res != 0)
+ {
+ my ($stdout, $stderr) =
+ run_command([ 'diff', '-u', $dump1, $dump2 ]);
+ print "=== diff of $dump1 and $dump2\n";
+ print "=== stdout ===\n";
+ print $stdout;
+ print "=== stderr ===\n";
+ print $stderr;
+ print "=== EOF ===\n";
+ }
+ else
+ {
+ note('first dump file: ' . $dump1);
+ note('second dump file: ' . $dump2);
+ }
+
+ return;
+}
+
+=pod
+
=back
=cut
diff --git a/src/test/perl/meson.build b/src/test/perl/meson.build
index fc9cf971ea3..3a98ac49daa 100644
--- a/src/test/perl/meson.build
+++ b/src/test/perl/meson.build
@@ -14,4 +14,5 @@ install_data(
'PostgreSQL/Test/Cluster.pm',
'PostgreSQL/Test/BackgroundPsql.pm',
'PostgreSQL/Test/AdjustUpgrade.pm',
+ 'PostgreSQL/Test/AdjustDump.pm',
install_dir: dir_pgxs / 'src/test/perl/PostgreSQL/Test')
diff --git a/src/test/recovery/t/027_stream_regress.pl b/src/test/recovery/t/027_stream_regress.pl
index d1ae32d97d6..b5ea1356751 100644
--- a/src/test/recovery/t/027_stream_regress.pl
+++ b/src/test/recovery/t/027_stream_regress.pl
@@ -116,8 +116,9 @@ command_ok(
'--no-sync', '-p', $node_standby_1->port
],
'dump standby server');
-command_ok(
- [ 'diff', $outputdir . '/primary.dump', $outputdir . '/standby.dump' ],
+compare_dumps(
+ $outputdir . '/primary.dump',
+ $outputdir . '/standby.dump',
'compare primary and standby dumps');
# Likewise for the catalogs of the regression database, after disabling
@@ -146,12 +147,9 @@ command_ok(
'regression'
],
'dump catalogs of standby server');
-command_ok(
- [
- 'diff',
- $outputdir . '/catalogs_primary.dump',
- $outputdir . '/catalogs_standby.dump'
- ],
+compare_dumps(
+ $outputdir . '/catalogs_primary.dump',
+ $outputdir . '/catalogs_standby.dump',
'compare primary and standby catalog dumps');
# Check some data from pg_stat_statements.
--
2.34.1
From 74f9a88c6f7ddfe26019dbd50f98c2789029ad9f Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat....@gmail.com>
Date: Fri, 20 Dec 2024 15:22:14 +0530
Subject: [PATCH 2/2] Address comments by Daniel Gustafsson
To be merged with the earlier commit.
---
doc/src/sgml/regress.sgml | 3 ++-
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 14 ++++++--------
src/test/perl/PostgreSQL/Test/AdjustDump.pm | 2 +-
src/test/perl/PostgreSQL/Test/Utils.pm | 7 +++++--
4 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 4be5d2d7d52..60da8eb95e5 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -343,7 +343,8 @@ make check-world PG_TEST_EXTRA='kerberos ldap ssl load_balance libpq_encryption'
<para>
When enabled, <filename>src/bin/pg_upgrade/t/002_pg_upgrade.pl</filename>
tests dump and restore of regression database left behind by the
- regression run. Not enabled by default because it is time consuming.
+ regression run. Not enabled by default because it is time and resource
+ consuming.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 42b68527146..a817ed0d00b 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -38,7 +38,7 @@ sub generate_db
# Filter the contents of a dump before its use in a content comparison for
# upgrade testing. This returns the path to the filtered dump.
-sub filter_dump_for_upgrade
+sub filter_dump
{
my ($is_old, $old_version, $dump_file) = @_;
my $dump_contents = slurp_file($dump_file);
@@ -525,10 +525,8 @@ push(@dump_command, '--extra-float-digits', '0')
$newnode->command_ok(\@dump_command, 'dump after running pg_upgrade');
# Filter the contents of the dumps.
-my $dump1_filtered =
- filter_dump_for_upgrade(1, $oldnode->pg_version, $dump1_file);
-my $dump2_filtered =
- filter_dump_for_upgrade(0, $oldnode->pg_version, $dump2_file);
+my $dump1_filtered = filter_dump(1, $oldnode->pg_version, $dump1_file);
+my $dump2_filtered = filter_dump(0, $oldnode->pg_version, $dump2_file);
# Compare the two dumps, there should be no differences.
compare_dumps($dump1_filtered, $dump2_filtered,
@@ -545,6 +543,7 @@ sub test_regression_dump_restore
{
my ($src_node, %node_params) = @_;
my $dst_node = PostgreSQL::Test::Cluster->new('dst_node');
+ my %dump_formats = ('plain' => 'p', 'tar' => 't', 'directory' => 'd', 'custom' => 'c');
# Dump the original database for comparison later.
my $src_dump = get_dump_for_comparison($src_node->connstr('regression'),
@@ -554,10 +553,9 @@ sub test_regression_dump_restore
$dst_node->init(%node_params);
$dst_node->start;
- for my $format ('plain', 'tar', 'directory', 'custom')
+ while (my ($format, $format_spec) = each %dump_formats)
{
my $dump_file = "$tempdir/regression_dump.$format";
- my $format_spec = substr($format, 0, 1);
my $restored_db = 'regression_' . $format;
# Even though we compare only schema from the original and the restored
@@ -628,7 +626,7 @@ sub get_dump_for_comparison
'dump for comparison succeeded');
open(my $dh, '>', $dump_adjusted)
- || die "opening $dump_adjusted ";
+ || die "could not open $dump_adjusted for writing the adjusted dump: $!";
print $dh adjust_regress_dumpfile(slurp_file($dumpfile),
$adjust_child_columns);
close($dh);
diff --git a/src/test/perl/PostgreSQL/Test/AdjustDump.pm b/src/test/perl/PostgreSQL/Test/AdjustDump.pm
index 0b0abb0cefc..5b9990e4719 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustDump.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustDump.pm
@@ -18,7 +18,7 @@ PostgreSQL::Test::AdjustDump - helper module for dump and restore tests
=head1 DESCRIPTION
C<PostgreSQL::Test::AdjustDump> encapsulates various hacks needed to
-compare the results of dump and retore tests
+compare the results of dump and restore tests
=cut
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 6efe5faf77d..bf56eb4b23c 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -1120,8 +1120,11 @@ sub compare_dumps
}
else
{
- note('first dump file: ' . $dump1);
- note('second dump file: ' . $dump2);
+ # Fail if the comparison succeeds because the files are the same. This
+ # will detect simple programming errors. It won't detect more complex
+ # errors like passing different links pointing to the same underlying
+ # file.
+ ok($dump1 ne $dump2, "dump files being compared are distinct")
}
return;
--
2.34.1
