Subject: Extracting cross-version-upgrade knowledge from buildfarm client

From: Tom Lane <[email-masked]>
Date: Fri, 13 Jan 2023 19:48:16 -0500

This is a followup to the discussion at [1], in which we agreed that
it's time to fix the buildfarm client so that knowledge about
cross-version discrepancies in pg_dump output can be moved into
the community git tree, making it feasible for people other than
Andrew to fix problems when we change things of that sort.
The idea is to create helper files that live in the git tree and
are used by the BF client to perform the activities that are likely
to need tweaking.

Attached are two patches, one for PG git and one for the buildfarm
client, that create a working POC for this approach.  I've only
carried this as far as making a helper file for HEAD, but I believe
that helper files for the back branches would mostly just need to
be cut-down versions of this one.  I've tested it successfully with
cross-version upgrade tests down to 9.3.  (9.2 would need some more
work, and I'm not sure if it's worth the trouble --- are we going to
retire 9.2 soon?)

I'm a very mediocre Perl programmer, so I'm sure there are stylistic
and other problems, but I'm encouraged that this seems feasible.

Also, I wonder if we can't get rid of
src/bin/pg_upgrade/upgrade_adapt.sql in favor of using this code.
I tried to write adjust_database_contents() in such a way that it
could be pointed at a database by some other Perl code that's
not the buildfarm client.

			regards, tom lane

[1] https://www.postgresql.org/message-id/951602.1673535249%40sss.pgh.pa.us
diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
new file mode 100644
index 0000000000..279b2bd0e6
--- /dev/null
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -0,0 +1,421 @@
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+=pod
+
+=head1 NAME
+
+PostgreSQL::Test::AdjustUpgrade - helper module for cross-version upgrade tests
+
+=head1 SYNOPSIS
+
+  use PostgreSQL::Test::AdjustUpgrade;
+
+  # Adjust contents of old-version database before dumping
+  adjust_database_contents($old_version, $psql, %dbnames);
+
+  # Adjust contents of old pg_dumpall output file to match newer version
+  $dump = adjust_old_dumpfile($old_version, $dump);
+
+  # Adjust contents of new pg_dumpall output file to match older version
+  $dump = adjust_new_dumpfile($old_version, $dump);
+
+=head1 DESCRIPTION
+
+C<PostgreSQL::Test::AdjustUpgrade> encapsulates various hacks needed to
+compare the results of cross-version upgrade tests.
+
+=cut
+
+package PostgreSQL::Test::AdjustUpgrade;
+
+use strict;
+use warnings;
+
+use Exporter 'import';
+
+our @EXPORT = qw(
+  adjust_database_contents
+  adjust_old_dumpfile
+  adjust_new_dumpfile
+);
+
+=pod
+
+=head1 ROUTINES
+
+=over
+
+=item adjust_database_contents($old_version, $psql, %dbnames)
+
+Perform any DDL operations against the old-version installation that are
+needed before we can pg_upgrade it into the current PostgreSQL version.
+
+Typically this involves dropping or adjusting no-longer-supported objects.
+
+Arguments:
+
+=over
+
+=item C<old_version>: Branch we are upgrading from, e.g. 'REL_11_STABLE'
+
+=item C<psql>: Object with a method C<old_psql($dbname, $sql_command)>
+to perform SQL against the old installation, returning psql's exit status
+
+=item C<dbnames>: Hash of database names present in the old installation
+
+=back
+
+=cut
+
+sub adjust_database_contents
+{
+	my ($old_version, $psql, %dbnames) = @_;
+
+	# nothing to do for non-cross-version tests
+	return if $old_version eq 'HEAD';
+
+	# stuff not supported from release 16
+	if (    $old_version ge 'REL_12_STABLE'
+		and $old_version lt 'REL_16_STABLE')
+	{
+		# Can't upgrade aclitem in user tables from pre 16 to 16+.
+		# Also can't handle child tables with locally-generated columns.
+		my $prstmt = join(';',
+			'alter table public.tab_core_types drop column aclitem',
+			'drop table public.gtest_normal_child',
+			'drop table public.gtest_normal_child2');
+
+		$psql->old_psql("regression", $prstmt);
+		return if $?;
+	}
+
+	# stuff not supported from release 14
+	if ($old_version lt 'REL_14_STABLE')
+	{
+		# postfix operators (some don't exist in very old versions)
+		my $prstmt = join(';',
+			'drop operator #@# (bigint,NONE)',
+			'drop operator #%# (bigint,NONE)',
+			'drop operator if exists !=- (bigint,NONE)',
+			'drop operator if exists #@%# (bigint,NONE)');
+
+		$psql->old_psql("regression", $prstmt);
+		return if $?;
+
+		# get rid of dblink's dependencies on regress.so
+		$prstmt = join(';',
+			'drop function if exists public.putenv(text)',
+			'drop function if exists public.wait_pid(integer)');
+
+		my $regrdb =
+		  $old_version le "REL9_4_STABLE"
+		  ? "contrib_regression"
+		  : "contrib_regression_dblink";
+
+		if ($dbnames{$regrdb})
+		{
+			$psql->old_psql($regrdb, $prstmt);
+			return if $?;
+		}
+	}
+
+	# user table OIDs are gone from release 12 on
+	if ($old_version lt 'REL_12_STABLE')
+	{
+		my $nooid_stmt = q{
+           DO $stmt$
+           DECLARE
+              rec text;
+           BEGIN
+              FOR rec in
+                 select oid::regclass::text
+                 from pg_class
+                 where relname !~ '^pg_'
+                    and relhasoids
+                    and relkind in ('r','m')
+                 order by 1
+              LOOP
+                 execute 'ALTER TABLE ' || rec || ' SET WITHOUT OIDS';
+                 RAISE NOTICE 'removing oids from table %', rec;
+              END LOOP;
+           END; $stmt$;
+        };
+
+		foreach my $oiddb ("regression", "contrib_regression_btree_gist")
+		{
+			next unless $dbnames{$oiddb};
+			$psql->old_psql($oiddb, $nooid_stmt);
+			return if $?;
+		}
+
+		# this one had OIDs too, but we'll just drop it
+		if (   $old_version ge 'REL_10_STABLE'
+			&& $dbnames{'contrib_regression_postgres_fdw'})
+		{
+			$psql->old_psql(
+				"contrib_regression_postgres_fdw",
+				"drop foreign table ft_pg_type");
+			return if $?;
+		}
+	}
+
+	# abstime+friends are gone from release 12 on; but these tables
+	# might or might not be present depending on regression test vintage
+	if ($old_version lt 'REL_12_STABLE')
+	{
+		$psql->old_psql("regression",
+			"drop table if exists abstime_tbl, reltime_tbl, tinterval_tbl");
+		return if $?;
+	}
+
+	# some regression functions gone from release 11 on
+	if ($old_version lt 'REL_11_STABLE')
+	{
+		my $prstmt = join(';',
+			'drop function if exists public.boxarea(box)',
+			'drop function if exists public.funny_dup17()');
+
+		$psql->old_psql("regression", $prstmt);
+		return if $?;
+	}
+
+	# version-0 C functions are no longer supported
+	if ($old_version lt 'REL_10_STABLE')
+	{
+		$psql->old_psql("regression",
+			"drop function oldstyle_length(integer, text)");
+		return if $?;
+	}
+
+	if ($old_version lt 'REL9_5_STABLE')
+	{
+		# changes of underlying functions
+		my $prstmt = join(';',
+			'drop operator @#@ (NONE, bigint)',
+			'CREATE OPERATOR @#@ ('
+			  . 'PROCEDURE = factorial, RIGHTARG = bigint )',
+			'drop aggregate public.array_cat_accum(anyarray)',
+			'CREATE AGGREGATE array_larger_accum (anyarray) ' . ' ( '
+			  . '   sfunc = array_larger, '
+			  . '   stype = anyarray, '
+			  . '   initcond = $${}$$ '
+			  . '  ) ');
+
+		$psql->old_psql("regression", $prstmt);
+		return if $?;
+
+		# "=>" is no longer valid as an operator name
+		$psql->old_psql("regression",
+			'drop operator if exists public.=> (bigint, NONE)');
+		return if $?;
+	}
+
+	# remove dbs of modules known to cause pg_upgrade to fail
+	# anything not builtin and incompatible should clean up its own db
+	foreach my $bad_module ("test_ddl_deparse", "tsearch2")
+	{
+		if ($dbnames{"contrib_regression_$bad_module"})
+		{
+			$psql->old_psql("postgres",
+				"drop database contrib_regression_$bad_module");
+			return if $?;
+		}
+	}
+
+	# dblink in 9.5 has permissions oddities, not worth fixing
+	if (   $old_version eq 'REL9_5_STABLE'
+		&& $dbnames{"contrib_regression_dblink"})
+	{
+		$psql->old_psql("postgres",
+			"drop database contrib_regression_dblink");
+		return if $?;
+	}
+
+	# avoid version number issues with test_ext7
+	if ($dbnames{contrib_regression_test_extensions})
+	{
+		$psql->old_psql(
+			"contrib_regression_test_extensions",
+			"drop extension if exists test_ext7");
+		return if $?;
+	}
+
+	return;
+}
+
+=pod
+
+=item adjust_old_dumpfile($old_version, $dump)
+
+Edit a dump output file, taken from the adjusted old-version installation
+by current-version C<pg_dumpall -s>, so that it will match the results of
+C<pg_dumpall -s> on the pg_upgrade'd installation.
+
+Typically this involves coping with cosmetic differences in the output
+of backend subroutines used by pg_dump.
+
+Arguments:
+
+=over
+
+=item C<old_version>: Branch we are upgrading from, e.g. 'REL_11_STABLE'
+
+=item C<dump>: Contents of dump file
+
+=back
+
+Returns the modified dump text.
+
+=cut
+
+sub adjust_old_dumpfile
+{
+	my ($old_version, $dump) = @_;
+
+	# nothing to do for non-cross-version tests
+	return $dump if $old_version eq 'HEAD';
+
+	# Version comments will certainly not match.
+	$dump =~ s/^-- Dumped from database version.*\n//mg;
+
+	if (    $old_version ge 'REL_14_STABLE'
+		and $old_version lt 'REL_16_STABLE')
+	{
+		# Fix up some privilege-set discrepancies.
+		$dump =~
+		  s/^REVOKE SELECT,INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLE/REVOKE ALL ON TABLE/mg;
+		$dump =~
+		  s/^GRANT SELECT,INSERT,REFERENCES,TRIGGER,TRUNCATE,UPDATE ON TABLE/GRANT SELECT,INSERT,REFERENCES,TRIGGER,TRUNCATE,MAINTAIN,UPDATE ON TABLE/mg;
+	}
+
+	if ($old_version lt 'REL_14_STABLE')
+	{
+		# Remove mentions of extended hash functions.
+		$dump =~
+		  s/^(\s+OPERATOR 1 =\(integer,integer\)) ,\n\s+FUNCTION 2 \(integer, integer\) public\.part_hashint4_noop\(integer,bigint\);/$1;/mg;
+		$dump =~
+		  s/^(\s+OPERATOR 1 =\(text,text\)) ,\n\s+FUNCTION 2 \(text, text\) public\.part_hashtext_length\(text,bigint\);/$1;/mg;
+	}
+
+	# Change trigger definitions to say ... EXECUTE FUNCTION ...
+	if ($old_version lt 'REL_12_STABLE')
+	{
+		# would like to use lookbehind here but perl complains
+		# so do it this way
+		$dump =~ s/
+			(^CREATE\sTRIGGER\s.*?)
+			\sEXECUTE\sPROCEDURE
+			/$1 EXECUTE FUNCTION/mgx;
+	}
+
+	if ($old_version lt 'REL9_6_STABLE')
+	{
+		# adjust some places where we don't print so many parens anymore
+		$dump =~
+		  s/^'New York'\tnew & york \| big & apple \| nyc\t'new' & 'york'\t\( 'new' & 'york' \| 'big' & 'appl' \) \| 'nyc'/'New York'\tnew & york | big & apple | nyc\t'new' & 'york'\t'new' & 'york' | 'big' & 'appl' | 'nyc'/mg;
+		$dump =~
+		  s/^'Sanct Peter'\tPeterburg \| peter \| 'Sanct Peterburg'\t'sanct' & 'peter'\t\( 'peterburg' \| 'peter' \) \| 'sanct' & 'peterburg'/'Sanct Peter'\tPeterburg | peter | 'Sanct Peterburg'\t'sanct' & 'peter'\t'peterburg' | 'peter' | 'sanct' & 'peterburg'/mg;
+	}
+
+	if ($old_version lt 'REL9_5_STABLE')
+	{
+		# adjust some places where we don't print so many parens anymore
+		$dump =~
+		  s/CONSTRAINT sequence_con CHECK \(\(\(\(x > 3\) AND \(y <> 'check failed'::text\)\) AND \(z < 8\)\)\)/CONSTRAINT sequence_con CHECK (((x > 3) AND (y <> 'check failed'::text) AND (z < 8)))/mg;
+		$dump =~
+		  s/CONSTRAINT copy_con CHECK \(\(\(\(x > 3\) AND \(y <> 'check failed'::text\)\) AND \(x < 7\)\)\)/CONSTRAINT copy_con CHECK (((x > 3) AND (y <> 'check failed'::text) AND (x < 7)))/mg;
+		$dump =~
+		  s/CONSTRAINT insert_con CHECK \(\(\(\(x >= 3\) AND \(y <> 'check failed'::text\)\) AND \(x < 8\)\)\)/CONSTRAINT insert_con CHECK (((x >= 3) AND (y <> 'check failed'::text) AND (x < 8)))/mg;
+		$dump =~
+		  s/DEFAULT \(\(-1\) \* currval\('public\.insert_seq'::regclass\)\)/DEFAULT ('-1'::integer * currval('public.insert_seq'::regclass))/mg;
+		$dump =~
+		  s/WHERE \(\(\(rsl\.sl_color = rsh\.slcolor\) AND \(rsl\.sl_len_cm >= rsh\.slminlen_cm\)\) AND \(rsl\.sl_len_cm <= rsh\.slmaxlen_cm\)\)/WHERE ((rsl.sl_color = rsh.slcolor) AND (rsl.sl_len_cm >= rsh.slminlen_cm) AND (rsl.sl_len_cm <= rsh.slmaxlen_cm))/mg;
+		$dump =~
+		  s/WHERE \(\(\(rule_and_refint_t3\.id3a = new\.id3a\) AND \(rule_and_refint_t3\.id3b = new\.id3b\)\) AND \(rule_and_refint_t3\.id3c = new\.id3c\)\)/WHERE ((rule_and_refint_t3.id3a = new.id3a) AND (rule_and_refint_t3.id3b = new.id3b) AND (rule_and_refint_t3.id3c = new.id3c))/mg;
+		$dump =~
+		  s/WHERE \(\(\(rule_and_refint_t3_1\.id3a = new\.id3a\) AND \(rule_and_refint_t3_1\.id3b = new\.id3b\)\) AND \(rule_and_refint_t3_1\.id3c = new\.id3c\)\)/WHERE ((rule_and_refint_t3_1.id3a = new.id3a) AND (rule_and_refint_t3_1.id3b = new.id3b) AND (rule_and_refint_t3_1.id3c = new.id3c))/mg;
+	}
+
+	# Suppress blank lines, as some places in pg_dump emit more or fewer.
+	$dump =~ s/\n\n+/\n/g;
+
+	return $dump;
+}
+
+=pod
+
+=item adjust_new_dumpfile($old_version, $dump)
+
+Edit a dump output file, taken from the pg_upgrade'd installation
+by current-version C<pg_dumpall -s>, so that it will match the old
+dump output file as adjusted by C<adjust_old_dumpfile>.
+
+Typically this involves deleting data not present in the old installation.
+
+Arguments:
+
+=over
+
+=item C<old_version>: Branch we are upgrading from, e.g. 'REL_11_STABLE'
+
+=item C<dump>: Contents of dump file
+
+=back
+
+Returns the modified dump text.
+
+=cut
+
+sub adjust_new_dumpfile
+{
+	my ($old_version, $dump) = @_;
+
+	# nothing to do for non-cross-version tests
+	return $dump if $old_version eq 'HEAD';
+
+	# Version comments will certainly not match.
+	$dump =~ s/^-- Dumped from database version.*\n//mg;
+
+	if ($old_version lt 'REL_14_STABLE')
+	{
+		# Suppress noise-word uses of IN in CREATE/ALTER PROCEDURE.
+		$dump =~ s/^(CREATE PROCEDURE .*?)\(IN /$1(/mg;
+		$dump =~ s/^(ALTER PROCEDURE .*?)\(IN /$1(/mg;
+		$dump =~ s/^(CREATE PROCEDURE .*?), IN /$1, /mg;
+		$dump =~ s/^(ALTER PROCEDURE .*?), IN /$1, /mg;
+		$dump =~ s/^(CREATE PROCEDURE .*?), IN /$1, /mg;
+		$dump =~ s/^(ALTER PROCEDURE .*?), IN /$1, /mg;
+
+		# Remove SUBSCRIPT clauses in CREATE TYPE.
+		$dump =~ s/^\s+SUBSCRIPT = raw_array_subscript_handler,\n//mg;
+
+		# Remove multirange_type_name clauses in CREATE TYPE AS RANGE.
+		$dump =~ s/,\n\s+multirange_type_name = .*?(,?)$/$1/mg;
+
+		# Remove mentions of extended hash functions.
+		$dump =~
+		  s/^ALTER OPERATOR FAMILY public\.part_test_int4_ops USING hash ADD\n\s+FUNCTION 2 \(integer, integer\) public\.part_hashint4_noop\(integer,bigint\);//mg;
+		$dump =~
+		  s/^ALTER OPERATOR FAMILY public\.part_test_text_ops USING hash ADD\n\s+FUNCTION 2 \(text, text\) public\.part_hashtext_length\(text,bigint\);//mg;
+	}
+
+	# pre-v12 dumps will not say anything about default_table_access_method.
+	if ($old_version lt 'REL_12_STABLE')
+	{
+		$dump =~ s/^SET default_table_access_method = heap;\n//mg;
+	}
+
+	# Suppress blank lines, as some places in pg_dump emit more or fewer.
+	$dump =~ s/\n\n+/\n/g;
+
+	return $dump;
+}
+
+=pod
+
+=back
+
+=cut
+
+1;
diff -pudr client-code-REL_15.orig/PGBuild/Modules/TestUpgradeXversion.pm client-code-REL_15/PGBuild/Modules/TestUpgradeXversion.pm
--- client-code-REL_15.orig/PGBuild/Modules/TestUpgradeXversion.pm	2022-12-31 09:15:03.000000000 -0500
+++ client-code-REL_15/PGBuild/Modules/TestUpgradeXversion.pm	2023-01-13 19:24:27.113794437 -0500
@@ -92,6 +92,21 @@ sub run_psql    ## no critic (Subroutine
 	return;     # callers can check $?
 }
 
+# Exported method for AdjustUpgrade.pm
+sub old_psql
+{
+	my ($self, $dbname, $sql_command) = @_;
+
+	my $this_branch  = $self->{this_branch};
+	my $other_branch = $self->{other_branch};
+	my $upgrade_loc  = "$self->{upgrade_install_root}/$this_branch";
+	my $oversion     = basename $other_branch;
+
+	run_psql("$other_branch/inst/bin/psql", "-e -v ON_ERROR_STOP=1",
+		$sql_command, $dbname, "$upgrade_loc/$oversion-fix.log", 1);
+	return;    # callers can check $?
+}
+
 sub get_lock
 {
 	my $self      = shift;
@@ -323,31 +338,6 @@ sub save_for_testing
 		return if $?;
 	}
 
-	if ($this_branch ne 'HEAD' && $this_branch le 'REL9_4_STABLE')
-	{
-		my $opsql = 'drop operator if exists public.=> (bigint, NONE)';
-
-		# syntax is illegal in 9.5 and later, and it shouldn't
-		# be possible for it to exist there anyway.
-		# quoting the operator can also fail,  so it's left unquoted.
-		run_psql("$installdir/bin/psql", "-e", $opsql, "regression",
-			"$upgrade_loc/fix.log", 1);
-		return if $?;
-	}
-
-	# remove dbs of modules known to cause pg_upgrade to fail
-	# anything not builtin and incompatible should clean up its own db
-	# e.g. jsonb_set_lax
-
-	foreach my $bad_module ("test_ddl_deparse")
-	{
-		my $dsql = "drop database if exists contrib_regression_$bad_module";
-
-		run_psql("$installdir/bin/psql", "-e", $dsql,
-			"postgres", "$upgrade_loc/fix.log", 1);
-		return if $?;
-	}
-
 	# use a different logfile here to get around windows sharing issue
 	system( qq{"$installdir/bin/pg_ctl" -D "$installdir/data-C" -w stop }
 		  . qq{>> "$upgrade_loc/ctl2.log" 2>&1});
@@ -375,6 +365,16 @@ sub test_upgrade    ## no critic (Subrou
 	print time_str(), "checking upgrade from $oversion to $this_branch ...\n"
 	  if $verbose;
 
+	# save paths to be accessed in old_psql
+	$self->{this_branch}  = $this_branch;
+	$self->{other_branch} = $other_branch;
+
+	# load helper module from source tree
+	unshift(@INC, "$self->{pgsql}/src/test/perl");
+	require PostgreSQL::Test::AdjustUpgrade;
+	PostgreSQL::Test::AdjustUpgrade->import;
+	shift(@INC);
+
 	rmtree "$other_branch/inst/$upgrade_test";
 	copydir(
 		"$other_branch/inst/data-C",
@@ -425,178 +425,8 @@ sub test_upgrade    ## no critic (Subrou
 	do { s/\r$//; $dbnames{$_} = 1; }
 	  foreach @dbnames;
 
-	if ($this_branch gt 'REL9_6_STABLE' || $this_branch eq 'HEAD')
-	{
-		run_psql(
-			"$other_branch/inst/bin/psql",                         "-e",
-			"drop database if exists contrib_regression_tsearch2", "postgres",
-			"$upgrade_loc/$oversion-copy.log",                     1
-		);
-		return if $?;
-
-		run_psql(
-			"$other_branch/inst/bin/psql",
-			"-e",
-			"drop function if exists oldstyle_length(integer, text)",
-			"regression",
-			"$upgrade_loc/$oversion-copy.log",
-			1
-		);
-		return if $?;
-	}
-
-	# some regression functions gone from release 11 on
-	if (   ($this_branch ge 'REL_11_STABLE' || $this_branch eq 'HEAD')
-		&& ($oversion lt 'REL_11_STABLE' && $oversion ne 'HEAD'))
-	{
-		my $missing_funcs = q{drop function if exists public.boxarea(box);
-                              drop function if exists public.funny_dup17();
-                            };
-		$missing_funcs =~ s/\n//g;
-
-		run_psql("$other_branch/inst/bin/psql", "-e", $missing_funcs,
-			"regression", "$upgrade_loc/$oversion-copy.log", 1);
-		return if $?;
-	}
-
-	# avoid version number issues with test_ext7
-	if ($dbnames{contrib_regression_test_extensions})
-	{
-		my $noext7 = "drop extension if exists test_ext7";
-		run_psql(
-			"$other_branch/inst/bin/psql", "-e", $noext7,
-			"contrib_regression_test_extensions",
-			"$upgrade_loc/$oversion-copy.log", 1
-		);
-		return if $?;
-	}
-
-	# user table OIDS and abstime+friends are gone from release 12 on
-	if (   ($this_branch gt 'REL_11_STABLE' || $this_branch eq 'HEAD')
-		&& ($oversion le 'REL_11_STABLE' && $oversion ne 'HEAD'))
-	{
-		my $nooid_stmt = q{
-           DO $stmt$
-           DECLARE
-              rec text;
-           BEGIN
-              FOR rec in
-                 select oid::regclass::text
-                 from pg_class
-                 where relname !~ '^pg_'
-                    and relhasoids
-                    and relkind in ('r','m')
-                 order by 1
-              LOOP
-                 execute 'ALTER TABLE ' || rec || ' SET WITHOUT OIDS';
-                 RAISE NOTICE 'removing oids from table %', rec;
-              END LOOP;
-           END; $stmt$;
-        };
-		foreach my $oiddb ("regression", "contrib_regression_btree_gist")
-		{
-			next unless $dbnames{$oiddb};
-			run_psql("$other_branch/inst/bin/psql", "-e", $nooid_stmt,
-				"$oiddb", "$upgrade_loc/$oversion-copy.log", 1);
-			return if $?;
-		}
-
-		if (   $oversion ge 'REL_10_STABLE'
-			&& $dbnames{'contrib_regression_postgres_fdw'})
-		{
-			run_psql(
-				"$other_branch/inst/bin/psql",
-				"-e",
-				"drop foreign table if exists ft_pg_type",
-				"contrib_regression_postgres_fdw",
-				"$upgrade_loc/$oversion-copy.log",
-				1
-			);
-			return if $?;
-		}
-
-		if ($oversion lt 'REL9_3_STABLE')
-		{
-			run_psql(
-				"$other_branch/inst/bin/psql",
-				"-e",
-				"drop table if exists abstime_tbl, reltime_tbl, tinterval_tbl",
-				"regression",
-				"$upgrade_loc/$oversion-copy.log",
-				1
-			);
-			return if $?;
-		}
-	}
-
-	# stuff not supported from release 14
-	if (   ($this_branch gt 'REL_13_STABLE' || $this_branch eq 'HEAD')
-		&& ($oversion le 'REL_13_STABLE' && $oversion ne 'HEAD'))
-	{
-		my $prstmt = join(';',
-			'drop operator if exists #@# (bigint,NONE)',
-			'drop operator if exists #%# (bigint,NONE)',
-			'drop operator if exists !=- (bigint,NONE)',
-			'drop operator if exists #@%# (bigint,NONE)');
-
-		run_psql("$other_branch/inst/bin/psql", "-e", $prstmt,
-			"regression", "$upgrade_loc/$oversion-copy.log", 1);
-		return if $?;
-
-		$prstmt = "drop function if exists public.putenv(text)";
-
-		my $regrdb =
-		  $oversion le "REL9_4_STABLE"
-		  ? "contrib_regression"
-		  : "contrib_regression_dblink";
-
-		if ($dbnames{$regrdb})
-		{
-			run_psql("$other_branch/inst/bin/psql", "-e", $prstmt,
-				"$regrdb", "$upgrade_loc/$oversion-copy.log", 1);
-			return if $?;
-		}
-
-		if ($oversion le 'REL9_4_STABLE')
-		{
-			# this is fixed in 9.5 and later
-			$prstmt = join(';',
-				'drop operator @#@ (NONE, bigint)',
-				'CREATE OPERATOR @#@ ('
-				  . 'PROCEDURE = factorial, '
-				  . 'RIGHTARG = bigint )');
-			run_psql("$other_branch/inst/bin/psql", "-e", $prstmt,
-				"regression", "$upgrade_loc/$oversion-copy.log", 1);
-			return if $?;
-		}
-
-		if ($oversion le 'REL9_4_STABLE')
-		{
-			# this is fixed in 9.5 and later
-			$prstmt = join(';',
-				'drop aggregate if exists public.array_cat_accum(anyarray)',
-				'CREATE AGGREGATE array_larger_accum (anyarray) ' . ' ( '
-				  . '   sfunc = array_larger, '
-				  . '   stype = anyarray, '
-				  . '   initcond = $${}$$ '
-				  . '  ) ');
-			run_psql("$other_branch/inst/bin/psql", "-e", $prstmt,
-				"regression", "$upgrade_loc/$oversion-copy.log", 1);
-			return if $?;
-		}
-	}
-
-	# can't upgrade aclitem in user tables from pre 16 to 16+
-	if (   ($this_branch gt 'REL_15_STABLE' || $this_branch eq 'HEAD')
-		&& ($oversion le 'REL_15_STABLE' && $oversion ne 'HEAD'))
-	{
-		my $prstmt = "alter table if exists public.tab_core_types
-                      drop column if exists aclitem";
-
-		run_psql("$other_branch/inst/bin/psql", "-e", $prstmt,
-			"regression", "$upgrade_loc/$oversion-copy.log", 1);
-		return if $?;
-	}
+	adjust_database_contents($oversion, $self, %dbnames);
+	return if $?;
 
 	my $extra_digits = "";
 
@@ -786,28 +616,27 @@ sub test_upgrade    ## no critic (Subrou
 		return if $?;
 	}
 
-	foreach my $dump ("$upgrade_loc/origin-$oversion.sql",
-		"$upgrade_loc/converted-$oversion-to-$this_branch.sql")
-	{
-		# Change trigger definitions to say ... EXECUTE FUNCTION ...
+	my $olddumpfile = "$upgrade_loc/origin-$oversion.sql";
+	my $dump        = file_contents($olddumpfile);
 
-		my $contents = file_contents($dump);
+	$dump = adjust_old_dumpfile($oversion, $dump);
 
-		# would like to use lookbehind here but perl complains
-		# so do it this way
-		$contents =~ s/
-                         (^CREATE\sTRIGGER\s.*?)
-                         \sEXECUTE\sPROCEDURE
-                      /$1 EXECUTE FUNCTION/mgx;
-		open(my $dh, '>', "$dump.fixed") || die "opening $dump.fixed";
-		print $dh $contents;
-		close($dh);
-	}
+	open(my $odh, '>', "$olddumpfile.fixed")
+	  || die "opening $olddumpfile.fixed: $!";
+	print $odh $dump;
+	close($odh);
 
-	system( qq{diff -I "^\$" -I "SET default_table_access_method = heap;" }
-		  . qq{ -I "^SET default_toast_compression = 'pglz';\$" -I "^-- " }
-		  . qq{-u "$upgrade_loc/origin-$oversion.sql.fixed" }
-		  . qq{"$upgrade_loc/converted-$oversion-to-$this_branch.sql.fixed" }
+	my $newdumpfile = "$upgrade_loc/converted-$oversion-to-$this_branch.sql";
+	$dump = file_contents($newdumpfile);
+
+	$dump = adjust_new_dumpfile($oversion, $dump);
+
+	open(my $ndh, '>', "$newdumpfile.fixed")
+	  || die "opening $newdumpfile.fixed: $!";
+	print $ndh $dump;
+	close($ndh);
+
+	system( qq{diff -u "$olddumpfile.fixed" "$newdumpfile.fixed" }
 		  . qq{> "$upgrade_loc/dumpdiff-$oversion" 2>&1});
 
 	# diff exits with status 1 if files differ
@@ -822,22 +651,7 @@ sub test_upgrade    ## no critic (Subrou
 	}
 	close($diffile);
 
-	# If the versions match we require that there be no diff lines.
-	# In the past we have seen a handful of diffs from reordering of
-	# large object output, but that appears to have disppeared.
-	# If the versions don't match we heuristically allow more lines of diffs
-	# based on observed differences. For versions from 9.6 on, that's
-	# not very many lines, though.
-
-	if (
-		($oversion eq $this_branch && $difflines == 0)
-		|| (   $oversion ne $this_branch
-			&& $oversion ge 'REL9_6_STABLE'
-			&& $difflines < 90)
-		|| (   $oversion ne $this_branch
-			&& $oversion lt 'REL9_6_STABLE'
-			&& $difflines < 700)
-	  )
+	if ($difflines == 0)
 	{
 		return 1;
 	}

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 14 Jan 2023 09:49:12 -0500

> [1] https://www.postgresql.org/message-id/951602.1673535249%40sss.pgh.pa.us

OK, we've been on parallel tracks (sorry about that). Let's run with
yours, as it covers more ground.

One thing I would change is that your adjust_database_contents tries to
make the adjustments rather than passing back a set of statements. We
could make that work, although your attempt won't really work for the
buildfarm, but I would just make actually performing the adjustments the
client's responsibility. That would make for much less disturbance in
the buildfarm code.

I also tried to remove a lot of the ugly release tag processing,
leveraging our PostgreSQL::Version gadget. I think that's worthwhile too.

cheers

andrew

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

---

From: Tom Lane <[email-masked]>
Date: Sat, 14 Jan 2023 10:47:35 -0500

Andrew Dunstan <[email-masked]> writes:
>> client, that create a working POC for this approach.

> yours, as it covers more ground.

Cool.

> make the adjustments rather than passing back a set of statements.

Agreed.  I'd thought maybe adjust_database_contents would need to
actually interact with the target DB; but experience so far says
that IF EXISTS conditionality is sufficient, so we can just build
a static list of statements to issue.  It's definitely a simpler
API that way.

> leveraging our PostgreSQL::Version gadget. I think that's worthwhile too.

OK, I'll take a look at that and make a new draft.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Sat, 14 Jan 2023 15:06:06 -0500

> OK, I'll take a look at that and make a new draft.

Here's version 2, incorporating your suggestions and with some
further work to make it handle 9.2 fully.  I think this could
be committable so far as HEAD is concerned, though I still
need to make versions of AdjustUpgrade.pm for the back branches.

I tried to use this to replace upgrade_adapt.sql, but failed so
far because I couldn't figure out exactly how you're supposed
to use 002_pg_upgrade.pl with an old source installation.
It's not terribly well documented.  In any case I think we
need a bit more thought about that, because it looks like
002_pg_upgrade.pl thinks that you can supply any random dump
file to serve as the initial state of the old installation;
but neither what I have here nor any likely contents of
upgrade_adapt.sql or the "custom filter" rules are going to
work on databases that aren't just the standard regression
database(s) of the old version.

I assume we should plan on reverting 9814ff550 (Add custom filtering
rules to the TAP tests of pg_upgrade)?  Does that have any
plausible use that's not superseded by this patchset?

			regards, tom lane

diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
new file mode 100644
index 0000000000..622f649b05
--- /dev/null
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -0,0 +1,500 @@
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+=pod
+
+=head1 NAME
+
+PostgreSQL::Test::AdjustUpgrade - helper module for cross-version upgrade tests
+
+=head1 SYNOPSIS
+
+  use PostgreSQL::Test::AdjustUpgrade;
+
+  # Build commands to adjust contents of old-version database before dumping
+  $statements = adjust_database_contents($old_version, %dbnames);
+
+  # Adjust contents of old pg_dumpall output file to match newer version
+  $dump = adjust_old_dumpfile($old_version, $dump);
+
+  # Adjust contents of new pg_dumpall output file to match older version
+  $dump = adjust_new_dumpfile($old_version, $dump);
+
+=head1 DESCRIPTION
+
+C<PostgreSQL::Test::AdjustUpgrade> encapsulates various hacks needed to
+compare the results of cross-version upgrade tests.
+
+=cut
+
+package PostgreSQL::Test::AdjustUpgrade;
+
+use strict;
+use warnings;
+
+use Exporter 'import';
+use PostgreSQL::Version;
+
+our @EXPORT = qw(
+  adjust_database_contents
+  adjust_old_dumpfile
+  adjust_new_dumpfile
+);
+
+=pod
+
+=head1 ROUTINES
+
+=over
+
+=item $statements = adjust_database_contents($old_version, %dbnames)
+
+Generate SQL commands to perform any changes to an old-version installation
+that are needed before we can pg_upgrade it into the current PostgreSQL
+version.
+
+Typically this involves dropping or adjusting no-longer-supported objects.
+
+Arguments:
+
+=over
+
+=item C<old_version>: Branch we are upgrading from.  This can be a branch
+name such as 'HEAD' or 'REL_11_STABLE', but it can also be any string
+that PostgreSQL::Version accepts.
+
+=item C<dbnames>: Hash of database names present in the old installation.
+
+=back
+
+Returns a reference to a hash, wherein the keys are database names and the
+values are arrayrefs to lists of statements to be run in those databases.
+
+=cut
+
+sub adjust_database_contents
+{
+	my ($old_version, %dbnames) = @_;
+	my $result = {};
+
+	# nothing to do for non-cross-version tests
+	return $result if $old_version eq 'HEAD';
+
+	# convert branch name to numeric form
+	$old_version =~ s/REL_?(\d+(?:_\d+)?)_STABLE/$1/;
+	$old_version =~ s/_/./;
+	$old_version = PostgreSQL::Version->new($old_version);
+
+	# remove dbs of modules known to cause pg_upgrade to fail
+	# anything not builtin and incompatible should clean up its own db
+	foreach my $bad_module ('test_ddl_deparse', 'tsearch2')
+	{
+		if ($dbnames{"contrib_regression_$bad_module"})
+		{
+			_add_st($result, 'postgres',
+				"drop database contrib_regression_$bad_module");
+			delete($dbnames{"contrib_regression_$bad_module"});
+		}
+	}
+
+	# avoid version number issues with test_ext7
+	if ($dbnames{contrib_regression_test_extensions})
+	{
+		_add_st(
+			$result,
+			'contrib_regression_test_extensions',
+			'drop extension if exists test_ext7');
+	}
+
+	# stuff not supported from release 16
+	if ($old_version >= 12 && $old_version < 16)
+	{
+		# Can't upgrade aclitem in user tables from pre 16 to 16+.
+		_add_st($result, 'regression',
+			'alter table public.tab_core_types drop column aclitem');
+		# Can't handle child tables with locally-generated columns.
+		_add_st(
+			$result, 'regression',
+			'drop table public.gtest_normal_child',
+			'drop table public.gtest_normal_child2');
+	}
+
+	# stuff not supported from release 14
+	if ($old_version < 14)
+	{
+		# postfix operators (some don't exist in very old versions)
+		_add_st(
+			$result,
+			'regression',
+			'drop operator #@# (bigint,NONE)',
+			'drop operator #%# (bigint,NONE)',
+			'drop operator if exists !=- (bigint,NONE)',
+			'drop operator if exists #@%# (bigint,NONE)');
+
+		# get rid of dblink's dependencies on regress.so
+		my $regrdb =
+		  $old_version le '9.4'
+		  ? 'contrib_regression'
+		  : 'contrib_regression_dblink';
+
+		if ($dbnames{$regrdb})
+		{
+			_add_st(
+				$result, $regrdb,
+				'drop function if exists public.putenv(text)',
+				'drop function if exists public.wait_pid(integer)');
+		}
+	}
+
+	# user table OIDs are gone from release 12 on
+	if ($old_version < 12)
+	{
+		my $nooid_stmt = q{
+           DO $stmt$
+           DECLARE
+              rec text;
+           BEGIN
+              FOR rec in
+                 select oid::regclass::text
+                 from pg_class
+                 where relname !~ '^pg_'
+                    and relhasoids
+                    and relkind in ('r','m')
+                 order by 1
+              LOOP
+                 execute 'ALTER TABLE ' || rec || ' SET WITHOUT OIDS';
+                 RAISE NOTICE 'removing oids from table %', rec;
+              END LOOP;
+           END; $stmt$;
+        };
+
+		foreach my $oiddb ('regression', 'contrib_regression_btree_gist')
+		{
+			next unless $dbnames{$oiddb};
+			_add_st($result, $oiddb, $nooid_stmt);
+		}
+
+		# this table had OIDs too, but we'll just drop it
+		if ($old_version >= 10 && $dbnames{'contrib_regression_postgres_fdw'})
+		{
+			_add_st(
+				$result,
+				'contrib_regression_postgres_fdw',
+				'drop foreign table ft_pg_type');
+		}
+	}
+
+	# abstime+friends are gone from release 12 on; but these tables
+	# might or might not be present depending on regression test vintage
+	if ($old_version < 12)
+	{
+		_add_st($result, 'regression',
+			'drop table if exists abstime_tbl, reltime_tbl, tinterval_tbl');
+	}
+
+	# some regression functions gone from release 11 on
+	if ($old_version < 11)
+	{
+		_add_st(
+			$result, 'regression',
+			'drop function if exists public.boxarea(box)',
+			'drop function if exists public.funny_dup17()');
+	}
+
+	# version-0 C functions are no longer supported
+	if ($old_version < 10)
+	{
+		_add_st($result, 'regression',
+			'drop function oldstyle_length(integer, text)');
+	}
+
+	if ($old_version lt '9.5')
+	{
+		# cope with changes of underlying functions
+		_add_st(
+			$result,
+			'regression',
+			'drop operator @#@ (NONE, bigint)',
+			'CREATE OPERATOR @#@ ('
+			  . 'PROCEDURE = factorial, RIGHTARG = bigint )',
+			'drop aggregate public.array_cat_accum(anyarray)',
+			'CREATE AGGREGATE array_larger_accum (anyarray) ' . ' ( '
+			  . '   sfunc = array_larger, '
+			  . '   stype = anyarray, '
+			  . '   initcond = $${}$$ ' . '  ) ');
+
+		# "=>" is no longer valid as an operator name
+		_add_st($result, 'regression',
+			'drop operator if exists public.=> (bigint, NONE)');
+	}
+
+	return $result;
+}
+
+# Internal subroutine to add statement(s) to the list for the given db.
+sub _add_st
+{
+	my ($result, $db, @st) = @_;
+
+	$result->{$db} ||= [];
+	push(@{ $result->{$db} }, @st);
+}
+
+=pod
+
+=item adjust_old_dumpfile($old_version, $dump)
+
+Edit a dump output file, taken from the adjusted old-version installation
+by current-version C<pg_dumpall -s>, so that it will match the results of
+C<pg_dumpall -s> on the pg_upgrade'd installation.
+
+Typically this involves coping with cosmetic differences in the output
+of backend subroutines used by pg_dump.
+
+Arguments:
+
+=over
+
+=item C<old_version>: Branch we are upgrading from.  This can be a branch
+name such as 'HEAD' or 'REL_11_STABLE', but it can also be any string
+that PostgreSQL::Version accepts.
+
+=item C<dump>: Contents of dump file
+
+=back
+
+Returns the modified dump text.
+
+=cut
+
+sub adjust_old_dumpfile
+{
+	my ($old_version, $dump) = @_;
+
+	# nothing to do for non-cross-version tests
+	return $dump if $old_version eq 'HEAD';
+
+	# convert branch name to numeric form
+	$old_version =~ s/REL_?(\d+(?:_\d+)?)_STABLE/$1/;
+	$old_version =~ s/_/./;
+	$old_version = PostgreSQL::Version->new($old_version);
+
+	# Version comments will certainly not match.
+	$dump =~ s/^-- Dumped from database version.*\n//mg;
+
+	if ($old_version >= 14 && $old_version < 16)
+	{
+		# Fix up some privilege-set discrepancies.
+		$dump =~
+		  s/^REVOKE SELECT,INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLE/REVOKE ALL ON TABLE/mg;
+		$dump =~
+		  s/^GRANT SELECT,INSERT,REFERENCES,TRIGGER,TRUNCATE,UPDATE ON TABLE/GRANT SELECT,INSERT,REFERENCES,TRIGGER,TRUNCATE,MAINTAIN,UPDATE ON TABLE/mg;
+	}
+
+	if ($old_version < 14)
+	{
+		# Remove mentions of extended hash functions.
+		$dump =~
+		  s/^(\s+OPERATOR 1 =\(integer,integer\)) ,\n\s+FUNCTION 2 \(integer, integer\) public\.part_hashint4_noop\(integer,bigint\);/$1;/mg;
+		$dump =~
+		  s/^(\s+OPERATOR 1 =\(text,text\)) ,\n\s+FUNCTION 2 \(text, text\) public\.part_hashtext_length\(text,bigint\);/$1;/mg;
+	}
+
+	# Change trigger definitions to say ... EXECUTE FUNCTION ...
+	if ($old_version < 12)
+	{
+		# would like to use lookbehind here but perl complains
+		# so do it this way
+		$dump =~ s/
+			(^CREATE\sTRIGGER\s.*?)
+			\sEXECUTE\sPROCEDURE
+			/$1 EXECUTE FUNCTION/mgx;
+	}
+
+	if ($old_version lt '9.6')
+	{
+		# adjust some places where we don't print so many parens anymore
+		$dump =~
+		  s/^'New York'\tnew & york \| big & apple \| nyc\t'new' & 'york'\t\( 'new' & 'york' \| 'big' & 'appl' \) \| 'nyc'/'New York'\tnew & york | big & apple | nyc\t'new' & 'york'\t'new' & 'york' | 'big' & 'appl' | 'nyc'/mg;
+		$dump =~
+		  s/^'Sanct Peter'\tPeterburg \| peter \| 'Sanct Peterburg'\t'sanct' & 'peter'\t\( 'peterburg' \| 'peter' \) \| 'sanct' & 'peterburg'/'Sanct Peter'\tPeterburg | peter | 'Sanct Peterburg'\t'sanct' & 'peter'\t'peterburg' | 'peter' | 'sanct' & 'peterburg'/mg;
+	}
+
+	if ($old_version lt '9.5')
+	{
+		# adjust some places where we don't print so many parens anymore
+		$dump =~
+		  s/CONSTRAINT sequence_con CHECK \(\(\(\(x > 3\) AND \(y <> 'check failed'::text\)\) AND \(z < 8\)\)\)/CONSTRAINT sequence_con CHECK (((x > 3) AND (y <> 'check failed'::text) AND (z < 8)))/mg;
+		$dump =~
+		  s/CONSTRAINT copy_con CHECK \(\(\(\(x > 3\) AND \(y <> 'check failed'::text\)\) AND \(x < 7\)\)\)/CONSTRAINT copy_con CHECK (((x > 3) AND (y <> 'check failed'::text) AND (x < 7)))/mg;
+		$dump =~
+		  s/CONSTRAINT insert_con CHECK \(\(\(\(x >= 3\) AND \(y <> 'check failed'::text\)\) AND \(x < 8\)\)\)/CONSTRAINT insert_con CHECK (((x >= 3) AND (y <> 'check failed'::text) AND (x < 8)))/mg;
+		$dump =~
+		  s/DEFAULT \(\(-1\) \* currval\('public\.insert_seq'::regclass\)\)/DEFAULT ('-1'::integer * currval('public.insert_seq'::regclass))/mg;
+		$dump =~
+		  s/WHERE \(\(\(rsl\.sl_color = rsh\.slcolor\) AND \(rsl\.sl_len_cm >= rsh\.slminlen_cm\)\) AND \(rsl\.sl_len_cm <= rsh\.slmaxlen_cm\)\)/WHERE ((rsl.sl_color = rsh.slcolor) AND (rsl.sl_len_cm >= rsh.slminlen_cm) AND (rsl.sl_len_cm <= rsh.slmaxlen_cm))/mg;
+		$dump =~
+		  s/WHERE \(\(\(rule_and_refint_t3\.id3a = new\.id3a\) AND \(rule_and_refint_t3\.id3b = new\.id3b\)\) AND \(rule_and_refint_t3\.id3c = new\.id3c\)\)/WHERE ((rule_and_refint_t3.id3a = new.id3a) AND (rule_and_refint_t3.id3b = new.id3b) AND (rule_and_refint_t3.id3c = new.id3c))/mg;
+		$dump =~
+		  s/WHERE \(\(\(rule_and_refint_t3_1\.id3a = new\.id3a\) AND \(rule_and_refint_t3_1\.id3b = new\.id3b\)\) AND \(rule_and_refint_t3_1\.id3c = new\.id3c\)\)/WHERE ((rule_and_refint_t3_1.id3a = new.id3a) AND (rule_and_refint_t3_1.id3b = new.id3b) AND (rule_and_refint_t3_1.id3c = new.id3c))/mg;
+	}
+
+	if ($old_version lt '9.3')
+	{
+		# CREATE VIEW/RULE statements were not pretty-printed before 9.3.
+		# To cope, reduce all whitespace sequences within them to one space.
+		# This must be done on both old and new dumps.
+		$dump = _mash_view_whitespace($dump);
+
+		# _mash_view_whitespace doesn't handle multi-command rules;
+		# rather than trying to fix that, just hack the exceptions manually.
+		$dump =~
+		  s/CREATE RULE rtest_sys_del AS ON DELETE TO public\.rtest_system DO \(DELETE FROM public\.rtest_interface WHERE \(rtest_interface\.sysname = old\.sysname\); DELETE FROM public\.rtest_admin WHERE \(rtest_admin\.sysname = old\.sysname\); \);/CREATE RULE rtest_sys_del AS ON DELETE TO public.rtest_system DO (DELETE FROM public.rtest_interface WHERE (rtest_interface.sysname = old.sysname);\n DELETE FROM public.rtest_admin\n  WHERE (rtest_admin.sysname = old.sysname);\n);/m;
+		$dump =~
+		  s/CREATE RULE rtest_sys_upd AS ON UPDATE TO public\.rtest_system DO \(UPDATE public\.rtest_interface SET sysname = new\.sysname WHERE \(rtest_interface\.sysname = old\.sysname\); UPDATE public\.rtest_admin SET sysname = new\.sysname WHERE \(rtest_admin\.sysname = old\.sysname\); \);/CREATE RULE rtest_sys_upd AS ON UPDATE TO public.rtest_system DO (UPDATE public.rtest_interface SET sysname = new.sysname WHERE (rtest_interface.sysname = old.sysname);\n UPDATE public.rtest_admin SET sysname = new.sysname\n  WHERE (rtest_admin.sysname = old.sysname);\n);/m;
+
+		# and there's one place where pre-9.3 uses more aliases than we do now
+		$dump =~
+		  s/CREATE RULE rule_and_refint_t3_ins AS ON INSERT TO public\.rule_and_refint_t3 WHERE \(EXISTS \(SELECT 1 FROM public\.rule_and_refint_t3 WHERE \(\(rule_and_refint_t3\.id3a = new\.id3a\) AND \(rule_and_refint_t3\.id3b = new\.id3b\) AND \(rule_and_refint_t3\.id3c = new\.id3c\)\)\)\) DO INSTEAD UPDATE public\.rule_and_refint_t3 SET data = new\.data WHERE \(\(rule_and_refint_t3\.id3a = new\.id3a\) AND \(rule_and_refint_t3\.id3b = new\.id3b\) AND \(rule_and_refint_t3\.id3c = new\.id3c\)\);/CREATE RULE rule_and_refint_t3_ins AS ON INSERT TO public.rule_and_refint_t3 WHERE (EXISTS (SELECT 1 FROM public.rule_and_refint_t3 rule_and_refint_t3_1 WHERE ((rule_and_refint_t3_1.id3a = new.id3a) AND (rule_and_refint_t3_1.id3b = new.id3b) AND (rule_and_refint_t3_1.id3c = new.id3c)))) DO INSTEAD UPDATE public.rule_and_refint_t3 SET data = new.data WHERE ((rule_and_refint_t3.id3a = new.id3a) AND (rule_and_refint_t3.id3b = new.id3b) AND (rule_and_refint_t3.id3c = new.id3c));/m;
+
+		# Also fix old use of NATURAL JOIN syntax
+		$dump =~
+		  s/NATURAL JOIN public\.credit_card r/JOIN public.credit_card r USING (cid)/mg;
+		$dump =~
+		  s/NATURAL JOIN public\.credit_usage r/JOIN public.credit_usage r USING (cid)/mg;
+	}
+
+	# Suppress blank lines, as some places in pg_dump emit more or fewer.
+	$dump =~ s/\n\n+/\n/g;
+
+	return $dump;
+}
+
+# Internal subroutine to mangle whitespace within view/rule commands.
+# Any consecutive sequence of whitespace is reduced to one space.
+sub _mash_view_whitespace
+{
+	my ($dump) = @_;
+
+	foreach my $leader ('CREATE VIEW', 'CREATE RULE')
+	{
+		my @splitchunks = split $leader, $dump;
+
+		$dump = shift(@splitchunks);
+		foreach my $chunk (@splitchunks)
+		{
+			my @thischunks = split /;/, $chunk, 2;
+			my $stmt = shift(@thischunks);
+
+			# now $stmt is just the body of the CREATE VIEW/RULE
+			$stmt =~ s/\s+/ /sg;
+			# we also need to smash these forms for sub-selects and rules
+			$stmt =~ s/\( SELECT/(SELECT/g;
+			$stmt =~ s/\( INSERT/(INSERT/g;
+			$stmt =~ s/\( UPDATE/(UPDATE/g;
+			$stmt =~ s/\( DELETE/(DELETE/g;
+
+			$dump .= $leader . $stmt . ';' . $thischunks[0];
+		}
+	}
+	return $dump;
+}
+
+=pod
+
+=item adjust_new_dumpfile($old_version, $dump)
+
+Edit a dump output file, taken from the pg_upgrade'd installation
+by current-version C<pg_dumpall -s>, so that it will match the old
+dump output file as adjusted by C<adjust_old_dumpfile>.
+
+Typically this involves deleting data not present in the old installation.
+
+Arguments:
+
+=over
+
+=item C<old_version>: Branch we are upgrading from.  This can be a branch
+name such as 'HEAD' or 'REL_11_STABLE', but it can also be any string
+that PostgreSQL::Version accepts.
+
+=item C<dump>: Contents of dump file
+
+=back
+
+Returns the modified dump text.
+
+=cut
+
+sub adjust_new_dumpfile
+{
+	my ($old_version, $dump) = @_;
+
+	# nothing to do for non-cross-version tests
+	return $dump if $old_version eq 'HEAD';
+
+	# convert branch name to numeric form
+	$old_version =~ s/REL_?(\d+(?:_\d+)?)_STABLE/$1/;
+	$old_version =~ s/_/./;
+	$old_version = PostgreSQL::Version->new($old_version);
+
+	# Version comments will certainly not match.
+	$dump =~ s/^-- Dumped from database version.*\n//mg;
+
+	if ($old_version < 14)
+	{
+		# Suppress noise-word uses of IN in CREATE/ALTER PROCEDURE.
+		$dump =~ s/^(CREATE PROCEDURE .*?)\(IN /$1(/mg;
+		$dump =~ s/^(ALTER PROCEDURE .*?)\(IN /$1(/mg;
+		$dump =~ s/^(CREATE PROCEDURE .*?), IN /$1, /mg;
+		$dump =~ s/^(ALTER PROCEDURE .*?), IN /$1, /mg;
+		$dump =~ s/^(CREATE PROCEDURE .*?), IN /$1, /mg;
+		$dump =~ s/^(ALTER PROCEDURE .*?), IN /$1, /mg;
+
+		# Remove SUBSCRIPT clauses in CREATE TYPE.
+		$dump =~ s/^\s+SUBSCRIPT = raw_array_subscript_handler,\n//mg;
+
+		# Remove multirange_type_name clauses in CREATE TYPE AS RANGE.
+		$dump =~ s/,\n\s+multirange_type_name = .*?(,?)$/$1/mg;
+
+		# Remove mentions of extended hash functions.
+		$dump =~
+		  s/^ALTER OPERATOR FAMILY public\.part_test_int4_ops USING hash ADD\n\s+FUNCTION 2 \(integer, integer\) public\.part_hashint4_noop\(integer,bigint\);//mg;
+		$dump =~
+		  s/^ALTER OPERATOR FAMILY public\.part_test_text_ops USING hash ADD\n\s+FUNCTION 2 \(text, text\) public\.part_hashtext_length\(text,bigint\);//mg;
+	}
+
+	# pre-v12 dumps will not say anything about default_table_access_method.
+	if ($old_version < 12)
+	{
+		$dump =~ s/^SET default_table_access_method = heap;\n//mg;
+	}
+
+	# dumps from pre-9.6 dblink may include redundant ACL settings
+	if ($old_version lt '9.6')
+	{
+		$dump =~
+		  s/^--\n-- Name: FUNCTION dblink_connect_u\(text\); Type: ACL; Schema: public; Owner: .*\n--\n+REVOKE ALL ON FUNCTION public\.dblink_connect_u\(text\) FROM PUBLIC;\n+--\n-- Name: FUNCTION dblink_connect_u\(text, text\); Type: ACL; Schema: public; Owner: .*\n--\n+REVOKE ALL ON FUNCTION public\.dblink_connect_u\(text, text\) FROM PUBLIC;\n//mg;
+	}
+
+	if ($old_version lt '9.3')
+	{
+		# CREATE VIEW/RULE statements were not pretty-printed before 9.3.
+		# To cope, reduce all whitespace sequences within them to one space.
+		# This must be done on both old and new dumps.
+		$dump = _mash_view_whitespace($dump);
+	}
+
+	# Suppress blank lines, as some places in pg_dump emit more or fewer.
+	$dump =~ s/\n\n+/\n/g;
+
+	return $dump;
+}
+
+=pod
+
+=back
+
+=cut
+
+1;
diff -pudr client-code-REL_16.orig/PGBuild/Modules/TestUpgradeXversion.pm client-code-REL_16/PGBuild/Modules/TestUpgradeXversion.pm
--- client-code-REL_16.orig/PGBuild/Modules/TestUpgradeXversion.pm	2023-01-13 12:20:51.000000000 -0500
+++ client-code-REL_16/PGBuild/Modules/TestUpgradeXversion.pm	2023-01-14 14:23:27.120834037 -0500
@@ -323,31 +323,6 @@ sub save_for_testing
 		return if $?;
 	}
 
@@ -375,6 +350,12 @@ sub test_upgrade    ## no critic (Subrou
 
@@ -414,6 +395,7 @@ sub test_upgrade    ## no critic (Subrou
 
 	return if $?;
 
+	# collect names of databases present in old installation.
 	my $sql = 'select datname from pg_database';
 
 	run_psql("psql", "-A -t", $sql, "postgres",
@@ -425,186 +407,19 @@ sub test_upgrade    ## no critic (Subrou
 	do { s/\r$//; $dbnames{$_} = 1; }
 	  foreach @dbnames;
 
+	# obtain and execute commands needed to make old database upgradable.
+	my $adjust_cmds = adjust_database_contents($oversion, %dbnames);
 
-	# stuff not supported from release 16
-	if (   ($this_branch gt 'REL_15_STABLE' || $this_branch eq 'HEAD')
-		&& ($oversion le 'REL_15_STABLE' && $oversion ne 'HEAD'))
+	foreach my $updb (keys %$adjust_cmds)
 	{
-		# Can't upgrade aclitem in user tables from pre 16 to 16+.
-		# Also can't handle child tables with newly-generated columns.
-		my $prstmt = join(
-			';',
-			'alter table if exists public.tab_core_types
-						  drop column if exists aclitem',
-			'drop table if exists public.gtest_normal_child',
-			'drop table if exists public.gtest_normal_child2'
-		);
+		my $upcmds = join(";\n", @{ $adjust_cmds->{$updb} });
 
-		run_psql("$other_branch/inst/bin/psql", "-e", $prstmt,
-			"regression", "$upgrade_loc/$oversion-copy.log", 1);
+		run_psql("$other_branch/inst/bin/psql", "-e -v ON_ERROR_STOP=1",
+			$upcmds, $updb, "$upgrade_loc/$oversion-fix.log", 1);
 		return if $?;
 	}
 
+	# perform a dump from the old database for comparison purposes.
 	my $extra_digits = "";
 
 	if (   $oversion ne 'HEAD'
@@ -793,28 +608,27 @@ sub test_upgrade    ## no critic (Subrou
 		return if $?;
 	}

 

 	# diff exits with status 1 if files differ
@@ -829,22 +643,7 @@ sub test_upgrade    ## no critic (Subrou
 	}
 	close($diffile);

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 15 Jan 2023 08:21:21 -0500

> need to make versions of AdjustUpgrade.pm for the back branches.

This looks pretty good to me.

I'll probably change this line

   my $adjust_cmds = adjust_database_contents($oversion, %dbnames);

so it's only called if the old and new versions are different. Is there
any case where a repo shouldn't be upgradeable to its own version
without adjustment?

cheers

andrew