Re: Improve TAP tests of pg_upgrade for cross-version tests

[Michael Paquier]

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

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

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

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

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

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

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl 
b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 2f9b13bf0a..ac03ef5734 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -30,6 +30,27 @@ sub generate_db
"created database with ASCII characters from $from_char to 
$to_char");
 }
 
+# Filter the contents of a dump before its use in a content comparison.
+# This returns the path to the filtered dump.
+sub filter_dump
+{
+   my ($node, $dump_file) = @_;
+   my $dump_contents = slurp_file($dump_file);
+
+   # Remove the comments.
+   $dump_contents =~ s/^\-\-.*//mgx;
+   # Remove empty lines.
+   $dump_contents =~ s/^\n//mgx;
+
+   my $dump_file_filtered = "${dump_file}_filtered";
+   open(my $dh, '>', $dump_file_filtered)
+ || die "opening $dump_file_filtered";
+   print $dh $dump_contents;
+   close($dh);
+
+   return $dump_file_filtered;
+}
+
 # The test of pg_upgrade requires two clusters, an old one and a new one
 # that gets upgraded.  Before running the upgrade, a logical dump of the
 # old cluster is taken, and a second logical dump of the new one is taken
@@ -49,8 +70,10 @@ if (   (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
die "olddump or oldinstall is undefined";
 }
 
-# Temporary location for the dumps taken
+# Paths to the dumps taken during the tests.
 my $tempdir = PostgreSQL::Test::Utils::tempdir;
+my $dump1_file = "$tempdir/dump1.sql";
+my $dump2_file = "$tempdir/dump2.sql";
 
 # Initialize node to upgrade
 my $oldnode =
@@ -60,7 +83,10 @@ my $oldnode =
 # To increase coverage of non-standard segment size and group access without
 # increasing test runtime, run these tests with a custom setting.
 # --allow-group-access and --wal-segsize have been added in v11.
-$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+my %node_params = ();
+$node_params{extra} = [ '--wal-segsize', '1', '--allow-group-access' ]
+if $oldnode->pg_version >= 11;
+$oldnode->init(%node_params);
 $oldnode->start;
 
 # The default location of the source code is the root of this directory.
@@ -129,37 +155,39 @@ else
is($rc, 0, 'regression tests pass');
 }
 
+# Initialize a new node for the upgrade.
+# Initialize a new node for the upgrade.
+my $newnode = PostgreSQL::Test::Cluster->new('new_node');
+$newnode->init(%node_params);
+
+my $newbindir = $newnode->co

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Andres Freund]

Hi,

On 2022-07-29 16:15:26 -0500, Justin Pryzby wrote:
> This was using the old psql rather than the new one.
> Before v10, psql didn't have \if.
> 
> I think Cluster.pm should be updated to support the upgrades that upgrade.sh
> supported.  I guess it ought to be fixed in v15.

This fails tests widely, and has so for a while:
https://cirrus-ci.com/build/4862820121575424
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3649

Note that it causes timeouts, which end up chewing up a cfbot "slot" for an
hour...

Greetings,

Andres Freund

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Anton A. Melnikov]

Hello!

On 30.07.2022 10:29, Michael Paquier wrote:

 [
-   'psql', '-X',
+   "$newbindir/psql", '-X',



Found that adding $newbindir to psql gives an error when upgrading from 
versions 14 and below to master when the test tries to run 
upgrade_adapt.sql script:


t/002_pg_upgrade.pl .. 1/?
#   Failed test 'ran adapt script'
#   at t/002_pg_upgrade.pl line 141.

in regress_log_002_pg_upgrade:
# Running: <$newbindir>/psql -X -f 
<$srcdir>/src/bin/pg_upgrade/upgrade_adapt.sql regression
<$newbindir>/psql: symbol lookup error: <$newbindir>/psql: undefined 
symbol: PQmblenBounded


Tests from 15-stable and from itself work as expected.

Question about similar error was here: 
https://www.postgresql.org/message-id/flat/BN0PR20MB3912AA107FA6E90FB6B0A034FD9F9%40BN0PR20MB3912.namprd20.prod.outlook.com 



With best regards,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Michael Paquier]

On Fri, Jul 29, 2022 at 04:15:26PM -0500, Justin Pryzby wrote:
> This was using the old psql rather than the new one.
> Before v10, psql didn't have \if.

# Note that upgrade_adapt.sql from the new version is used, to
# cope with an upgrade to this version.
-   $oldnode->command_ok(
+   $newnode->command_ok(
[
-   'psql', '-X',
+   "$newbindir/psql", '-X',

Yeah, you are right here that this psql command should use the one
from the new cluster and connect to the old cluster.  There is no
point in adding $newbindir though as Cluster::_get_env would enforce
PATH to find the correct binary.  I'll look at that in details later.
--
Michael


signature.asc
Description: PGP signature

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Justin Pryzby]

On Wed, Jul 06, 2022 at 03:27:28PM +0900, Michael Paquier wrote:
> On Thu, Jun 09, 2022 at 04:49:01PM +0900, Michael Paquier wrote:
> > Rebased to cope with the recent changes in this area.
> 
> Please find attached an updated version of this patch, where I have
> extended the support of the upgrade script down to 9.5 as origin
> version, as ~9.4 now fail because of cluster_name, so Cluster.pm does
> not support that.  FWIW, I have created an extra set of dumps down to
> 9.4.

This was using the old psql rather than the new one.
Before v10, psql didn't have \if.

I think Cluster.pm should be updated to support the upgrades that upgrade.sh
supported.  I guess it ought to be fixed in v15.

-- 
Justin
>From 02d31c8ad1f50d4014a6839734b109ab24189c41 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 6 Jul 2022 15:23:24 +0900
Subject: [PATCH 1/2] Add more filtering capabilities in the dumps.

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

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 9ed48c4e06a..4a79a9e13d9 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -30,6 +30,39 @@ sub generate_db
 		"created database with ASCII characters from $from_char to $to_char");
 }
 
+# Filter the contents of a dump before its use in a content comparison.
+# This returns the path to the filtered dump.
+sub filter_dump
+{
+	my ($node, $tempdir, $dump_file_name) = @_;
+	my $dump_file = "$tempdir/$dump_file_name";
+	my $dump_contents = slurp_file($dump_file);
+
+	# Remove the comments.
+	$dump_contents =~ s/^\-\-.*//mgx;
+	# Remove empty lines.
+	$dump_contents =~ s/^\n//mgx;
+
+	# Locale setup has changed for collations in 15~.
+	$dump_contents =~
+	  s/(^CREATE\sCOLLATION\s.*?)\slocale\s=\s'und'/$1 locale = ''/mgx
+	  if ($node->pg_version < 15);
+
+	# Dumps taken from <= 11 use EXECUTE PROCEDURE.  Replace it
+	# with EXECUTE FUNCTION.
+	$dump_contents =~
+	  s/(^CREATE\sTRIGGER\s.*?)\sEXECUTE\sPROCEDURE/$1 EXECUTE FUNCTION/mgx
+	  if ($node->pg_version < 12);
+
+	my $dump_file_filtered = "$tempdir/${dump_file_name}_filter";
+	open(my $dh, '>', $dump_file_filtered)
+	  || die "opening $dump_file_filtered";
+	print $dh $dump_contents;
+	close($dh);
+
+	return $dump_file_filtered;
+}
+
 # The test of pg_upgrade requires two clusters, an old one and a new one
 # that gets upgraded.  Before running the upgrade, a logical dump of the
 # old cluster is taken, and a second logical dump of the new one is taken
@@ -60,7 +93,10 @@ my $oldnode =
 # To increase coverage of non-standard segment size and group access without
 # increasing test runtime, run these tests with a custom setting.
 # --allow-group-access and --wal-segsize have been added in v11.
-$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+my %node_params = ();
+$node_params{extra} = [ '--wal-segsize', '1', '--allow-group-access' ]
+if $oldnode->pg_version >= 11;
+$oldnode->init(%node_params);
 $oldnode->start;
 
 # The default location of the source code is the root of this directory.
@@ -147,19 +183,19 @@ if (defined($ENV{oldinstall}))
 
 # Initialize a new node for the upgrade.
 my $newnode = PostgreSQL::Test::Cluster->new('new_node');
-$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+$newnode->init(%node_params);
 my $newbindir = $newnode->config_data('--bindir');
 my $oldbindir = $oldnode->config_data('--bindir');
 
 # Take a dump before performing the upgrade as a base comparison. Note
 # that we need to use pg_dumpall from the new node here.
-$newnode->command_ok(
-	[
-		'pg_dumpall', '--no-sync',
-		'-d', $oldnode->connstr('postgres'),
-		'-f', "$tempdir/dump1.sql"
-	],
-	'dump before running pg_upgrade');
+my @dump_command = (
+	'pg_dumpall', '--no-sync', '-d', $oldnode->connstr('postgres'),
+	'-f', "$tempdir/dump1.sql");
+# --extra-float-digits is needed when upgrading from a version older than 11.
+push(@dump_command, '--extra-float-digits', '0')
+  if ($oldnode->pg_version < 12);
+$newnode->command_ok(\@dump_command, 'dump before running pg_upgrade');
 
 # After dumping, update references to the old source tree's regress.so
 # to point to the new tree.
@@ -286,24 +322,35 @@ if (-d $log_path)
 }
 
 # Second dump from the upgraded instance.
-$newnode->command_ok(
-	[
-		'pg_dumpall', '--no-sync',
-		'-d', $newnode->connstr('postgres'),
-		'-f', "$tempdir/dump2.sql"
-	],
-	'dump after running pg_upgrade');
+@dump_command = (
+	'pg_dumpall', '--no-sync', '-d', $newnode->connstr('postgres'),
+	'-f', "$tempdir/dump2.sql");
+# --extra-float-digits

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Michael Paquier]

On Thu, Jun 09, 2022 at 04:49:01PM +0900, Michael Paquier wrote:
> Rebased to cope with the recent changes in this area.

Please find attached an updated version of this patch, where I have
extended the support of the upgrade script down to 9.5 as origin
version, as ~9.4 now fail because of cluster_name, so Cluster.pm does
not support that.  FWIW, I have created an extra set of dumps down to
9.4.

This adds proper handling for initdb --wal-segsize and
--allow-group-access when it comes to v11~, as these options are
missing in ~10.
--
Michael
From d6a585e7b0180ca9e4cda56474ceffe146593ce2 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 6 Jul 2022 15:23:24 +0900
Subject: [PATCH v3] Add more filtering capabilities in the dumps.

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

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 2f9b13bf0a..159417ceec 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -30,6 +30,39 @@ sub generate_db
 		"created database with ASCII characters from $from_char to $to_char");
 }
 
+# Filter the contents of a dump before its use in a content comparison.
+# This returns the path to the filtered dump.
+sub filter_dump
+{
+	my ($node, $tempdir, $dump_file_name) = @_;
+	my $dump_file = "$tempdir/$dump_file_name";
+	my $dump_contents = slurp_file($dump_file);
+
+	# Remove the comments.
+	$dump_contents =~ s/^\-\-.*//mgx;
+	# Remove empty lines.
+	$dump_contents =~ s/^\n//mgx;
+
+	# Locale setup has changed for collations in 15~.
+	$dump_contents =~
+	  s/(^CREATE\sCOLLATION\s.*?)\slocale\s=\s'und'/$1 locale = ''/mgx
+	  if ($node->pg_version < 15);
+
+	# Dumps taken from <= 11 use EXECUTE PROCEDURE.  Replace it
+	# with EXECUTE FUNCTION.
+	$dump_contents =~
+	  s/(^CREATE\sTRIGGER\s.*?)\sEXECUTE\sPROCEDURE/$1 EXECUTE FUNCTION/mgx
+	  if ($node->pg_version < 12);
+
+	my $dump_file_filtered = "$tempdir/${dump_file_name}_filter";
+	open(my $dh, '>', $dump_file_filtered)
+	  || die "opening $dump_file_filtered";
+	print $dh $dump_contents;
+	close($dh);
+
+	return $dump_file_filtered;
+}
+
 # The test of pg_upgrade requires two clusters, an old one and a new one
 # that gets upgraded.  Before running the upgrade, a logical dump of the
 # old cluster is taken, and a second logical dump of the new one is taken
@@ -60,7 +93,10 @@ my $oldnode =
 # To increase coverage of non-standard segment size and group access without
 # increasing test runtime, run these tests with a custom setting.
 # --allow-group-access and --wal-segsize have been added in v11.
-$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+my %node_params = ();
+$node_params{extra} = [ '--wal-segsize', '1', '--allow-group-access' ]
+if $oldnode->pg_version >= 11;
+$oldnode->init(%node_params);
 $oldnode->start;
 
 # The default location of the source code is the root of this directory.
@@ -147,19 +183,19 @@ if (defined($ENV{oldinstall}))
 
 # Initialize a new node for the upgrade.
 my $newnode = PostgreSQL::Test::Cluster->new('new_node');
-$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+$newnode->init(%node_params);
 my $newbindir = $newnode->config_data('--bindir');
 my $oldbindir = $oldnode->config_data('--bindir');
 
 # Take a dump before performing the upgrade as a base comparison. Note
 # that we need to use pg_dumpall from the new node here.
-$newnode->command_ok(
-	[
-		'pg_dumpall', '--no-sync',
-		'-d', $oldnode->connstr('postgres'),
-		'-f', "$tempdir/dump1.sql"
-	],
-	'dump before running pg_upgrade');
+my @dump_command = (
+	'pg_dumpall', '--no-sync', '-d', $oldnode->connstr('postgres'),
+	'-f', "$tempdir/dump1.sql");
+# --extra-float-digits is needed when upgrading from a version older than 11.
+push(@dump_command, '--extra-float-digits', '0')
+  if ($oldnode->pg_version < 12);
+$newnode->command_ok(\@dump_command, 'dump before running pg_upgrade');
 
 # After dumping, update references to the old source tree's regress.so
 # to point to the new tree.
@@ -284,24 +320,35 @@ if (-d $log_path)
 }
 
 # Second dump from the upgraded instance.
-$newnode->command_ok(
-	[
-		'pg_dumpall', '--no-sync',
-		'-d', $newnode->connstr('postgres'),
-		'-f', "$tempdir/dump2.sql"
-	],
-	'dump after running pg_upgrade');
+@dump_command = (
+	'pg_dumpall', '--no-sync', '-d', $newnode->connstr('postgres'),
+	'-f', "$tempdir/dump2.sql");
+# --extra-float-digits is needed when upgrading from a version older than 11.
+push(@dump_command, '--extra-float-digits', '0')
+  if ($oldnode->pg_version < 12);
+$newnode->command_ok(\

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Andrew Dunstan]

On 2022-06-13 Mo 03:51, Michael Paquier wrote:
> On Sun, Jun 12, 2022 at 05:58:54PM -0400, Andrew Dunstan wrote:
>> On 2022-06-12 Su 10:14, Andrew Dunstan wrote:
>>> I tried in fb16d2c658 to avoid littering the mainline code with
>>> version-specific tests, and put that in the methods in the subclasses
>>> that override the mainline functions.
> Except that manipulating the diffs of pg_upgrade is not something that
> needs to be internal to the subclasses where we set up the nodes :)
>
>> I think I must have been insufficiently caffeinated when I wrote this,
>> because clearly I was not reading correctly.
>>
>> I have had another look and the patch looks fine, although I haven't
>> tested it.
> I have a question about the tests done in the buildfarm though.  Do
> the dumps from the older versions drop some of the objects that cause
> the diffs in the dumps?  At which extent is that a dump from
> installcheck?


See lines 324..347 (save stage) and 426..586 (upgrade stage) of


We save the cluster to be upgraded after all the installcheck stages
have run, so on crake here's the list of databases upgraded for HEAD:


postgres
template1
template0
contrib_regression_pgxml
contrib_regression_bool_plperl
contrib_regression_hstore_plperl
contrib_regression_jsonb_plperl
regression
contrib_regression_redis_fdw
contrib_regression_file_textarray_fdw
isolation_regression
pl_regression_plpgsql
contrib_regression_hstore_plpython3
pl_regression_plperl
pl_regression_plpython3
pl_regression_pltcl
contrib_regression_adminpack
contrib_regression_amcheck
contrib_regression_bloom
contrib_regression_btree_gin
contrib_regression_btree_gist
contrib_regression_citext
contrib_regression_cube
contrib_regression_dblink
contrib_regression_dict_int
contrib_regression_dict_xsyn
contrib_regression_earthdistance
contrib_regression_file_fdw
contrib_regression_fuzzystrmatch
contrib_regression_hstore
contrib_regression__int
contrib_regression_isn
contrib_regression_lo
contrib_regression_ltree
contrib_regression_pageinspect
contrib_regression_passwordcheck
contrib_regression_pg_surgery
contrib_regression_pg_trgm
contrib_regression_pgstattuple
contrib_regression_pg_visibility
contrib_regression_postgres_fdw
contrib_regression_seg
contrib_regression_tablefunc
contrib_regression_tsm_system_rows
contrib_regression_tsm_system_time
contrib_regression_unaccent
contrib_regression_pgcrypto
contrib_regression_uuid-ossp
contrib_regression_jsonb_plpython3
contrib_regression_ltree_plpython3
isolation_regression_summarization-and-inprogress-insertion
isolation_regression_delay_execution
contrib_regression_dummy_index_am
contrib_regression_dummy_seclabel
contrib_regression_plsample
contrib_regression_spgist_name_ops
contrib_regression_test_bloomfilter
contrib_regression_test_extensions
contrib_regression_test_ginpostinglist
contrib_regression_test_integerset
contrib_regression_test_parser
contrib_regression_test_pg_dump
contrib_regression_test_predtest
contrib_regression_test_rbtree
contrib_regression_test_regex
contrib_regression_test_rls_hooks
contrib_regression_test_shm_mq
contrib_regression_rolenames


cheers


andrew





--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Michael Paquier]

On Sun, Jun 12, 2022 at 05:58:54PM -0400, Andrew Dunstan wrote:
> On 2022-06-12 Su 10:14, Andrew Dunstan wrote:
>> I tried in fb16d2c658 to avoid littering the mainline code with
>> version-specific tests, and put that in the methods in the subclasses
>> that override the mainline functions.

Except that manipulating the diffs of pg_upgrade is not something that
needs to be internal to the subclasses where we set up the nodes :)

> I think I must have been insufficiently caffeinated when I wrote this,
> because clearly I was not reading correctly.
> 
> I have had another look and the patch looks fine, although I haven't
> tested it.

I have a question about the tests done in the buildfarm though.  Do
the dumps from the older versions drop some of the objects that cause
the diffs in the dumps?  At which extent is that a dump from
installcheck?
--
Michael


signature.asc
Description: PGP signature

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Andrew Dunstan]

On 2022-06-12 Su 10:14, Andrew Dunstan wrote:
> On 2022-06-09 Th 03:49, Michael Paquier wrote:
>> On Wed, May 25, 2022 at 10:35:57AM +0900, Michael Paquier wrote:
>>> On Tue, May 24, 2022 at 03:03:28PM +0900, Michael Paquier wrote:
 (Adding Andrew Dunstan in CC.)

 I have been toying with $subject, trying to improve the ways to test
 pg_upgrade across different major versions as perl makes that easier.
 The buildfarm does three things to allow such tests to work (see
 TestUpgradeXversion.pm):
>> Rebased to cope with the recent changes in this area.
>
> I tried in fb16d2c658 to avoid littering the mainline code with
> version-specific tests, and put that in the methods in the subclasses
> that override the mainline functions.
>
> I'll try to rework this along those lines.
>
>

I think I must have been insufficiently caffeinated when I wrote this,
because clearly I was not reading correctly.


I have had another look and the patch looks fine, although I haven't
tested it.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Andrew Dunstan]

On 2022-06-09 Th 03:49, Michael Paquier wrote:
> On Wed, May 25, 2022 at 10:35:57AM +0900, Michael Paquier wrote:
>> On Tue, May 24, 2022 at 03:03:28PM +0900, Michael Paquier wrote:
>>> (Adding Andrew Dunstan in CC.)
>>>
>>> I have been toying with $subject, trying to improve the ways to test
>>> pg_upgrade across different major versions as perl makes that easier.
>>> The buildfarm does three things to allow such tests to work (see
>>> TestUpgradeXversion.pm):
> Rebased to cope with the recent changes in this area.


I tried in fb16d2c658 to avoid littering the mainline code with
version-specific tests, and put that in the methods in the subclasses
that override the mainline functions.

I'll try to rework this along those lines.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Michael Paquier]

On Wed, May 25, 2022 at 10:35:57AM +0900, Michael Paquier wrote:
> On Tue, May 24, 2022 at 03:03:28PM +0900, Michael Paquier wrote:
>> (Adding Andrew Dunstan in CC.)
>> 
>> I have been toying with $subject, trying to improve the ways to test
>> pg_upgrade across different major versions as perl makes that easier.
>> The buildfarm does three things to allow such tests to work (see
>> TestUpgradeXversion.pm):

Rebased to cope with the recent changes in this area.
--
Michael
From 61d69469887533bbdde75d8aad1d2d39907ac831 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 9 Jun 2022 16:42:47 +0900
Subject: [PATCH v2] Add more filtering capabilities in the dumps.

This adds support for some filtering capabilities, for some tests done
down to v11.  This allows the tests to pass with v14, while v11~13 still
generate a few diffs.
---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 78 --
 1 file changed, 61 insertions(+), 17 deletions(-)

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 3f11540e18..20906181b2 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -30,6 +30,39 @@ sub generate_db
 		"created database with ASCII characters from $from_char to $to_char");
 }
 
+# Filter the contents of a dump before its use in a content comparison.
+# This returns the path to the filtered dump.
+sub filter_dump
+{
+	my ($node, $tempdir, $dump_file_name) = @_;
+	my $dump_file = "$tempdir/$dump_file_name";
+	my $dump_contents = slurp_file($dump_file);
+
+	# Remove the comments.
+	$dump_contents =~ s/^\-\-.*//mgx;
+	# Remove empty lines.
+	$dump_contents =~ s/^\n//mgx;
+
+	# Locale setup has changed for collations in 15~.
+	$dump_contents =~
+	  s/(^CREATE\sCOLLATION\s.*?)\slocale\s=\s'und'/$1 locale = ''/mgx
+	  if ($node->pg_version < 15);
+
+	# Dumps taken from <= 11 use EXECUTE PROCEDURE.  Replace it
+	# with EXECUTE FUNCTION.
+	$dump_contents =~
+	  s/(^CREATE\sTRIGGER\s.*?)\sEXECUTE\sPROCEDURE/$1 EXECUTE FUNCTION/mgx
+	  if ($node->pg_version < 12);
+
+	my $dump_file_filtered = "$tempdir/${dump_file_name}_filter";
+	open(my $dh, '>', $dump_file_filtered)
+	  || die "opening $dump_file_filtered";
+	print $dh $dump_contents;
+	close($dh);
+
+	return $dump_file_filtered;
+}
+
 # The test of pg_upgrade requires two clusters, an old one and a new one
 # that gets upgraded.  Before running the upgrade, a logical dump of the
 # old cluster is taken, and a second logical dump of the new one is taken
@@ -153,13 +186,13 @@ my $oldbindir = $oldnode->config_data('--bindir');
 
 # Take a dump before performing the upgrade as a base comparison. Note
 # that we need to use pg_dumpall from the new node here.
-$newnode->command_ok(
-	[
-		'pg_dumpall', '--no-sync',
-		'-d', $oldnode->connstr('postgres'),
-		'-f', "$tempdir/dump1.sql"
-	],
-	'dump before running pg_upgrade');
+my @dump_command = (
+	'pg_dumpall', '--no-sync', '-d', $oldnode->connstr('postgres'),
+	'-f', "$tempdir/dump1.sql");
+# --extra-float-digits is needed when upgrading from a version older than 11.
+push(@dump_command, '--extra-float-digits', '0')
+  if ($oldnode->pg_version < 12);
+$newnode->command_ok(\@dump_command, 'dump before running pg_upgrade');
 
 # After dumping, update references to the old source tree's regress.so
 # to point to the new tree.
@@ -282,24 +315,35 @@ if (-d $log_path)
 }
 
 # Second dump from the upgraded instance.
-$newnode->command_ok(
-	[
-		'pg_dumpall', '--no-sync',
-		'-d', $newnode->connstr('postgres'),
-		'-f', "$tempdir/dump2.sql"
-	],
-	'dump after running pg_upgrade');
+@dump_command = (
+	'pg_dumpall', '--no-sync', '-d', $newnode->connstr('postgres'),
+	'-f', "$tempdir/dump2.sql");
+# --extra-float-digits is needed when upgrading from a version older than 11.
+push(@dump_command, '--extra-float-digits', '0')
+  if ($oldnode->pg_version < 12);
+$newnode->command_ok(\@dump_command, 'dump after running pg_upgrade');
+
+# Filter the contents of the dumps from the old version of any contents.
+my $dump1_filtered = "$tempdir/dump1.sql";
+my $dump2_filtered = "$tempdir/dump2.sql";
+
+# No need to apply filters on the dumps if working on the same version.
+if ($oldnode->pg_version != $newnode->pg_version)
+{
+	$dump1_filtered = filter_dump($oldnode, $tempdir, "dump1.sql");
+	$dump2_filtered = filter_dump($newnode, $tempdir, "dump2.sql");
+}
 
 # Compare the two dumps, there should be no differences.
-my $compare_res = compare("$tempdir/dump1.sql", "$tempdir/dump2.sql");
+my $compare_res = compare($dump1_filtered, $dump2_filtered);
 is($compare_res, 0, 'old and new dumps match after pg_upgrade');
 
 # Provide more context if the dumps do not match.
 if ($compare_res != 0)
 {
 	my ($stdout, $stderr) =
-	  run_command([ 'diff', "$tempdir/dump1.sql", "$tempdir/dump2.sql" ]);
-	print "=== diff of $tempdir/dump1.sql and $tempdir/dump2.sql\n";
+	  run_command([ 'diff', $d

Re: Improve TAP tests of pg_upgrade for cross-version tests

[Michael Paquier]

On Tue, May 24, 2022 at 03:03:28PM +0900, Michael Paquier wrote:
> (Adding Andrew Dunstan in CC.)
> 
> I have been toying with $subject, trying to improve the ways to test
> pg_upgrade across different major versions as perl makes that easier.
> The buildfarm does three things to allow such tests to work (see
> TestUpgradeXversion.pm):

And with Andrew in CC, that's better ;p
--
Michael


signature.asc
Description: PGP signature