On Wed, Feb 05, 2025 at 03:28:04PM +0900, Michael Paquier wrote: > Hmm. I was reading through the patch and there is something that > clearly stands out IMO: the new compare_dumps(). It is in Utils.pm, > and it acts as a wrapper of `diff` with its formalized output format. > It is not really about dumps, but about file comparisons. This should > be renamed compare_files(), with internals adjusted as such, and > reused in all the existing tests. Good idea to use that in > 027_stream_regress.pl, actually. I'll go extract that first, to > reduce the presence of `diff` in the whole set of TAP tests.
The result of this part is pretty neat, resulting in 0001 where it is
possible to use the refactored routine as well in pg_combinebackup
where there is a piece comparing dumps. There are three more tests
with diff commands and assumptions of their own, that I've left out.
This has the merit of unifying the output generated should any diffs
show up, while removing a nice chunk from the main patch.
> AdjustDump.pm looks like a fine concept as it stands. I still need to
> think more about it. It feels like we don't have the most optimal
> interface, though, but perhaps that will be clearer once
> compare_dumps() is moved out of the way.
+ my %dump_formats = ('plain' => 'p', 'tar' => 't', 'directory' => 'd',
'custom' => 'c');
No need for this mapping, let's just use the long options.
+ # 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");
The use of command_ok() looks incorrect here. Shouldn't we use
$src_node->command_ok() here to ensure a correct PATH? That would be
more consistent with the other dump commands. Same remark about
@restore_command.
+ # The order of columns in COPY statements dumped from the original database
+ # and that from the restored database differs. These differences are hard
to
What are the relations we are talking about here?
I am attaching the patch set, with 0002 being the main patch adjusted
with the changes of 0001 that I'm planning to apply, before diving
more into the internals of 0002.
--
Michael
From 9dc46db3b024b4c3779a3d4ab3b9add06813bf1e Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 6 Feb 2025 14:31:30 +0900
Subject: [PATCH 1/2] Refactor code for file comparisons in TAP tests
This unifies the output used should any differences be found in the
files provided.
There are a couple of tests that still use directly a diff command:
001_pg_bsd_indent, 017_shm and test_json_parser's 003. These rely on
different properties and are left out for now.
---
.../pg_combinebackup/t/002_compare_backups.pl | 19 +--------
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 22 ++--------
src/test/perl/PostgreSQL/Test/Utils.pm | 40 +++++++++++++++++++
src/test/recovery/t/027_stream_regress.pl | 14 +++----
4 files changed, 52 insertions(+), 43 deletions(-)
diff --git a/src/bin/pg_combinebackup/t/002_compare_backups.pl b/src/bin/pg_combinebackup/t/002_compare_backups.pl
index ebd68bfb850..4ca489b4511 100644
--- a/src/bin/pg_combinebackup/t/002_compare_backups.pl
+++ b/src/bin/pg_combinebackup/t/002_compare_backups.pl
@@ -192,27 +192,12 @@ $pitr2->command_ok(
# Compare the two dumps, there should be no differences other than
# the tablespace paths.
-my $compare_res = compare_text(
+my $compare_res = compare_files(
$dump1, $dump2,
+ "contents of dumps match for both PITRs",
sub {
s{create tablespace .* location .*\btspitr\K[12]}{N}i for @_;
return $_[0] ne $_[1];
});
-note($dump1);
-note($dump2);
-is($compare_res, 0, "dumps are identical");
-
-# Provide more context if the dumps do not match.
-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";
-}
done_testing();
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index e49bff6454a..ddb4c40c2e6 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -6,9 +6,8 @@ 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;
@@ -515,20 +514,7 @@ 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.
-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', '-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 $compare_res = compare_files($dump1_filtered, $dump2_filtered,
+ 'old and new dumps match after pg_upgrade');
done_testing();
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 9c83d93f79f..0c867c179f6 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);
@@ -70,6 +71,7 @@ our @EXPORT = qw(
check_mode_recursive
chmod_recursive
check_pg_config
+ compare_files
dir_symlink
scan_server_header
system_or_bail
@@ -773,6 +775,44 @@ sub check_pg_config
=pod
+=item compare_files(file1, file2, testname)
+
+Check that two files match, printing the difference if any.
+
+C<line_comp_function> is an optional CODE reference to a line comparison
+function, passed down as-is to File::Compare::compare_text.
+
+=cut
+
+sub compare_files
+{
+ my ($file1, $file2, $testname, $line_comp_function) = @_;
+
+ $line_comp_function = sub { $_[0] ne $_[1] }
+ unless defined $line_comp_function;
+
+ my $compare_res =
+ File::Compare::compare_text($file1, $file2, $line_comp_function);
+ is($compare_res, 0, $testname);
+
+ # Provide more context if the files do not match.
+ if ($compare_res != 0)
+ {
+ my ($stdout, $stderr) =
+ run_command([ 'diff', '-u', $file1, $file2 ]);
+ print "=== diff of $file1 and $file2\n";
+ print "=== stdout ===\n";
+ print $stdout;
+ print "=== stderr ===\n";
+ print $stderr;
+ print "=== EOF ===\n";
+ }
+
+ return;
+}
+
+=pod
+
=item dir_symlink(oldname, newname)
Portably create a symlink for a directory. On Windows this creates a junction
diff --git a/src/test/recovery/t/027_stream_regress.pl b/src/test/recovery/t/027_stream_regress.pl
index bab7b28084b..0eac8f66a9c 100644
--- a/src/test/recovery/t/027_stream_regress.pl
+++ b/src/test/recovery/t/027_stream_regress.pl
@@ -120,8 +120,9 @@ command_ok(
'--port' => $node_standby_1->port,
],
'dump standby server');
-command_ok(
- [ 'diff', $outputdir . '/primary.dump', $outputdir . '/standby.dump', ],
+compare_files(
+ $outputdir . '/primary.dump',
+ $outputdir . '/standby.dump',
'compare primary and standby dumps');
# Likewise for the catalogs of the regression database, after disabling
@@ -150,12 +151,9 @@ command_ok(
'regression',
],
'dump catalogs of standby server');
-command_ok(
- [
- 'diff',
- $outputdir . '/catalogs_primary.dump',
- $outputdir . '/catalogs_standby.dump',
- ],
+compare_files(
+ $outputdir . '/catalogs_primary.dump',
+ $outputdir . '/catalogs_standby.dump',
'compare primary and standby catalog dumps');
# Check some data from pg_stat_statements.
--
2.47.2
From 97920995d9d04eeeaaedda00c441900425762030 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Thu, 6 Feb 2025 14:38:59 +0900 Subject: [PATCH 2/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. Till now 002_pg_upgrade only tested dump/restore through pg_upgrade which only uses binary mode. Many regression tests mention that they leave objects behind for dump/restore testing but they are not tested in a non-binary mode. 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. Multiple tests compare pg_dump outputs taken from two clusters in plain format as a way to compare the contents of those clusters. Note For the reviewers: 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. Author: Ashutosh Bapat Reviewed by: Michael Paquier, Daniel Gustafsson, Tom Lane Discussion: https://www.postgresql.org/message-id/CAExHW5uF5V=Cjecx3_Z=7xfh4rg2wf61pt+hfquzjbqourz...@mail.gmail.com --- src/bin/pg_upgrade/t/002_pg_upgrade.pl | 138 +++++++++++++++++++- src/test/perl/Makefile | 2 + src/test/perl/PostgreSQL/Test/AdjustDump.pm | 125 ++++++++++++++++++ src/test/perl/meson.build | 1 + doc/src/sgml/regress.sgml | 12 ++ 5 files changed, 275 insertions(+), 3 deletions(-) create mode 100644 src/test/perl/PostgreSQL/Test/AdjustDump.pm diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl index ddb4c40c2e6..e198615c7cb 100644 --- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl @@ -12,6 +12,7 @@ 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. @@ -35,8 +36,8 @@ 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. +# 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 { my ($is_old, $old_version, $dump_file) = @_; @@ -261,6 +262,21 @@ 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 and some extra disk space, so + # requires an opt-in with the PG_TEST_EXTRA environment variable. + # + # Do this while the old cluster is running before it is shut down by the + # upgrade test. + 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. @@ -514,7 +530,123 @@ 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. -my $compare_res = compare_files($dump1_filtered, $dump2_filtered, +compare_files($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 from +# `src_node` 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'); + my %dump_formats = ('plain' => 'p', 'tar' => 't', 'directory' => 'd', 'custom' => 'c'); + + # Make sure that the source and destination nodes have the same version and + # do not use custom install paths. In both the cases, the dump files may + # require additional adjustments unknown to code here. Do not run this test + # in such a case to avoid utilizing the time and resources unnecessarily. + if ($src_node->pg_version != $dst_node->pg_version or + defined $src_node->{_install_path}) + { + fail("same version dump and restore test using default installation"); + return; + } + + # 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; + + while (my ($format, $format_spec) = each %dump_formats) + { + my $dump_file = "$tempdir/regression_dump.$format"; + 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, + "restored 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_files($src_dump, $dst_dump, + "dump outputs from original and restored regression database (using $format format) match" + ); + } +} + +# Dump database pointed by given connection string `connstr` in plain format and adjust it +# for comparing dumps from the original and the 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. +# +# `adjust_child_columns` is passed to adjust_regress_dumpfile() which actually +# adjusts the dump output. +# +# The name of the file containting adjusted dump is returned. +sub get_dump_for_comparison +{ + 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 "could not open $dump_adjusted for writing the adjusted dump: $!"; + 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 d82fb67540e..def89650ead 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..c232ce2b1a5 --- /dev/null +++ b/src/test/perl/PostgreSQL/Test/AdjustDump.pm @@ -0,0 +1,125 @@ + +# 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, $adjust_child_columns); + +=head1 DESCRIPTION + +C<PostgreSQL::Test::AdjustDump> encapsulates various hacks needed to +compare the results of dump and restore 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, $adjust_child_columns) + +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 the relevant +C<CREATE TABLE... INHERITS> statements in the dump file from original database +to match those from the restored database. We could instead adjust the +statements in the dump from the restored database to match those from original +database or adjust both to a canonical order. But we have chosen to adjust the +statements in the dump from original database for no particular reason. + +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/meson.build b/src/test/perl/meson.build index 58e30f15f9d..492ca571ff8 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/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml index 7c474559bdf..3061ce42fd1 100644 --- a/doc/src/sgml/regress.sgml +++ b/doc/src/sgml/regress.sgml @@ -347,6 +347,18 @@ 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 and resource + consuming. + </para> + </listitem> + </varlistentry> </variablelist> Tests for features that are not supported by the current build -- 2.47.2
signature.asc
Description: PGP signature
