Subject: Re: Test to dump and restore objects left behind by regression

From: Michael Paquier <[email-masked]>
Date: Sun, 9 Feb 2025 16:55:41 +0900

On Fri, Feb 07, 2025 at 07:11:25AM +0900, Michael Paquier wrote:
> Okay, thanks for the feedback.  We have been relying on diff -u for
> the parts of the tests touched by 0001 for some time now, so if there
> are no objections I would like to apply 0001 in a couple of days.

This part has been applied as 169208092f5c.
--
Michael

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 11 Feb 2025 17:53:20 +0530

Hi Michael,

> This part has been applied as 169208092f5c.

Thanks. PFA rebased patches.

I have added another diff adjustment to adjust_regress_dumpfile().
It's introduced by 83ea6c54025bea67bcd4949a6d58d3fc11c3e21b.

On Thu, Feb 6, 2025 at 11:32 AM Michael Paquier <[email-masked]> wrote:
>
> 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.

Without knowing what makes the interface suboptimal, it's hard to make
it optimal. I did think about getting rid of adjust_child_columns
flag. But that either means we adjust CREATE TABLE ... INHERIT
statements from both the dump outputs from original and the restored
database to a canonical form or get rid of the tests in that function
to make sure that the adjustment is required. The first seems more
work (coding and run time). The tests look useful to detect when the
adjustment won't be required.

I also looked at the routines which adjust the dumps from upgrade
tests. They seem to be specific to the older versions and lack the
extensibility you mentioned earlier.

The third thing I looked at was the possibility of applying the
adjustments to only the dump from the original database where it is
required by passing the newline adjustments to compare_files().
However 0002 in the attached set of patches adds more logic,
applicable to both the original and restored dump outputs, to
AdjustDump.pm. So we can't do that either.

I am clueless as to what could be improved here.

>
> +    my %dump_formats = ('plain' => 'p', 'tar' => 't', 'directory' => 'd', 'custom' => 'c');
>
> No need for this mapping, let's just use the long options.

Hmm, didn't realize -F accepts whole format name as well. pg_dump
--help doesn't give that impression but user facing documentation
mentions it. Done.

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

Cluster::command_ok's prologue doesn't mention PATH but mentions
PGHOST and PGPORT.
```
Runs a shell command like PostgreSQL::Test::Utils::command_ok, but
with PGHOST and PGPORT set
so that the command will default to connecting to this PostgreSQL::Test::Cluster
```
According to sub _get_env(), PATH is set only when custom install path
is provided. In the absence of that, build path is used. In this case,
the source and destination nodes are created from the build itself, so
no separate path is required. PGHOST and PGPORT are anyway overridden
by the connection string fetched from the node. So I don't think
there's any correctness issue here, but it's better to use
Cluster::command_ok() just for better readability. Done

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

These are the child tables whose parent has added a column after being
inherited. Now that I have more expertise with perl regex, I have
added code in AdjustDump.pm to remove only the COPY statements where
we see legitimate difference. Added a comment explaining the cause
behind the difference. This patch is supposed to be merged into 0001
before committing the change.

--
Best Wishes,
Ashutosh Bapat
From 42ddf5e9fd6ea64c04e8c45033001fc834cd8039 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
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. 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.

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 Pacquire, Daniel Gustafsson, Tom Lane
Discussion: https://www.postgresql.org/message-id/CAExHW5uF5V=Cjecx3_Z=[email-masked]
---
 doc/src/sgml/regress.sgml                   |  12 ++
 src/bin/pg_upgrade/t/002_pg_upgrade.pl      | 140 +++++++++++++++++++-
 src/test/perl/Makefile                      |   2 +
 src/test/perl/PostgreSQL/Test/AdjustDump.pm | 134 +++++++++++++++++++
 src/test/perl/meson.build                   |   1 +
 5 files changed, 287 insertions(+), 2 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 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
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 68516fa486a..0d636529d74 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.
@@ -517,4 +533,124 @@ my $dump2_filtered = filter_dump(0, $oldnode->pg_version, $dump2_file);
 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');
+
+	# 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, 'regression', 'src_dump', 1);
+
+	# Setup destination database cluster
+	$dst_node->init(%node_params);
+	$dst_node->start;
+
+	for my $format ('plain', 'tar', 'directory', 'custom')
+	{
+		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.
+		$src_node->command_ok(
+			[
+				'pg_dump', "-F$format", '--no-sync',
+				'-d', $src_node->connstr('regression'),
+				'-f', $dump_file
+			],
+			"pg_dump on source instance in $format format");
+
+		# Create a new database for restoring dump from every format so that it
+		# is available for debugging in case the test fails.
+		$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
+			];
+		}
+		$dst_node->command_ok(@restore_command,
+			"restored dump taken in $format format on destination instance");
+
+		my $dst_dump =
+		  get_dump_for_comparison($dst_node, $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 `db` from the given `node` 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 ($node, $db, $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.
+	$node->command_ok(
+		[
+			'pg_dump', '-s', '--no-sync', '-d',
+			$node->connstr($db), '-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..e3e152b88fa
--- /dev/null
+++ b/src/test/perl/PostgreSQL/Test/AdjustDump.pm
@@ -0,0 +1,134 @@
+
+# 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 generated_stored_tests.gtestxx_4 adjustments');
+
+		$saved_dump = $dump;
+		$dump =~ s/(^CREATE\sTABLE\sgenerated_virtual_tests\.gtestxx_4\s\()
+				   (\n\s+b\sinteger),
+				   (\n\s+a\sinteger\sNOT\sNULL)/$1$3,$2/mgx;
+		ok($saved_dump ne $dump,
+			'applied generated_virtual_tests.gtestxx_4 adjustments');
+
+		$saved_dump = $dump;
+		$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 public.test_type_diff2_c1 adjustments');
+
+		$saved_dump = $dump;
+		$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 public.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')

base-commit: 6998db59c2959c4f280a9088054e6dbf7178efe0
-- 
2.34.1

From 6976ebad1e4fa717cd8fa2f70fbe73cd476e7caf Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Tue, 11 Feb 2025 16:31:10 +0530
Subject: [PATCH 2/2] Filter COPY statements with differing column order

---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl      | 10 +---
 src/test/perl/PostgreSQL/Test/AdjustDump.pm | 59 +++++++++++++++------
 2 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 0d636529d74..b3e1573ec34 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -569,9 +569,6 @@ sub test_regression_dump_restore
 		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.
 		$src_node->command_ok(
 			[
 				'pg_dump', "-F$format", '--no-sync',
@@ -633,13 +630,10 @@ sub get_dump_for_comparison
 	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.
 	$node->command_ok(
 		[
-			'pg_dump', '-s', '--no-sync', '-d',
-			$node->connstr($db), '-f', $dumpfile
+			'pg_dump', '--no-sync', '-d', $node->connstr($db), '-f',
+			$dumpfile
 		],
 		'dump for comparison succeeded');
 
diff --git a/src/test/perl/PostgreSQL/Test/AdjustDump.pm b/src/test/perl/PostgreSQL/Test/AdjustDump.pm
index e3e152b88fa..e00a00d1b2c 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustDump.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustDump.pm
@@ -44,22 +44,36 @@ our @EXPORT = qw(
 
 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
+outputs of both the dumps differ in the following cases. This routine adjusts
+the given dump so that dump outputs from the original and restored database,
+respectively, match.
+
+Case 1: Some regression tests purposefully create child tables in such a way
+that the order of their inherited columns differ from column orders of their
+respective parents. In the restored database, however, the order of their
+inherited columns 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.
+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.
+
+Case 2: When dumping COPY statements the columns are ordered by their attribute
+number by fmtCopyColumnList(). If a column is added to a parent table after a
+child has inherited the parent and the child has its own columns, the attribute
+number of the column changes after restoring the child table. This is because
+when executing the dumped C<CREATE TABLE... INHERITS> statement all the parent
+attributes are created before any child attributes. Thus the order of columns in
+COPY statements dumped from the original and the restored databases,
+respectively, differs. Such tables in regression tests are listed below. It is
+hard to adjust the column order in the COPY statement along with the data. Hence
+we just remove such COPY statements from the dump output.
+
+Additionally the routine adjusts blank and new lines to avoid noise.
 
 Arguments:
 
@@ -84,8 +98,6 @@ sub adjust_regress_dumpfile
 
 	# 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)
@@ -122,6 +134,21 @@ sub adjust_regress_dumpfile
 			'applied public.test_type_diff2_c2 adjustments');
 	}
 
+	# Remove COPY statements with differing column order
+	for my $table (
+		'public\.b_star', 'public\.c_star',
+		'public\.cc2', 'public\.d_star',
+		'public\.e_star', 'public\.f_star',
+		'public\.renamecolumnanother', 'public\.renamecolumnchild',
+		'public\.test_type_diff2_c1', 'public\.test_type_diff2_c2',
+		'public\.test_type_diff_c')
+	{
+		$dump =~ s/^COPY\s$table\s\(.+?^\\\.$//sm;
+	}
+
+	# Suppress blank lines, as some places in pg_dump emit more or fewer.
+	$dump =~ s/\n\n+/\n/g;
+
 	return $dump;
 }
 
-- 
2.34.1

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 25 Feb 2025 11:59:24 +0530

> Thanks. PFA rebased patches.

PFA rebased patches.

After rebasing I found another bug and reported it at [1].

For the time being I have added --no-statistics to the pg_dump command
when taking a dump for comparison.

[1] https://www.postgresql.org/message-id/CAExHW5vf9D+8-a5_BEX3y=2y_xY9hiCxV1=[email-masked]

-- 
Best Wishes,
Ashutosh Bapat
From 9c45faedda92901bf7638c4fee42b397b802be96 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 27 Jun 2024 10:03:53 +0530
Subject: [PATCH 1/3] Test pg_dump/restore of regression objects

Note For the reviewers:

Author: Ashutosh Bapat

diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 0e5e8e8f309..237b974b3ab 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -357,6 +357,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>
 
index 45ea94c84bb..25de01615f6 100644

 
 # Initialize a new node for the upgrade.
@@ -524,4 +540,124 @@ my $dump2_filtered = filter_dump(0, $oldnode->pg_version, $dump2_file);
 compare_files($dump1_filtered, $dump2_filtered,
 	'old and new dumps match after pg_upgrade');

 uninstall:
 
 endif

base-commit: 5b8f2ccc0a93375acb64a457817e61f400404a1f
-- 
2.34.1

From 996e175a17ff406373560134bcc5c657bc92a643 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Tue, 25 Feb 2025 11:42:51 +0530
Subject: [PATCH 3/3] Do not dump statistics in the file dumped for comparison

As reported at [1], the dumped and restored statistics may differ if there's a
primary key on the table. Hence do not dump the statistics to avoid differences
in the dump output from the original and restored database.

[1] https://www.postgresql.org/message-id/CAExHW5vf9D+8-a5_BEX3y=2y_xY9hiCxV1=[email-masked]

Ashutosh Bapat
---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 2cc571219ce..f3892f7150d 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -639,7 +639,7 @@ sub get_dump_for_comparison
 
 	$node->command_ok(
 		[
-			'pg_dump', '--no-sync', '-d', $node->connstr($db), '-f',
+			'pg_dump', '--no-sync', '--no-statistics', '-d', $node->connstr($db), '-f',
 			$dumpfile
 		],
 		'dump for comparison succeeded');
-- 
2.34.1

From d4372813f92ead1a6ebb57c42acc6439c8162427 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Tue, 11 Feb 2025 16:31:10 +0530
Subject: [PATCH 2/3] Filter COPY statements with differing column order

---

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 25de01615f6..2cc571219ce 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -576,9 +576,6 @@ sub test_regression_dump_restore
 		my $dump_file = "$tempdir/regression_dump.$format";
 		my $restored_db = 'regression_' . $format;
 
@@ -640,13 +637,10 @@ sub get_dump_for_comparison

 
 
 Arguments:

 	# use Unix newlines
 	$dump =~ s/\r\n/\n/g;

 
-- 
2.34.1

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 11 Mar 2025 16:14:45 +0530

> After rebasing I found another bug and reported it at [1].

This bug has been fixed. But now that it's fixed, it's easy to see
another bug related to materialized view statistics. I have reported
it at [2]. That's the fourth bug identified by this test.

> when taking a dump for comparison.

I have not taken out this option because of materialized view bug.

> [1] https://www.postgresql.org/message-id/CAExHW5vf9D+8-a5_BEX3y=2y_xY9hiCxV1=[email-masked]

[2] https://www.postgresql.org/message-id/[email-masked]

--
Best Wishes,
Ashutosh Bapat
From a140d50245249894a49c39a908163e9bac2fe4bb Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Tue, 11 Feb 2025 16:31:10 +0530
Subject: [PATCH 2/3] Filter COPY statements with differing column order

---

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index c6b99125d9e..e6d8ac9a757 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -581,9 +581,6 @@ sub test_regression_dump_restore
 		my $dump_file = "$tempdir/regression_dump.$format";
 		my $restored_db = 'regression_' . $format;
 
@@ -645,13 +642,10 @@ sub get_dump_for_comparison

 
 
 Arguments:

 	# use Unix newlines
 	$dump =~ s/\r\n/\n/g;

 
-- 
2.34.1

From 25c4c7e4ee754dd989d3fd8f015c7355fd9992d6 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 27 Jun 2024 10:03:53 +0530
Subject: [PATCH 1/3] Test pg_dump/restore of regression objects

Note For the reviewers:

Author: Ashutosh Bapat
Reviewed by: Michael Pacquire, Daniel Gustafsson, Tom Lane
Discussion: https://www.postgresql.org/message-id/CAExHW5uF5V=Cjecx3_Z=[email-masked]
---
 doc/src/sgml/regress.sgml                   |  12 ++
 src/bin/pg_upgrade/t/002_pg_upgrade.pl      | 142 +++++++++++++++++++-
 src/test/perl/Makefile                      |   2 +
 src/test/perl/PostgreSQL/Test/AdjustDump.pm | 134 ++++++++++++++++++
 src/test/perl/meson.build                   |   1 +
 5 files changed, 289 insertions(+), 2 deletions(-)
 create mode 100644 src/test/perl/PostgreSQL/Test/AdjustDump.pm

index c00cf68d660..c6b99125d9e 100644

 
 # Initialize a new node for the upgrade.
@@ -527,4 +543,126 @@ my $dump2_filtered = filter_dump(0, $oldnode->pg_version, $dump2_file);
 compare_files($dump1_filtered, $dump2_filtered,
 	'old and new dumps match after pg_upgrade');
 
+	# Stabilize stats for comparison.
+	$dst_node->append_conf('postgresql.conf', 'autovacuum = off');
 
 uninstall:
 
 endif

base-commit: dabccf45139a8c7c3c2e7683a943c31077e55a78
-- 
2.34.1

From 4258ed1bcad537418c4c3f4ba0e3712ec515e09e Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Tue, 25 Feb 2025 11:42:51 +0530
Subject: [PATCH 3/3] Do not dump statistics in the file dumped for comparison

Ashutosh Bapat
---

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index e6d8ac9a757..8924cf8344a 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -644,7 +644,7 @@ sub get_dump_for_comparison
 
 	$node->command_ok(
 		[

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 12 Mar 2025 13:05:45 +0100

Hello

When running these tests, I encounter this strange diff in the dumps,
which seems to be that the locale for type money does not match.  I
imagine the problem is that the locale is not set correctly when
initdb'ing one of them?  Grepping the regress_log for initdb, I see
this:

$ grep -B1 'Running: initdb' tmp_check/log/regress_log_002_pg_upgrade 
[13:00:57.580](0.003s) # initializing database system by running initdb
# Running: initdb -D /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/t_002_pg_upgrade_old_node_data/pgdata -A trust -N --wal-segsize 1 --allow-group-access --encoding UTF-8 --lc-collate C --lc-ctype C --locale-provider builtin --builtin-locale C.UTF-8 -k
--
[13:01:12.879](0.044s) # initializing database system by running initdb
# Running: initdb -D /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/t_002_pg_upgrade_dst_node_data/pgdata -A trust -N --wal-segsize 1 --allow-group-access --encoding UTF-8 --lc-collate C --lc-ctype C --locale-provider builtin --builtin-locale C.UTF-8 -k
--
[13:01:28.000](0.033s) # initializing database system by running initdb
# Running: initdb -D /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/t_002_pg_upgrade_new_node_data/pgdata -A trust -N --wal-segsize 1 --allow-group-access --encoding SQL_ASCII --locale-provider libc

[12:50:31.838](0.102s) not ok 15 - dump outputs from original and restored regression database (using plain format) match
[12:50:31.839](0.000s) 
[12:50:31.839](0.000s) #   Failed test 'dump outputs from original and restored regression database (using plain format) match'
#   at /pgsql/source/master/src/test/perl/PostgreSQL/Test/Utils.pm line 797.
[12:50:31.839](0.000s) #          got: '1'
#     expected: '0'
=== diff of /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/tmp_test_vVew/src_dump.sql_adjusted and /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/tmp_test_vVew/dest_dump.plain.sql_adjusted
=== stdout ===
--- /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/tmp_test_vVew/src_dump.sql_adjusted	2025-03-12 12:50:27.674918597 +0100
+++ /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/tmp_test_vVew/dest_dump.plain.sql_adjusted	2025-03-12 12:50:31.778840338 +0100
@@ -208972,7 +208972,7 @@
 -- Data for Name: money_data; Type: TABLE DATA; Schema: public; Owner: alvherre
 --
 COPY public.money_data (m) FROM stdin;
-$123.46
+$ 12.346,00
 \.
 --
 -- Data for Name: mvtest_t; Type: TABLE DATA; Schema: public; Owner: alvherre
@@ -376231,7 +376231,7 @@
 -- Data for Name: tab_core_types; Type: TABLE DATA; Schema: public; Owner: alvherre
 --
 COPY public.tab_core_types (point, line, lseg, box, openedpath, closedpath, polygon, circle, date, "time", "timestamp", timetz, timestamptz, "interval", "json", jsonb, jsonpath, inet, cidr, macaddr8, macaddr, int2, int4, int8, float4, float8, pi, "char", bpchar, "varchar", name, text, bool, bytea, "bit", varbit, money, refcursor, int2vector, oidvector, aclitem, tsvector, tsquery, uuid, xid8, regclass, type, regrole, oid, tid, xid, cid, txid_snapshot, pg_snapshot, pg_lsn, cardinal_number, character_data, sql_identifier, time_stamp, yes_or_no, int4range, int4multirange, int8range, int8multirange, numrange, nummultirange, daterange, datemultirange, tsrange, tsmultirange, tstzrange, tstzmultirange) FROM stdin;
-(11,12)	{1,-1,0}	[(11,11),(12,12)]	(13,13),(11,11)	((11,12),(13,13),(14,14))	[(11,12),(13,13),(14,14)]	((11,12),(13,13),(14,14))	<(1,1),1>	2025-03-12	04:50:14.125899	2025-03-12 04:50:14.125899	04:50:14.125899-07	2025-03-12 12:50:14.125899+01	00:00:12	{"reason":"because"}	{"when": "now"}	$."a"[*]?(@ > 2)	127.0.0.1	127.0.0.0/8	00:01:03:ff:fe:86:1c:ba	00:01:03:86:1c:ba	2	4	8	4	8	3.14159265358979	f	c	abc	name	txt	t	\\xdeadbeef	1	10001	$12.34	abc	1 2	1 2	alvherre=UC/alvherre	'a' 'and' 'ate' 'cat' 'fat' 'mat' 'on' 'rat' 'sat'	'fat' & 'rat'	a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11	11	pg_class	regtype	pg_monitor	1259	(1,1)	2	3	10:20:10,14,15	10:20:10,14,15	16/B374D848	1	l	n	2025-03-12 12:50:14.13+01	YES	empty	{}	empty	{}	(3,4)	{(3,4)}	[2020-01-03,2021-02-03)	{[2020-01-03,2021-02-03)}	("2020-01-02 03:04:05","2021-02-03 06:07:08")	{("2020-01-02 03:04:05","2021-02-03 06:07:08")}	("2020-01-02 12:04:05+01","2021-02-03 15:07:08+01")	{("2020-01-02 12:04:05+01","2021-02-03 15:07:08+01")}
+(11,12)	{1,-1,0}	[(11,11),(12,12)]	(13,13),(11,11)	((11,12),(13,13),(14,14))	[(11,12),(13,13),(14,14)]	((11,12),(13,13),(14,14))	<(1,1),1>	2025-03-12	04:50:14.125899	2025-03-12 04:50:14.125899	04:50:14.125899-07	2025-03-12 12:50:14.125899+01	00:00:12	{"reason":"because"}	{"when": "now"}	$."a"[*]?(@ > 2)	127.0.0.1	127.0.0.0/8	00:01:03:ff:fe:86:1c:ba	00:01:03:86:1c:ba	2	4	8	4	8	3.14159265358979	f	c	abc	name	txt	t	\\xdeadbeef	1	10001	$ 1.234,00	abc	1 2	1 2	alvherre=UC/alvherre	'a' 'and' 'ate' 'cat' 'fat' 'mat' 'on' 'rat' 'sat'	'fat' & 'rat'	a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11	11	pg_class	regtype	pg_monitor	1259	(1,1)	2	3	10:20:10,14,15	10:20:10,14,15	16/B374D848	1	l	n	2025-03-12 12:50:14.13+01	YES	empty	{}	empty	{}	(3,4)	{(3,4)}	[2020-01-03,2021-02-03)	{[2020-01-03,2021-02-03)}	("2020-01-02 03:04:05","2021-02-03 06:07:08")	{("2020-01-02 03:04:05","2021-02-03 06:07:08")}	("2020-01-02 12:04:05+01","2021-02-03 15:07:08+01")	{("2020-01-02 12:04:05+01","2021-02-03 15:07:08+01")}
 \.
 --
 -- Data for Name: tableam_parted_a_heap2; Type: TABLE DATA; Schema: public; Owner: alvherre=== stderr ===
=== EOF ===

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"¿Qué importan los años?  Lo que realmente importa es comprobar que
a fin de cuentas la mejor edad de la vida es estar vivo"  (Mafalda)

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 12 Mar 2025 21:28:35 +0530

> # Running: initdb -D /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/t_002_pg_upgrade_old_node_data/pgdata -A trust -N --wal-segsize 1 --allow-group-access --encoding UTF-8 --lc…

The original node and the node where dump is restored have the same
initdb commands. It's the upgraded node which has different initdb
command. But that's how the test is written originally.

> +++ /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/tmp_test_vVew/dest_dump.plain.sql_adjusted      2025-03-12 12:50:31.778840338 +0100

However these differences are coming from original and restored
database which are using the same initdb options.

Does the test pass for you if you don't apply my patches?

Over at [1], I had seen a locale related failure without applying my patches.

[1] https://www.postgresql.org/message-id/CAExHW5s+XNiP8aPGw9=[email-masked]

--
Best Wishes,
Ashutosh Bapat

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 12 Mar 2025 17:09:18 +0100

> Does the test pass for you if you don't apply my patches?

Yes.  It also passes if I keep PG_TEST_EXTRA empty.

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 13 Mar 2025 12:22:48 +0530

Hi Alvaro,

> Yes.  It also passes if I keep PG_TEST_EXTRA empty.

I am not able to reproduce this problem locally.

The test uses

In my case the money is printed $<digits before decimal>.<digits after
decimal> format in both the dumps. But in your case the money printed
from restored database has a space between $ and amount and the amount
also has decimal and comma in odd places - I can't figure out what
that means or what lc_monetary value would print something like that.
Can you please help me with
1. can you please run the test again and share the dump outputs. They
will be located in a temporary directory with names
src_dump.sql_adjusted and dest_dump.<format>.sql_adjusted.
2. Are you seeing this diff only with plain format or other formats as well?

Sorry for the trouble.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 13 Mar 2025 09:42:31 +0100

Hello

> src_dump.sql_adjusted and dest_dump.<format>.sql_adjusted.

Ah, I see the problem :-)  The first initdb does this:

	# Running: initdb -D /home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/t_002_pg_upgrade_old_node_data/pgdata -A trust -N --wal-segsize 1 --allow-group-access --encoding UTF-8 --lc-collate C --lc-ctype C --locale-provider builtin --builtin-locale C.UTF-8 -k
	The files belonging to this database system will be owned by user "alvherre".
	This user must also own the server process.

	The database cluster will be initialized with this locale configuration:
	  locale provider:   builtin
	  default collation: C.UTF-8
	  LC_COLLATE:  C
	  LC_CTYPE:    C
	  LC_MESSAGES: C
	  LC_MONETARY: es_CL.UTF-8
	  LC_NUMERIC:  es_CL.UTF-8
	  LC_TIME:     es_CL.UTF-8
	The default text search configuration will be set to "english".

	Data page checksums are enabled.

which for some reason used my environment setting for LC_MONETARY.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"But static content is just dynamic content that isn't moving!"
                http://smylers.hates-software.com/2007/08/15/fe244d0c.html

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 13 Mar 2025 18:09:25 +0530

> which for some reason used my environment setting for LC_MONETARY.

Thanks. This is super helpful. I am able to reproduce the problem
$ unset LC_MONETARY
$ export PG_TEST_EXTRA=regress_dump_test
$ meson test --suite setup && meson test pg_upgrade/002_pg_upgrade
... snip ...
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade        OK
       72.38s   44 subtests passed

Ok:                 1
Expected Fail:      0
Fail:               0
Unexpected Pass:    0
Skipped:            0
Timeout:            0

Full log written to
/home/ashutosh/work/units/pg_dump_test/build/dev/meson-logs/testlog.txt
$ export LC_MONETARY="es_CL.UTF-8"
$ meson test --suite setup && meson test pg_upgrade/002_pg_upgrade
... snip ...
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade        ERROR
       69.18s   exit status 4
>>> with_icu=no LD_LIBRARY_PATH=/home/ashutosh/work/units/pg_dump_test/build/dev/tmp_install//home/ashutosh/work/units/pg_dump_test/build/dev/lib/x86_64-linux-gnu REGRESS_SHLIB=/home/ashutosh/work/units/pg_dump_test/build/dev/src/test/regress/regress.so PATH=/home/ashutosh/work/units/pg_dump_test/build/dev/tmp_install//home/ashutosh/work/units/pg_dump_test/build/dev/bin:/home/ashutosh/work/units/pg_dump_test/build/dev/src/bin/pg_upgrade:/home/ashutosh/work/units/pg_dump_test/build/dev/src/bin/pg_upgrade/test:/home/ashutosh/work/units/pg_dump_test/build/dev/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/snap/bin MALLOC_PERTURB_=30 share_contrib_dir=/home/ashutosh/work/units/pg_dump_test/build/dev/tmp_install//home/ashutosh/work/units/pg_dump_test/build/dev/share/postgresql/contrib PG_REGRESS=/home/ashutosh/work/units/pg_dump_test/build/dev/src/test/regress/pg_regress top_builddir=/home/ashutosh/work/units/pg_dump_test/build/dev INITDB_TEMPLATE=/home/ashutosh/work/units/pg_dump_test/build/dev/tmp_install/initdb-template /usr/bin/python3 /home/ashutosh/work/units/pg_dump_test/build/dev/../../coderoot/pg/src/tools/testwrap --basedir /home/ashutosh/work/units/pg_dump_test/build/dev --srcdir /home/ashutosh/work/units/pg_dump_test/coderoot/pg/src/bin/pg_upgrade --pg-test-extra '' --testgroup pg_upgrade --testname 002_pg_upgrade -- /usr/bin/perl -I /home/ashutosh/work/units/pg_dump_test/coderoot/pg/src/test/perl -I /home/ashutosh/work/units/pg_dump_test/coderoot/pg/src/bin/pg_upgrade /home/ashutosh/work/units/pg_dump_test/coderoot/pg/src/bin/pg_upgrade/t/002_pg_upgrade.pl

Ok:                 0
Expected Fail:      0
Fail:               1
Unexpected Pass:    0
Skipped:            0
Timeout:            0

I see what's happening.  If I set LC_MONETARY environment explicitly,
that's taken by initdb
$ export LC_MONETARY="es_CL.UTF-8";rm -rf $DataDir; $BinDir/initdb -D
$DataDir -A trust -N --wal-segsize 1 --allow-group-access --encoding
UTF-8 --lc-collate C --lc-ctype C --locale-provider builtin
--builtin-locale C.UTF-8 -k
The files belonging to this database system will be owned by user "ashutosh".
This user must also own the server process.

  LC_MESSAGES: en_US.UTF-8
  LC_NUMERIC:  en_US.UTF-8
  LC_TIME:     en_US.UTF-8

If I don't set it explicitly, it's taken from default settings
$ unset LC_MONETARY;rm -rf $DataDir; $BinDir/initdb -D $DataDir -A
trust -N --wal-segsize 1 --allow-group-access --encoding UTF-8
--lc-collate C --lc-ctype C --locale-provider builtin --builtin-locale
C.UTF-8 -k
The files belonging to this database system will be owned by user "ashutosh".
This user must also own the server process.

  LC_MESSAGES: en_US.UTF-8
  LC_MONETARY: en_US.UTF-8
  LC_NUMERIC:  en_US.UTF-8
  LC_TIME:     en_US.UTF-8

In your case probably your default setting is es_CL.UTF-8 or have set
LC_MONETARY explicitly in your environment.

I think the fix is to explicitly pass --lc-monetary to the old cluster
and the restored cluster. 003 patch in the attached patch set does
that. Please check if it fixes the issue for you.

Additionally we should check that it gets copied to the new cluster as
well. But I haven't figured out how to get those settings yet. This
treatment is similar to how --lc-collate and --lc-ctype are treated. I
am wondering whether we should explicitly pass --lc-messages,
--lc-time and --lc-numeric as well.

2d819a08a1cbc11364e36f816b02e33e8dcc030b introduced buildin locale
provider and added overrides to LC_COLLATE and LC_TYPE. But it did not
override other LC_, which I think it should have. In pure upgrade
test, the upgraded node inherits the locale settings of the original
cluster, so this wasn't apparent. But with pg_dump testing, the
original and restored databases are independent. Hence I think we have
to override all LC_* settings by explicitly mentioning --lc-* options
to initdb. Please let me know what you think about this?

--
Best Wishes,
Ashutosh Bapat

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 13 Mar 2025 18:10:12 +0530

Here are patches missing in the previous email.

-- 
Best Wishes,
Ashutosh Bapat
From ec6c178ba0a1a20dc989fee94fdc8d53d531e2e4 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 27 Jun 2024 10:03:53 +0530
Subject: [PATCH 1/3] Test pg_dump/restore of regression objects

Note For the reviewers:

Author: Ashutosh Bapat
Reviewed by: Michael Pacquire, Daniel Gustafsson, Tom Lane, Alvaro Herrera
Discussion: https://www.postgresql.org/message-id/CAExHW5uF5V=Cjecx3_Z=[email-masked]
---
 doc/src/sgml/regress.sgml                   |  12 ++
 src/bin/pg_upgrade/t/002_pg_upgrade.pl      | 141 ++++++++++++++++-
 src/test/perl/Makefile                      |   2 +
 src/test/perl/PostgreSQL/Test/AdjustDump.pm | 167 ++++++++++++++++++++
 src/test/perl/meson.build                   |   1 +
 5 files changed, 321 insertions(+), 2 deletions(-)
 create mode 100644 src/test/perl/PostgreSQL/Test/AdjustDump.pm

index c00cf68d660..bd8313cee6f 100644

 
 # Initialize a new node for the upgrade.
@@ -527,4 +543,125 @@ my $dump2_filtered = filter_dump(0, $oldnode->pg_version, $dump2_file);
 compare_files($dump1_filtered, $dump2_filtered,
 	'old and new dumps match after pg_upgrade');
 
+	# Usually we avoid comparing statistics in our tests since it is flaky by
+	# nature. However, if statistics is dumped and restored it is expected to be
+	# restored as it is i.e. the statistics from the original database and that
+	# from the restored database should match. We turn off autovacuum on the
+	# source and the target database to avoid any statistics update during
+	# restore operation. Hence we do not exclude statistics from dump.
 
 uninstall:
 
 endif
index 00000000000..74b9a60cf34
@@ -0,0 +1,167 @@
+Note: Usually we avoid comparing statistics in our tests since it is flaky by
+nature. However, if statistics is dumped and restored it is expected to be
+restored as it is i.e. the statistics from the original database and that from
+the restored database should match. Hence we do not filter statistics from dump,
+if it's dumped.

base-commit: 3691edfab97187789b8a1cbb9dce4acf0ecd8f5a
-- 
2.34.1

From 2f41b4371c0f0b8a3535537c4e4f9ddd1118d1ce Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 13 Mar 2025 16:17:57 +0530
Subject: [PATCH 3/3] set lc_monetary explicitly at initdb time

---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 65f4c7d4f2b..51ba79c8589 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -134,6 +134,7 @@ my $original_enc_name;
 my $original_provider;
 my $original_datcollate = "C";
 my $original_datctype = "C";
+my $original_datmonetary = "C";
 my $original_datlocale;
 
 if ($oldnode->pg_version >= '17devel')
@@ -163,6 +164,7 @@ my @initdb_params = @custom_opts;
 push @initdb_params, ('--encoding', $original_enc_name);
 push @initdb_params, ('--lc-collate', $original_datcollate);
 push @initdb_params, ('--lc-ctype', $original_datctype);
+push @initdb_params, ('--lc-monetary', $original_datmonetary);
 
 # add --locale-provider, if supported
 my %provider_name = ('b' => 'builtin', 'i' => 'icu', 'c' => 'libc');
-- 
2.34.1

From 75f2b869764d9db3ac5c548636ed5c2be8b47b36 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Tue, 25 Feb 2025 11:42:51 +0530
Subject: [PATCH 2/3] Do not dump statistics in the file dumped for comparison

The dumped and restored statistics of a materialized view may differ as
reported in [1].  Hence do not dump the statistics to avoid differences
in the dump output from the original and restored database.

[1] https://www.postgresql.org/message-id/[email-masked]

Ashutosh Bapat
---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index bd8313cee6f..65f4c7d4f2b 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -641,15 +641,15 @@ sub get_dump_for_comparison
 	my $dumpfile = $tempdir . '/' . $file_prefix . '.sql';
 	my $dump_adjusted = "${dumpfile}_adjusted";
 
-	# Usually we avoid comparing statistics in our tests since it is flaky by
-	# nature. However, if statistics is dumped and restored it is expected to be
-	# restored as it is i.e. the statistics from the original database and that
-	# from the restored database should match. We turn off autovacuum on the
-	# source and the target database to avoid any statistics update during
-	# restore operation. Hence we do not exclude statistics from dump.
+	# If statistics is dumped and restored it is expected to be restored as it
+	# is i.e. the statistics from the original database and that from the
+	# restored database should match. We turn off autovacuum on the source and
+	# the target database to avoid any statistics update during restore
+	# operation. But as of now, there are cases when statistics is not being
+	# restored faithfully. Hence for now do not dump statistics.
 	$node->command_ok(
 		[
-			'pg_dump', '--no-sync', '-d', $node->connstr($db), '-f',
+			'pg_dump', '--no-sync', '--no-statistics', '-d', $node->connstr($db), '-f',
 			$dumpfile
 		],
 		'dump for comparison succeeded');
-- 
2.34.1

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 19 Mar 2025 17:13:12 +0530

> > to initdb. Please let me know what you think about this?

Investigated this further. The problem is that the pg_regress run
creates regression database with specific properties but the restored
database does not have those properties. That led me to a better
solution. Additionally it's local to the new test. Use --create when
dumping and restoring the regression database. This way the database
properties or "configuration variable settings (as pg_dump
documentation calls them) are copied to the restored database as well.
Those properties include LC_MONETARY. Additionally now the test covers
--create option as well.

PFA patches.

-- 
Best Wishes,
Ashutosh Bapat
From 886e241e304a23bb31b5e59f12149741dfff2b14 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Tue, 25 Feb 2025 11:42:51 +0530
Subject: [PATCH 2/2] Do not dump statistics in the file dumped for comparison

Ashutosh Bapat
---

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index d08eea6693f..f931fef2307 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -656,15 +656,15 @@ sub get_dump_for_comparison
 	my $dumpfile = $tempdir . '/' . $file_prefix . '.sql';
 	my $dump_adjusted = "${dumpfile}_adjusted";

From 1723050dadb89f3187fef19c994d8c866ee5a788 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 27 Jun 2024 10:03:53 +0530
Subject: [PATCH 1/2] Test pg_dump/restore of regression objects

Note For the reviewers:
The new test has uncovered many bugs so far in one year.
1. Introduced by 14e87ffa5c54. Fixed in fd41ba93e4630921a72ed5127cd0d552a8f3f8fc.
2. Introduced by 0413a556990ba628a3de8a0b58be020fd9a14ed0. Reverted in 74563f6b90216180fc13649725179fc119dddeb5.
3. Fixed by d611f8b1587b8f30caa7c0da99ae5d28e914d54f
3. Being discussed on hackers at https://www.postgresql.org/message-id/[email-masked]

Author: Ashutosh Bapat
Reviewed by: Michael Pacquire, Daniel Gustafsson, Tom Lane, Alvaro Herrera
Discussion: https://www.postgresql.org/message-id/CAExHW5uF5V=Cjecx3_Z=[email-masked]
---
 doc/src/sgml/regress.sgml                   |  12 ++
 src/bin/pg_upgrade/t/002_pg_upgrade.pl      | 144 ++++++++++++++++-
 src/test/perl/Makefile                      |   2 +
 src/test/perl/PostgreSQL/Test/AdjustDump.pm | 167 ++++++++++++++++++++
 src/test/perl/meson.build                   |   1 +
 5 files changed, 324 insertions(+), 2 deletions(-)
 create mode 100644 src/test/perl/PostgreSQL/Test/AdjustDump.pm

index 00051b85035..d08eea6693f 100644

@@ -262,6 +263,21 @@ else
 
 # Initialize a new node for the upgrade.
@@ -539,4 +555,128 @@ my $dump2_filtered = filter_dump(0, $oldnode->pg_version, $dump2_file);
 compare_files($dump1_filtered, $dump2_filtered,
 	'old and new dumps match after pg_upgrade');
 
+# run on `src_node`, which in turn, is left in running state. A fresh node is
+# created using given `node_params`, which are expected to be the same ones used
+# to create `src_node`, so as to avoid any differences in the databases.
+	# Test all formats one by one.
+		# Use --create in dump and restore commands so that the restored
+		# database has the same configurable variable settings as the original
+		# database and the plain dumps taken for comparsion do not differ
+		# because of locale changes. Additionally this provides test coverage
+		# for --create option.
+				'--create', '-f', $dump_file
+			@restore_command = [ 'psql', '-d', 'postgres', '-f', $dump_file ];
+				'pg_restore', '--create',
+				'-d', 'postgres', $dump_file
+		  get_dump_for_comparison($dst_node, 'regression',
+		# Rename the restored database so that it is available for debugging in
+		# case the test fails.
+		$dst_node->safe_psql('postgres', "ALTER DATABASE regression RENAME TO $restored_db");
 
 uninstall:
 
 endif

base-commit: 190dc27998d5b7b4c36e12bebe62f7176f4b4507
-- 
2.34.1

---

From: vignesh C <[email-masked]>
Date: Thu, 20 Mar 2025 20:36:58 +0530

> Those properties include LC_MONETARY. Additionally now the test covers

Will it help the execution time if we use --jobs in case of pg_dump
and pg_restore wherever supported:
+               $src_node->command_ok(
+                       [
+                               'pg_dump', "-F$format", '--no-sync',
+                               '-d', $src_node->connstr('regression'),
+                               '--create', '-f', $dump_file
+                       ],
+                       "pg_dump on source instance in $format format");
+
+               my @restore_command;
+               if ($format eq 'plain')
+               {
+                       # Restore dump in "plain" format using `psql`.
+                       @restore_command = [ 'psql', '-d', 'postgres',
'-f', $dump_file ];
+               }
+               else
+               {
+                       @restore_command = [
+                               'pg_restore', '--create',
+                               '-d', 'postgres', $dump_file
+                       ];
+               }

Should the copyright be only 2025 in this case:
diff --git a/src/test/perl/PostgreSQL/Test/AdjustDump.pm
b/src/test/perl/PostgreSQL/Test/AdjustDump.pm
new file mode 100644
index 00000000000..74b9a60cf34
--- /dev/null
+++ b/src/test/perl/PostgreSQL/Test/AdjustDump.pm
@@ -0,0 +1,167 @@
+
+# Copyright (c) 2024-2025, PostgreSQL Global Development Group

Regards,
Vignesh

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 20 Mar 2025 17:39:40 +0100

> and pg_restore wherever supported:

As I said in another thread, I think we should enable this test to run
without requiring any PG_TEST_EXTRA, because otherwise the only way to
know about problems is to commit a patch and wait for buildfarm to run
it.  Furthermore, I think running all 4 dump format modes is a waste of
time; there isn't any extra coverage by running this test in additional
formats.

Putting those two thoughts together with yours about running with -j,
I propose that what we should do is make this test use -Fc with no
compression (to avoid wasting CPU on that) and use a lowish -j value for
both pg_dump and pg_restore, probably 2, or 3 at most.  (Not more,
because this is likely to run in parallel with other tests anyway.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No renuncies a nada. No te aferres a nada."

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 21 Mar 2025 17:21:13 +0530

> +                               '-d', 'postgres', $dump_file

Will reply to this separately along with reply to Alvaro's comments.

> +++ b/src/test/perl/PostgreSQL/Test/AdjustDump.pm

The patch was posted in 2024 to this mailing list. So we better
protect the copyright since then. I remember a hackers discussion
where a senior member of the community mentioned that there's not harm
in mentioning longer copyright periods than being stricter about it. I
couldn't find the discussion though.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 21 Mar 2025 13:34:02 +0100

> couldn't find the discussion though.

On the other hand, my impression is that we do update copyright years to
current year, when committing new files of patches that have been around
for long. 

And there's always
https://liferay.dev/blogs/-/blogs/how-and-why-to-properly-write-copyright-statements-in-your-code

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Las cosas son buenas o malas segun las hace nuestra opinión" (Lisias)

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 21 Mar 2025 18:11:00 +0530

> https://liferay.dev/blogs/-/blogs/how-and-why-to-properly-write-copyright-statements-in-your-code

Right. So shouldn't the copyright notice be 2024-2025 and not just
only 2025? - Next year it will be changed to 2024-2026.

-- 
Best Wishes,
Ashutosh Bapat

---

From: vignesh C <[email-masked]>
Date: Fri, 21 Mar 2025 18:15:38 +0530

> time; there isn't any extra coverage by running this test in additional

+1 for running it in only one of the formats.

Regards,
Vignesh

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 21 Mar 2025 18:39:11 +0530

> because this is likely to run in parallel with other tests anyway.)

-Fc and -j are not allowed. -j is only allowed for directory format.

$ pg_dump -Fc -j2
pg_dump: error: parallel backup only supported by the directory format

Using just directory format, on my laptop with dev build (because
that's what most developers will use when running the tests)

$ meson test -C $BuildDir pg_upgrade/002_pg_upgrade | grep 002_pg_upgrade

without dump/restore test
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
33.51s   19 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
34.22s   19 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
34.64s   19 subtests passed

without -j, extra ~9 seconds
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
43.33s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
43.25s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
43.10s   28 subtests passed

with -j2, extra 7.5 seconds
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
42.77s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.67s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.88s   28 subtests passed

with -j3, extra 7 seconds
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
40.77s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.05s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.28s   28 subtests passed

Between -j2 and -j3 there's not much difference so we could use -j2.
But it still takes 7.5 extra seconds which almost 20% extra time. Do
you think that will be acceptable? I saw somewhere Andres mentioning
that he runs this test quite frequently. Please note that I would very
much like this test to be run by default, but Tom Lane has expressed a
concern about adding even that much time [1] to run the test and
mentioned that he would like the test to be opt-in.

When I started writing the test one year before, people raised
concerns about how useful the test would be. Within a year it has
shown 4 bugs. I have similar feeling about the formats - it's doubtful
now but will prove useful soon especially with the work happening on
dump formats in nearby threads. If we run the test by default, we
could run directory with -j by default and leave other formats as
opt-in OR just forget those formats for now. But If we are going to
make it opt-in, testing all formats gives the extra coverage.

About the format coverage, consensus so far is me and Daniel are for
including all formats when running test as opt-in. Alvaro and Vignesh
are for just one format. We need a tie-breaker or someone amongst us
needs to change their vote :D.

--
Best Wishes,
Ashutosh Bapat

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 21 Mar 2025 15:39:17 +0100

I passed PROVE_FLAGS="--timer -v" to get the timings and run under
--format=directory.

Without new test:
ok    23400 ms ( 0.00 usr  0.00 sys +  2.84 cusr  1.53 csys =  4.37 CPU)
ok    23409 ms ( 0.00 usr  0.01 sys +  2.81 cusr  1.53 csys =  4.35 CPU)

With new test, under --format=directory:
-j2 (parallel, default gzip compression)
ok    27517 ms ( 0.00 usr  0.00 sys +  3.92 cusr  1.86 csys =  5.78 CPU)
ok    27772 ms ( 0.01 usr  0.00 sys +  3.96 cusr  1.86 csys =  5.83 CPU)
ok    27654 ms ( 0.00 usr  0.00 sys +  3.81 cusr  1.94 csys =  5.75 CPU)
ok    27663 ms ( 0.00 usr  0.00 sys +  4.11 cusr  1.71 csys =  5.82 CPU)

-j2 --compress=0
ok    27710 ms ( 0.00 usr  0.00 sys +  3.79 cusr  1.86 csys =  5.65 CPU)
ok    27567 ms ( 0.01 usr  0.00 sys +  3.67 cusr  1.96 csys =  5.64 CPU)
ok    27582 ms ( 0.00 usr  0.00 sys +  3.60 cusr  1.90 csys =  5.50 CPU)
ok    27519 ms ( 0.01 usr  0.00 sys +  3.71 cusr  1.80 csys =  5.52 CPU)

-j2 --compress=zstd
ok    27240 ms ( 0.01 usr  0.00 sys +  3.65 cusr  2.10 csys =  5.76 CPU)
ok    27301 ms ( 0.01 usr  0.00 sys +  3.77 cusr  1.97 csys =  5.75 CPU)

-j2 --compress=zstd:1
ok    27695 ms ( 0.01 usr  0.00 sys +  3.66 cusr  2.05 csys =  5.72 CPU)
ok    27671 ms ( 0.01 usr  0.00 sys +  3.76 cusr  1.95 csys =  5.72 CPU)

--compress=zstd:1 (no parallelism)
ok    28417 ms ( 0.01 usr  0.00 sys +  3.90 cusr  1.75 csys =  5.66 CPU)
ok    28388 ms ( 0.00 usr  0.00 sys +  3.74 cusr  1.81 csys =  5.55 CPU)

--compress=zstd (no parallelism)
ok    28310 ms ( 0.00 usr  0.01 sys +  3.81 cusr  1.83 csys =  5.65 CPU)
ok    28277 ms ( 0.01 usr  0.00 sys +  3.71 cusr  1.87 csys =  5.59 CPU)

So apparently, zstd if available is a bit better than gzip and
parallelism is better than no.  But the differences are small -- half a
second or so.  The total increase in runtime in the best case is about
four seconds.  In all cases I used the same parallelism in pg_restore
than pg_dump; not sure if that could cause a difference.

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 21 Mar 2025 21:33:18 +0530

> than pg_dump; not sure if that could cause a difference.

I used the same parallelism in pg_restore and pg_dump too. And your
numbers seem to be similar to mine; slightly less than 20% slowdown.
But is that slowdown acceptable? From the earlier discussions, it
seems the answer is No. Haven't heard otherwise.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 21 Mar 2025 19:07:56 +0100

> seems the answer is No. Haven't heard otherwise.

I don't think we need to see slowdown this in relative terms, the way we
would discuss a change in the executor.  This is not a change that
would affect user-level stuff in any way.  We need to see it in absolute
terms: in machines similar to mine, the pg_upgrade test would go from
taking 23s to taking 27s.  This is 4s slower, but this isn't an increase
in total test runtime, because decently run test suites run multiple
tests in parallel.  This is the same that Peter said in [1].  The total
test runtime change might not be *that* large.  I'll take a few numbers
and report back.

[1] https://postgr.es/m/[email-masked]

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I love the Postgres community. It's all about doing things _properly_. :-)"
(David Garamond)

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 24 Mar 2025 15:24:30 +0530

> test runtime change might not be *that* large.  I'll take a few numbers

 Using -j2 in pg_dump and -j3 in pg_restore does not improve timing
much on my laptop. I have used -j2 for both pg_dump and restore
instead of -j3 so as to avoid using more cores when tests are run in
parallel.

Further to reduce run time, I tried -1/--single-transaction but that's
not allowed with --create. I also tried --transaction-size=1000 but
that doesn't affect the run time of the test. Next I thought of using
standard output and input instead of files but it doesn't help since
1. directory format cannot use those and it's the only format allowing
parallelism, 2. that's slower than using files with --no-sync. Didn't
find any other way which can help us reduce the test time.

Please note that the dumps taken for comparison cannot use -j since
they are required to be in "plain" format so that text manipulation
comparison works on them.

One concern I have with directory format is the dumped database is not
readable. This might make investigating a but identified the test a
bit more complex. But I guess, in such a case investigator can either
use the dumps taken for comparison or change the code to use plain
format for investigation. So it's a price we pay for making test
faster.

Here's next patchset:
0001 - it's the same 0001 patch as previous one, includes the test
with all formats and also the PG_TEST_EXTRA option

0002 - removes PG_TEST_EXTRA and also tests only one format
--directory with -j2 with default compression. It should be merged
into 0001 before committing. This is a separate patch for now in case
we decide to go back to 0001.

0003 - same as 0002 in the previous patch set. It excludes statistics
from comparison, otherwise the test will fail because of bug reported
at [1]. Ideally we shouldn't commit this patch so as to test
statistics dump and restore, but in case we need the test to pass till
the bug is fixed, we should merge this patch to 0001 before
committing.

--
Best Wishes,
Ashutosh Bapat
From fcfd0d25ecd374d55970817b4d3ea2aecdd58251 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 27 Jun 2024 10:03:53 +0530
Subject: [PATCH 1/3] Test pg_dump/restore of regression objects

Note For the reviewers:

Author: Ashutosh Bapat

 

 
 
 uninstall:
 
 endif

base-commit: 73eba5004a06a744b6b8570e42432b9e9f75997b
-- 
2.34.1

From 435c659489b34a803675abb65144fab6f0550432 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Tue, 25 Feb 2025 11:42:51 +0530
Subject: [PATCH 3/3] Do not dump statistics in the file dumped for comparison

Ashutosh Bapat
---

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index cbd9831bf9e..abe93a49258 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -630,15 +630,15 @@ sub get_dump_for_comparison
 	my $dumpfile = $tempdir . '/' . $file_prefix . '.sql';
 	my $dump_adjusted = "${dumpfile}_adjusted";

From f26f88364a196dc9589ca451cb54f5e514e3422e Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Mon, 24 Mar 2025 11:21:12 +0530
Subject: [PATCH 2/3] Use only one format and make the test run default

According to Alvaro (and I agree with him), the test should be run by
default. Otherwise we get to know about a bug only after buildfarm
animal where it's enabled reports a failure. Further testing only one
format may suffice; since all the formats have shown the same bugs till
now.

If we use --directory format we can use -j which reduces the time taken
by dump/restore test by about 12%.

This patch removes PG_TEST_EXTRA option as well as runs the test only in
directory format with parallelism enabled.

Note for committer: If we decide to accept this change, it should be
merged with the previous commit.
---
 doc/src/sgml/regress.sgml              | 12 ----
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 76 +++++++++-----------------
 2 files changed, 25 insertions(+), 63 deletions(-)

diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 237b974b3ab..0e5e8e8f309 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -357,18 +357,6 @@ 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
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index d08eea6693f..cbd9831bf9e 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -268,16 +268,9 @@ else
 	# 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);
-	}
+	test_regression_dump_restore($oldnode, %node_params);
 }
 
 # Initialize a new node for the upgrade.
@@ -590,53 +583,34 @@ sub test_regression_dump_restore
 	$dst_node->append_conf('postgresql.conf', 'autovacuum = off');
 	$dst_node->start;
 
-	# Test all formats one by one.
-	for my $format ('plain', 'tar', 'directory', 'custom')
-	{
-		my $dump_file = "$tempdir/regression_dump.$format";
-		my $restored_db = 'regression_' . $format;
-
-		# Use --create in dump and restore commands so that the restored
-		# database has the same configurable variable settings as the original
-		# database and the plain dumps taken for comparsion do not differ
-		# because of locale changes. Additionally this provides test coverage
-		# for --create option.
-		$src_node->command_ok(
-			[
-				'pg_dump', "-F$format", '--no-sync',
-				'-d', $src_node->connstr('regression'),
-				'--create', '-f', $dump_file
-			],
-			"pg_dump on source instance in $format format");
+	my $dump_file = "$tempdir/regression.dump";
 
-		my @restore_command;
-		if ($format eq 'plain')
-		{
-			# Restore dump in "plain" format using `psql`.
-			@restore_command = [ 'psql', '-d', 'postgres', '-f', $dump_file ];
-		}
-		else
-		{
-			@restore_command = [
-				'pg_restore', '--create',
-				'-d', 'postgres', $dump_file
-			];
-		}
-		$dst_node->command_ok(@restore_command,
-			"restored dump taken in $format format on destination instance");
+	# Use --create in dump and restore commands so that the restored database
+	# has the same configurable variable settings as the original database so
+	# that the plain dumps taken from both the database taken for comparisong do
+	# not differ because of locale changes. Additionally this provides test
+	# coverage for --create option.
+	#
+	# We use directory format which allows dumping and restoring in parallel to
+	# reduce the test's run time.
+	$src_node->command_ok(
+		[
+			'pg_dump', '-Fd', '-j2', '--no-sync',
+			'-d', $src_node->connstr('regression'),
+			'--create', '-f', $dump_file
+		],
+		"pg_dump on source instance succeeded");
 
-		my $dst_dump =
-		  get_dump_for_comparison($dst_node, 'regression',
-			'dest_dump.' . $format, 0);
+	$dst_node->command_ok(
+		[ 'pg_restore', '--create', '-j2', '-d', 'postgres', $dump_file ],
+		"restored dump to destination instance");
 
-		compare_files($src_dump, $dst_dump,
-			"dump outputs from original and restored regression database (using $format format) match"
-		);
+	my $dst_dump = get_dump_for_comparison($dst_node, 'regression',
+		'dest_dump', 0);
 
-		# Rename the restored database so that it is available for debugging in
-		# case the test fails.
-		$dst_node->safe_psql('postgres', "ALTER DATABASE regression RENAME TO $restored_db");
-	}
+	compare_files($src_dump, $dst_dump,
+			"dump outputs from original and restored regression database match"
+		);
 }
 
 # Dump database `db` from the given `node` in plain format and adjust it for
-- 
2.34.1

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 24 Mar 2025 10:59:03 +0100

> On 24 Mar 2025, at 10:54, Ashutosh Bapat <[email-masked]> wrote:

> the bug is fixed, we should merge this patch to 0001 before

If the reported bug isn't fixed before feature freeze I think we should commit
this regardless as it has clearly shown value by finding bugs (though perhaps
under PG_TEST_EXTRA or in some disconnected till the bug is fixed to limit the
blast-radius in the buildfarm).

--
Daniel Gustafsson

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 24 Mar 2025 13:14:55 +0100

On 2025-Mar-24, Ashutosh Bapat wrote:

> bit more complex.

Oh, it's readable all right.  You just need to use `pg_restore -f-` to
read it.  No big deal.

So I ran this a few times:
/usr/bin/time make -j8 -Otarget -C /pgsql/build/master check-world -s PROVE_FLAGS="-c -j6" > /dev/null

commenting out the call to test_regression_dump_restore() to test how
much additional runtime does the new test incur.

With test:

136.95user 116.56system 1:13.23elapsed 346%CPU (0avgtext+0avgdata 250704maxresident)k
4928inputs+55333008outputs (114major+14784937minor)pagefaults 0swaps

138.11user 117.43system 1:15.54elapsed 338%CPU (0avgtext+0avgdata 278592maxresident)k
48inputs+55333464outputs (80major+14794494minor)pagefaults 0swaps

137.05user 113.13system 1:08.19elapsed 366%CPU (0avgtext+0avgdata 279272maxresident)k
48inputs+55330064outputs (83major+14758028minor)pagefaults 0swaps

without the new test:

135.46user 114.55system 1:14.69elapsed 334%CPU (0avgtext+0avgdata 145372maxresident)k
32inputs+55155256outputs (105major+14737549minor)pagefaults 0swaps

135.48user 114.57system 1:09.60elapsed 359%CPU (0avgtext+0avgdata 148224maxresident)k
16inputs+55155432outputs (95major+14749502minor)pagefaults 0swaps

133.76user 113.26system 1:14.92elapsed 329%CPU (0avgtext+0avgdata 148064maxresident)k
48inputs+55154952outputs (84major+14749531minor)pagefaults 0swaps

134.06user 113.83system 1:16.09elapsed 325%CPU (0avgtext+0avgdata 145940maxresident)k
32inputs+55155032outputs (83major+14738602minor)pagefaults 0swaps

The increase in duration here is less than a second.

My conclusion with these numbers is that it's not worth hiding this test
in PG_TEST_EXTRA.  If we really wanted to save some total test runtime,
it might be better to write a regress schedule file for
027_stream_regress.pl which only takes the test that emit useful WAL,
rather than all tests.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The ability of users to misuse tools is, of course, legendary" (David Steele)
https://postgr.es/m/[email-masked]

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 25 Mar 2025 16:09:14 +0530

On Mon, Mar 24, 2025 at 5:44 PM Alvaro Herrera <[email-masked]> wrote:
> in PG_TEST_EXTRA.

Thanks for the conclusion.

> blast-radius in the buildfarm).

Combining Alvaro's and Daniel's recommendations, I think we should
squash all the three of my patches while committing the test if the
bug is not fixed by then. Otherwise we should squash first two patches
and commit it. Just attaching the patches again for reference.

> If we really wanted to save some total test runtime,

That's out of scope for this patch, but it seems like an idea worth exploring.

 

 
-		my @restore_command;
 
-		my $dst_dump =

 

Note For the reviewers:

Author: Ashutosh Bapat

 

 
 
 uninstall:
 
 endif

Ashutosh Bapat
---

---

From: vignesh C <[email-masked]>
Date: Thu, 27 Mar 2025 18:01:37 +0530

> and commit it. Just attaching the patches again for reference.

Couple of minor thoughts:
1) I felt this error message is not conveying the error message correctly:
+       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;
+       }

how about something like below:
fail("source and destination nodes must have the same PostgreSQL
version and default installation paths");

2) Should "`" be ' or " here, we   generally use "`" to enclose commands:
+# It is expected that regression tests, which create `regression` database, are
+# run on `src_node`, which in turn, is left in running state. A fresh node is
+# created using given `node_params`, which are expected to be the
same ones used
+# to create `src_node`, so as to avoid any differences in the databases.

There are few other instances similarly in the file.

Regards,
Vignesh

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 27 Mar 2025 22:01:37 +0530

> version and default installation paths");

The text in ok(), fail() etc. are test names and not error messages.
See [1]. Your suggestion and other versions that I came up with became
too verbose to be test names. So I think the text here is compromise
between conveying enough information and not being too long. We
usually have to pick the testname and lookup the test code to
investigate the failure. This text serves that purpose.

> +# to create `src_node`, so as to avoid any differences in the databases.

Looking at prologues or some other functions, I see that we don't add
any decoration around the name of the argument. Hence dropped ``
altogether. Will post it with the next set of patches.

[1] https://metacpan.org/pod/Test::More

-- 
Best Wishes,
Ashutosh Bapat

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 27 Mar 2025 18:15:06 +0100

> investigate the failure. This text serves that purpose.

Maybe
fail("roundtrip dump/restore of the regression database")

BTW another idea to shorten this tests's runtime might be to try and
identify which of parallel_schedule tests leave objects behind and
create a shorter schedule with only those (a possible implementation
might keep a list of the slow tests that don't leave any useful object
behind, then filter parallel_schedule to exclude those; this ensures
test files created in the future are still used.)

---

From: Michael Paquier <[email-masked]>
Date: Fri, 28 Mar 2025 10:36:48 +0900

> test files created in the future are still used.)

I'm not much a fan of approaches that require an extra schedule,
because this is prone to forget the addition of objects that we'd want
to cover for the scope of this thread with the dump/restore
inter-dependencies, failing our goal of having more coverage.  And
history has proven that we are quite bad at maintaining multiple
schedules for the regression test suite (remember the serial one or
the standby one in pg_regress?).  So we should really do things so as
the schedules are down to a strict minimum: 1.

If we're worried about the time taken by the test (spoiler: I am and
the upgrade tests already show always as last to finish in parallel
runs), I would recommend to put that under a PG_TEST_EXTRA.  I'm OK to
add the switch to my buildfarm animals if this option is the consensus
and if it gets into the tree.
--
Michael

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 28 Mar 2025 12:02:58 +0530

> fail("roundtrip dump/restore of the regression database")

No, that's losing some information like default installation and the
same version.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 28 Mar 2025 12:20:29 +0530

> the schedules are down to a strict minimum: 1.

I see Alvaro's point about using a different and minimal schedule. We
already have 002_pg_upgrade and 027_stream_ as candidates which could
use schedules other than default and avoid wasting CPU cycles.
But I also agree with your opinion that maintaining multiple schedules
is painful and prone to errors.

What we could do is to create the schedule files automatically during
build. The automation script will require to know which file to place
in which schedules. That information could be either part of the sql
file itself or could be in a separate text file. For example, every
SQL file has the following line listing all the schedules that this
SQL file should be part of. E.g.

-- schedules: parallel, serial, upgrade

The automated script looks at every .sql file in a given sql directory
and creates the schedule files containing all the SQL files which had
respective schedules mentioned in their "schedule" annotation. The
automation script would flag SQL files that do not have scheduled
annotation so any new file added won't be missed. However, we will
still miss a SQL file if it wasn't part of a given schedule and later
acquired some changes which required it to be added to a new schedule.

If we go this route, we could make 'make check-tests' better. We could
add another annotation for depends listing all the SQL files that a
given SQL file depends upon. make check-tests would collect all
dependencies, sort them and run all the dependencies as well.

Of course that's out of scope for this patch. We don't have time left
for this in PG 18.

> and if it gets into the tree.

I would prefer to run this test by default as Alvaro mentioned
previously. But if that means that we won't get this test committed at
all, I am ok putting it under PG_TEST_EXTRA. (Hence I have kept 0001
and 0002 separate.) But I will be disappointed if the test, which has
unearthed four bugs in a year alone, does not get committed to PG 18
because of this debate.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 28 Mar 2025 14:58:10 +0530

Vignesh and Alvaro

> No, that's losing some information like default installation and the

How about "dump and restore across servers with same PostgreSQL
version using default installation". That's still mouthful but is more
readable.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 28 Mar 2025 11:35:26 +0100

> No, that's losing some information like default installation and the

You don't need to preserve such information.  This is just a test name.
People looking for more details can grep for the name and they will find
the comments.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 28 Mar 2025 16:50:54 +0530

> People looking for more details can grep for the name and they will find

Ok. In that case what's wrong with the testname I have in the patch?

-- 
Best Wishes,
Ashutosh Bapat

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 28 Mar 2025 17:57:50 +0530

> Of course that's out of scope for this patch. We don't have time left

I spent several hours today examining each SQL file to decide whether
or not it has "interesting" objects that it leaves behind for
dump/restore test. I came up with attached schedule - which may not be
accurate since I it would require much more time to examine all tests
to get an accurate schedule. But what I have got may be close enough.
With that we could save about 6 seconds on my laptop. If we further
compact the schedule reorganizing the parallel groups we may shave
some more seconds.

no modifications to parallel schedule
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.84s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.80s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.37s   28 subtests passed

with attached modified parallel schedule
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
36.13s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
35.86s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
36.33s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
36.02s   28 subtests passed

However, it's a very painful process to come up with the schedule and
more painful and error prone to maintain it. It could take many days
to come up with the right schedule which can become inaccurate the
moment next SQL file is added OR an existing file is modified to
add/drop "interesting" objects.

--
Best Wishes,
Ashutosh Bapat

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 28 Mar 2025 15:11:15 +0100

> add/drop "interesting" objects.

Hmm, I didn't mean that we'd maintain a separate schedule.  I meant that
we'd take the existing schedule, then apply some Perl magic to it that
grep-outs the tests that we know to contribute nothing, and generate a
new schedule file dynamically.  We don't need to maintain a separate
schedule file.

You're right that if an existing uninteresting test is modified to
create interesting objects, we'd lose coverage of those objects.  That
seems a much smaller problem to me.  So it's just a matter of doing some
Perl map/grep to generate a new schedule file using the attached
exclusion file.

(For what it's worth, what I did to try to determine which tests to
include, rather than scan each file manually, is to run pg_regress with
"test_setup thetest tablespace", then dump the regression database, and
see if anything is there that's not in the dump when I just with just
"test_setup tablespace".  I didn't carry the experiment to completion
though.)

For the future, we could annotate each test as you said, either by
adding a marker on the test file itself, or by adding something next to
its name in the schedule file, so the schedule file could look like:

test: plancache(dump_ignore) limit(stream_ignore) plpgsql copy2
	temp(stream_ignore,dump_ignore) domain rangefuncs(stream_ignore)
	prepare conversion truncate alter_table
	sequence polymorphism rowtypes returning largeobject with xml

... and so on.

---

From: Tom Lane <[email-masked]>
Date: Fri, 28 Mar 2025 10:22:08 -0400

> new schedule file dynamically.  We don't need to maintain a separate

This seems like a fundamentally broken approach to me.

The entire argument for using the core regression tests as a source of
data to test dump/restore is that, more or less "for free", we can
expect to get coverage when new SQL language features are added.
That's always been a little bit questionable --- there's a temptation
to drop objects again at the end of a test script.  But with this,
it becomes a complete crapshoot whether the objects you need will be
included in the dump.

I think instead of going this direction, we really need to create a
separately-purposed script that simply creates "one of everything"
without doing anything else (except maybe loading a little data).
I believe it'd be a lot easier to remember to add to that when
inventing new SQL than to remember to leave something behind from the
core regression tests.  This would also be far faster to run than any
approach that involves picking a random subset of the core test
scripts.

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 28 Mar 2025 19:12:58 +0100

> approach that involves picking a random subset of the core test

FWIW this sounds closely related to what I tried to do with
src/test/modules/test_ddl_deparse; it's currently incomplete, but maybe
we can use that as a starting point.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Always assume the user will do much worse than the stupidest thing
you can imagine."                                (Julien PUYDT)

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 31 Mar 2025 17:07:15 +0530

> > approach that involves picking a random subset of the core test

It's easier to remember to do something or not do something in the
same file than in some other file. I find it hard to believe that
introducing another set of SQL files somewhere far from regress would
make this problem easier.

The number of states in which objects can be left behind in the
regress/sql is very large - and maintaining that 1:1 in some other set
of scripts is impossible unless it's automated.

> we can use that as a starting point.

create_table.sql in test_ddl_deparse has only one statement creating
an inheritance table whereas there are dozens of different states of
parent/child tables created by regress. It will require a lot of work
to bridge the gap between regress_ddl_deparse and regress and more
work to maintain it.

I might be missing something in your ideas.

IMO, whatever we do it should rely on a single set of files. One
possible way could be to break the existing files into three files
each, containing DDL, DML and queries from those files respectively
and create three schedules DDL, DML and queries containing the
respective files. These schedules will be run as required. Standard
regression run runs all the three schedules one by one. But
002_pg_upgrade will run DDL and DML on the source database and run
queries on target - thus checking sanity of the dump/restore or
pg_upgrade beyond just the dump comparison. 027_stream_regress might
run DDL, DML on the source server and queries on the target.

But that too is easier said than done for:
1. Our tests mix all three kinds of statements and also rely on the
order in which they are run. It will require some significant effort
to carefully separate the statements.
2. With the new set of files backpatching would become hard.

--
Best Wishes,
Ashutosh Bapat

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 31 Mar 2025 17:24:03 +0530

On Mon, Mar 31, 2025 at 5:07 PM Ashutosh Bapat
<[email-masked]> wrote:
The bug related to materialized views has been fixed and now the test
passes even if we compare statistics from dumped and restored
databases. Hence removing 0003. In the attached patchset I have also
addressed Vignesh's below comment

-- 
Best Wishes,
Ashutosh Bapat
From 5ef4a15bf229d104028eac3a046636453e1e05fc Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Mon, 24 Mar 2025 11:21:12 +0530
Subject: [PATCH 2/2] Use only one format and make the test run default

 
index 8d22d538529..f7d5b96ecd2 100644

 
-		my @restore_command;
 
-		my $dst_dump =

 
 # Dump database db from the given node in plain format and adjust it for
-- 
2.34.1

From aa1c74951b3b557de8330230185fd5f2ee46ecda Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 27 Jun 2024 10:03:53 +0530
Subject: [PATCH 1/2] Test pg_dump/restore of regression objects

Note For the reviewers:

Author: Ashutosh Bapat

index 00051b85035..8d22d538529 100644

 
 
+# It is expected that regression tests, which create 'regression' database, are
+# run on src_node, which in turn, is left in running state. A fresh node is
+# created using given node_params, which are expected to be the same ones use
+# to create src_node, so as to avoid any differences in the databases.
+# Dump database db from the given node in plain format and adjust it for
+# file_prefix is used to create unique names for all dump files so that they
+# adjust_child_columns is passed to adjust_regress_dumpfile() which actually
 
 uninstall:
 
 endif

base-commit: e2809e3a1015697832ee4d37b75ba1cd0caac0f0
-- 
2.34.1

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 31 Mar 2025 23:17:15 +0200

> On 28 Mar 2025, at 19:12, Alvaro Herrera <[email-masked]> wrote:
> 

Given where we are in the cycle, it seems to make sense to stick to using the
schedule we already have rather than invent a new process for generating it,
and work on that for 19?

--
Daniel Gustafsson