Обсуждение: Extracting cross-version-upgrade knowledge from buildfarm client
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,UPDATEON 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'/'NewYork'\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\)\)\)/CONSTRAINTsequence_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_conCHECK (((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\)\)\)/CONSTRAINTinsert_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;
}
On 2023-01-13 Fr 19:48, Tom Lane wrote: > 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 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
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-13 Fr 19:48, Tom Lane wrote:
>> Attached are two patches, one for PG git and one for the buildfarm
>> client, that create a working POC for this approach.
> OK, we've been on parallel tracks (sorry about that). Let's run with
> yours, as it covers more ground.
Cool.
> One thing I would change is that your adjust_database_contents tries to
> 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.
> I also tried to remove a lot of the ugly release tag processing,
> 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
I wrote:
> 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,UPDATEON 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'/'NewYork'\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\)\)\)/CONSTRAINTsequence_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_conCHECK (((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\)\)\)/CONSTRAINTinsert_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\.sysnameWHERE \(rtest_admin\.sysname = old\.sysname\); \);/CREATE RULE rtest_sys_upd AS ON UPDATE TO
public.rtest_systemDO (UPDATE public.rtest_interface SET sysname = new.sysname WHERE (rtest_interface.sysname =
old.sysname);\nUPDATE 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
FROMpublic\.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\.dataWHERE \(\(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_t3SET 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
FUNCTIONpublic\.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 $?;
}
- 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 +350,12 @@ sub test_upgrade ## no critic (Subrou
print time_str(), "checking upgrade from $oversion to $this_branch ...\n"
if $verbose;
+ # 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",
@@ -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;
- 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 $?;
- }
- }
+ # 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 $?;
}
- 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
@@ -829,22 +643,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;
}
On 2023-01-14 Sa 15:06, Tom Lane wrote: > I wrote: >> 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. 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 -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-14 Sa 15:06, Tom Lane wrote:
>> Here's version 2, incorporating your suggestions and with some
>> further work to make it handle 9.2 fully.
> This looks pretty good to me.
Great! I'll work on making back-branch versions.
> 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?
Makes sense. I'd keep the check for $oversion eq 'HEAD' in the
subroutines, but that's mostly just to protect the version
conversion code below it.
Another thing I was just thinking about was not bothering to run
"diff" if the fixed dump strings are equal in-memory. You could
take that even further and not write out the fixed files at all,
but that seems like a bad idea for debuggability of the adjustment
subroutines. However, I don't see why we need to write an
empty diff file, nor parse it.
One other question before I continue --- do the adjustment
subroutines need to worry about Windows newlines in the strings?
It's not clear to me whether Perl will automatically make "\n"
in a pattern match "\r\n", or whether it's not a problem because
something upstream will have stripped \r's.
regards, tom lane
On Sat, Jan 14, 2023 at 03:06:06PM -0500, Tom Lane wrote: > 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. As in pg_upgrade's TESTING or the comments of the tests? > 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. Yeah, this code needs an extra push that I have not been able to figure out yet, as we could recommend the creation of a dump with installcheck-world and USE_MODULE_DB=1. Perhaps a module is just better at the end. > 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? Nope, this could just be removed if we finish by adding a module that does exactly the same work. If you are planning on committing the module you have, i'd be happy to take care of a revert for this part. + # 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'); Could you just use a DO block here to detect tables with such attributes, like in upgrade_adapt.sql, rather than dropping the table from the core tests? That's more consistent with the treatment of WITH OIDS. Is this module pluggable with 002_pg_upgrade.pl? -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes:
> On Sat, Jan 14, 2023 at 03:06:06PM -0500, Tom Lane wrote:
> + # 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');
> Could you just use a DO block here to detect tables with such
> attributes, like in upgrade_adapt.sql, rather than dropping the table
> from the core tests? That's more consistent with the treatment of
> WITH OIDS.
I guess, but it seems like make-work as long as there's just the one
column.
> Is this module pluggable with 002_pg_upgrade.pl?
I did find that 002_pg_upgrade.pl could load it. I got stuck at
the point of trying to test things, because I didn't understand
what the test process is supposed to be for an upgrade from a
back branch. For some reason I thought that 002_pg_upgrade.pl
could automatically create the old regression database, but
now I see that's not implemented.
regards, tom lane
On 2023-01-15 Su 11:01, Tom Lane wrote: > Another thing I was just thinking about was not bothering to run > "diff" if the fixed dump strings are equal in-memory. You could > take that even further and not write out the fixed files at all, > but that seems like a bad idea for debuggability of the adjustment > subroutines. However, I don't see why we need to write an > empty diff file, nor parse it. Yeah, that makes sense. > One other question before I continue --- do the adjustment > subroutines need to worry about Windows newlines in the strings? > It's not clear to me whether Perl will automatically make "\n" > in a pattern match "\r\n", or whether it's not a problem because > something upstream will have stripped \r's. > > I don't think we need to worry about them, but I will have a closer look. Those replacement lines are very difficult to read. I think use of extended regexes and some multi-part replacements would help. I'll have a go at that tomorrow. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> Those replacement lines are very difficult to read. I think use of
> extended regexes and some multi-part replacements would help. I'll have
> a go at that tomorrow.
Yeah, after I wrote that code I remembered about \Q ... \E, which would
eliminate the need for most of the backslashes and probably make things
better that way. I didn't get around to improving it yet though, so
feel free to have a go.
regards, tom lane
On Sun, Jan 15, 2023 at 06:02:07PM -0500, Tom Lane wrote: > I guess, but it seems like make-work as long as there's just the one > column. Well, the query is already written, so I would use that, FWIW. > I did find that 002_pg_upgrade.pl could load it. I got stuck at > the point of trying to test things, because I didn't understand > what the test process is supposed to be for an upgrade from a > back branch. For some reason I thought that 002_pg_upgrade.pl > could automatically create the old regression database, but > now I see that's not implemented. test.sh did that, until I noticed that we need to worry about pg_regress from the past branches to be compatible in the script itself because we need to run it in the old source tree. This makes the whole much more complicated to maintain, especially with the recent removal of input/ and output/ folders in the regression tests :/ -- Michael
Вложения
On 2023-01-15 Su 18:37, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Those replacement lines are very difficult to read. I think use of >> extended regexes and some multi-part replacements would help. I'll have >> a go at that tomorrow. > Yeah, after I wrote that code I remembered about \Q ... \E, which would > eliminate the need for most of the backslashes and probably make things > better that way. I didn't get around to improving it yet though, so > feel free to have a go. > > OK, here's my version. It tests clean against all of crake's dump files back to 9.2. To some extent it's a matter of taste, but I hate very long regex lines - it makes it very hard to see what's actually changing, so I broke up most of those. Given that we are looking at newlines in some places I decided that after all it was important to convert CRLF to LF. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Вложения
Andrew Dunstan <andrew@dunslane.net> writes:
> OK, here's my version. It tests clean against all of crake's dump files
> back to 9.2.
> To some extent it's a matter of taste, but I hate very long regex lines
> - it makes it very hard to see what's actually changing, so I broke up
> most of those.
I don't mind breaking things up, but I'm not terribly excited about
making the patterns looser, as you've done in some places like
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;
+ $dump =~ s {(^\s+OPERATOR\s1\s=\((?:integer,integer|text,text)\))\s,\n
+ \s+FUNCTION\s2\s.*?public.part_hash.*?;}
+ {$1;}mxg;
}
I don't think that's any easier to read, and it risks masking
diffs that we'd wish to know about.
regards, tom lane
On 2023-01-16 Mo 14:34, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> OK, here's my version. It tests clean against all of crake's dump files
>> back to 9.2.
>> To some extent it's a matter of taste, but I hate very long regex lines
>> - it makes it very hard to see what's actually changing, so I broke up
>> most of those.
> I don't mind breaking things up, but I'm not terribly excited about
> making the patterns looser, as you've done in some places like
>
> 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;
> + $dump =~ s {(^\s+OPERATOR\s1\s=\((?:integer,integer|text,text)\))\s,\n
> + \s+FUNCTION\s2\s.*?public.part_hash.*?;}
> + {$1;}mxg;
> }
>
> I don't think that's any easier to read, and it risks masking
> diffs that we'd wish to know about.
OK, I'll make another pass and tighten things up.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-16 Mo 14:34, Tom Lane wrote:
>> I don't think that's any easier to read, and it risks masking
>> diffs that we'd wish to know about.
> OK, I'll make another pass and tighten things up.
Don't sweat it, I'm just working the bugs out of a new version.
I'll have something to post shortly, I hope.
regards, tom lane
OK, here's a v4:
* It works with 002_pg_upgrade.pl now. The only substantive change
I had to make for that was to define the $old_version arguments as
being always PostgreSQL::Version objects not strings, because
otherwise I got complaints like
Argument "HEAD" isn't numeric in numeric comparison (<=>) at
/home/postgres/pgsql/src/bin/pg_upgrade/../../../src/test/perl/PostgreSQL/Version.pmline 130.
So now TestUpgradeXversion.pm is responsible for performing that
conversion, and also for not doing any conversions on HEAD (which
Andrew wanted anyway).
* I improved pg_upgrade's TESTING directions after figuring out how
to get it to work for contrib modules.
* Incorporated (most of) Andrew's stylistic improvements.
* Simplified TestUpgradeXversion.pm's use of diff, as discussed.
I think we're about ready to go, except for cutting down
AdjustUpgrade.pm to make versions to put in the back branches.
I'm slightly tempted to back-patch 002_pg_upgrade.pl so that there
is an in-tree way to verify back-branch AdjustUpgrade.pm files.
On the other hand, it's hard to believe that testing that in
HEAD won't be sufficient; I doubt the back-branch copies will
need to change much.
regards, tom lane
diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING
index 98286231d7..81a4324a76 100644
--- a/src/bin/pg_upgrade/TESTING
+++ b/src/bin/pg_upgrade/TESTING
@@ -10,31 +10,14 @@ This will run the TAP tests to run pg_upgrade, performing an upgrade
from the version in this source tree to a new instance of the same
version.
-Testing an upgrade from a different version requires a dump to set up
-the contents of this instance, with its set of binaries. The following
-variables are available to control the test (see DETAILS below about
-the creation of the dump):
+Testing an upgrade from a different PG version is also possible, and
+provides a more thorough test that pg_upgrade does what it's meant for.
+This requires both a source tree and an installed tree for the old
+version, as well as a dump file to set up the instance to be upgraded.
+The following environment variables must be set to enable this testing:
export olddump=...somewhere/dump.sql (old version's dump)
export oldinstall=...otherversion/ (old version's install base path)
-
-"filter_rules" is a variable that can be used to specify a file with custom
-filtering rules applied before comparing the dumps of the PostgreSQL
-instances near the end of the tests, in the shape of regular expressions
-valid for perl. This is useful to enforce certain validation cases where
-pg_dump could create inconsistent outputs across major versions.
-For example:
-
- # Remove all CREATE POLICY statements
- s/^CREATE\sPOLICY.*//mgx
- # Replace REFRESH with DROP for materialized views
- s/^REFRESH\s(MATERIALIZED\sVIEW)/DROP $1/mgx
-
-Lines beginning with '#' and empty lines are ignored. One rule can be
-defined per line.
-
-Finally, the tests can be done by running
-
- make check
+See DETAILS below for more information about creation of the dump.
You can also test the different transfer modes (--copy, --link,
--clone) by setting the environment variable PG_TEST_PG_UPGRADE_MODE
@@ -52,22 +35,32 @@ The most effective way to test pg_upgrade, aside from testing on user
data, is by upgrading the PostgreSQL regression database.
This testing process first requires the creation of a valid regression
-database dump that can be then used for $olddump. Such files contain
+database dump that can then be used for $olddump. Such files contain
most database features and are specific to each major version of Postgres.
Here are the steps needed to create a dump file:
1) Create and populate the regression database in the old cluster.
This database can be created by running 'make installcheck' from
- src/test/regress using its source code tree.
+ src/test/regress in the old version's source code tree.
-2) Use pg_dumpall to dump out the contents of the instance, including the
- regression database, in the shape of a SQL file. This requires the *old*
- cluster's pg_dumpall so as the dump created is compatible with the
- version of the cluster it is dumped into.
+ If you like, you can also populate regression databases for one or
+ more contrib modules by running 'make installcheck USE_MODULE_DB=1'
+ in their directories. (USE_MODULE_DB is essential so that the
+ pg_upgrade test script will understand which database is which.)
-Once the dump is created, it can be repeatedly used with $olddump and
-`make check`, that automates the dump of the old database, its upgrade,
-the dump out of the new database and the comparison of the dumps between
-the old and new databases. The contents of the dumps can also be manually
-compared.
+2) Use pg_dumpall to dump out the contents of the instance, including the
+ regression database(s), into a SQL file. Use the *old* version's
+ pg_dumpall so that the dump created is compatible with that version.
+
+Once the dump file is created, it can be used repeatedly. Set $olddump
+to point to the dump file and run 'make check' or 'make installcheck'
+in the new version's src/bin/pg_upgrade directory. (If you included any
+contrib databases in the old dump, you must use 'make installcheck' and
+ensure that the corresponding contrib modules have been installed in
+the new version's installation tree.) This will build a temporary cluster
+using the old installation's executables, populate it from the dump file,
+and then try to pg_upgrade it to the new version. Success is reported
+if pg_dumpall output matches between the pre-upgrade and post-upgrade
+databases. In case of trouble, manually comparing those dump files may
+help to isolate the problem.
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index c066fd7d93..62a8fa9d8b 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -10,6 +10,7 @@ use File::Path qw(rmtree);
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::AdjustUpgrade;
use Test::More;
# Can be changed to test the other modes.
@@ -37,37 +38,16 @@ sub generate_db
# This returns the path to the filtered dump.
sub filter_dump
{
- my ($node, $dump_file) = @_;
+ my ($is_old, $old_version, $dump_file) = @_;
my $dump_contents = slurp_file($dump_file);
- # Remove the comments.
- $dump_contents =~ s/^\-\-.*//mgx;
- # Remove empty lines.
- $dump_contents =~ s/^\n//mgx;
-
- # Apply custom filtering rules, if any.
- if (defined($ENV{filter_rules}))
+ if ($is_old)
{
- my $filter_file = $ENV{filter_rules};
- die "no file with custom filter rules found!" unless -e $filter_file;
-
- open my $filter_handle, '<', $filter_file
- or die "could not open $filter_file";
- while (<$filter_handle>)
- {
- my $filter_line = $_;
-
- # Skip comments and empty lines
- next if ($filter_line =~ /^#/);
- next if ($filter_line =~ /^\s*$/);
-
- # Apply lines with filters.
- note "Applying custom rule $filter_line to $dump_file";
- my $filter = "\$dump_contents =~ $filter_line";
- ## no critic (ProhibitStringyEval)
- eval $filter;
- }
- close $filter_handle;
+ $dump_contents = adjust_old_dumpfile($old_version, $dump_contents);
+ }
+ else
+ {
+ $dump_contents = adjust_new_dumpfile($old_version, $dump_contents);
}
my $dump_file_filtered = "${dump_file}_filtered";
@@ -83,7 +63,7 @@ sub filter_dump
# 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
# after the upgrade. The upgrade test passes if there are no differences
-# in these two dumps.
+# (after filtering) in these two dumps.
# Testing upgrades with an older version of PostgreSQL requires setting up
# two environment variables, as of:
@@ -198,15 +178,29 @@ my $oldbindir = $oldnode->config_data('--bindir');
# only if different major versions are used for the dump.
if (defined($ENV{oldinstall}))
{
- # Note that upgrade_adapt.sql and psql from the new version are used,
- # to cope with an upgrade to this version.
- $newnode->command_ok(
- [
- 'psql', '-X',
- '-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
- '-d', $oldnode->connstr('regression'),
- ],
- 'ran adapt script');
+ # Consult AdjustUpgrade to find out what we need to do.
+ my $dbnames =
+ $oldnode->safe_psql('postgres', qq(SELECT datname FROM pg_database));
+ my %dbnames;
+ do { $dbnames{$_} = 1; }
+ foreach split /\s+/s, $dbnames;
+ my $adjust_cmds =
+ adjust_database_contents($oldnode->pg_version, %dbnames);
+
+ foreach my $updb (keys %$adjust_cmds)
+ {
+ my $upcmds = join(";\n", @{ $adjust_cmds->{$updb} });
+
+ # For simplicity, use the newer version's psql to issue the commands.
+ $newnode->command_ok(
+ [
+ 'psql', '-X',
+ '-v', 'ON_ERROR_STOP=1',
+ '-c', $upcmds,
+ '-d', $oldnode->connstr($updb),
+ ],
+ "ran version adaptation commands for database $updb");
+ }
}
# Take a dump before performing the upgrade as a base comparison. Note
@@ -359,8 +353,8 @@ my $dump1_filtered = $dump1_file;
my $dump2_filtered = $dump2_file;
if ($oldnode->pg_version != $newnode->pg_version)
{
- $dump1_filtered = filter_dump($oldnode, $dump1_file);
- $dump2_filtered = filter_dump($newnode, $dump2_file);
+ $dump1_filtered = filter_dump(1, $oldnode->pg_version, $dump1_file);
+ $dump2_filtered = filter_dump(0, $oldnode->pg_version, $dump2_file);
}
# Compare the two dumps, there should be no differences.
@@ -371,7 +365,7 @@ is($compare_res, 0, 'old and new dumps match after pg_upgrade');
if ($compare_res != 0)
{
my ($stdout, $stderr) =
- run_command([ 'diff', $dump1_filtered, $dump2_filtered ]);
+ run_command([ 'diff', '-u', $dump1_filtered, $dump2_filtered ]);
print "=== diff of $dump1_filtered and $dump2_filtered\n";
print "=== stdout ===\n";
print $stdout;
diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
new file mode 100644
index 0000000000..7b4a19be2a
--- /dev/null
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -0,0 +1,524 @@
+
+# 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, represented as a
+PostgreSQL::Version object.
+
+=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 = {};
+
+ # 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, represented as a
+PostgreSQL::Version object.
+
+=item C<dump>: Contents of dump file
+
+=back
+
+Returns the modified dump text.
+
+=cut
+
+sub adjust_old_dumpfile
+{
+ my ($old_version, $dump) = @_;
+
+ # use Unix newlines
+ $dump =~ s/\r\n/\n/g;
+
+ # 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}
+ {$1,MAINTAIN,UPDATE ON TABLE}mg;
+ }
+
+ if ($old_version < 14)
+ {
+ # Remove mentions of extended hash functions.
+ $dump =~ s {^(\s+OPERATOR\s1\s=\(integer,integer\))\s,\n
+ \s+FUNCTION\s2\s\(integer,\sinteger\)\spublic\.part_hashint4_noop\(integer,bigint\);}
+ {$1;}mxg;
+ $dump =~ s {^(\s+OPERATOR\s1\s=\(text,text\))\s,\n
+ \s+FUNCTION\s2\s\(text,\stext\)\spublic\.part_hashtext_length\(text,bigint\);}
+ {$1;}mxg;
+ }
+
+ # 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
+
+ my $prefix =
+ "'New York'\tnew & york | big & apple | nyc\t'new' & 'york'\t";
+ my $orig = "( 'new' & 'york' | 'big' & 'appl' ) | 'nyc'";
+ my $repl = "'new' & 'york' | 'big' & 'appl' | 'nyc'";
+ $dump =~ s/(?<=^\Q$prefix\E)\Q$orig\E/$repl/mg;
+
+ $prefix =
+ "'Sanct Peter'\tPeterburg | peter | 'Sanct Peterburg'\t'sanct' & 'peter'\t";
+ $orig = "( 'peterburg' | 'peter' ) | 'sanct' & 'peterburg'";
+ $repl = "'peterburg' | 'peter' | 'sanct' & 'peterburg'";
+ $dump =~ s/(?<=^\Q$prefix\E)\Q$orig\E/$repl/mg;
+ }
+
+ if ($old_version lt '9.5')
+ {
+ # adjust some places where we don't print so many parens anymore
+
+ my $prefix = "CONSTRAINT (?:sequence|copy)_con CHECK [(][(]";
+ my $orig = "((x > 3) AND (y <> 'check failed'::text))";
+ my $repl = "(x > 3) AND (y <> 'check failed'::text)";
+ $dump =~ s/($prefix)\Q$orig\E/$1$repl/mg;
+
+ $prefix = "CONSTRAINT insert_con CHECK [(][(]";
+ $orig = "((x >= 3) AND (y <> 'check failed'::text))";
+ $repl = "(x >= 3) AND (y <> 'check failed'::text)";
+ $dump =~ s/($prefix)\Q$orig\E/$1$repl/mg;
+
+ $orig = "DEFAULT ((-1) * currval('public.insert_seq'::regclass))";
+ $repl =
+ "DEFAULT ('-1'::integer * currval('public.insert_seq'::regclass))";
+ $dump =~ s/\Q$orig\E/$repl/mg;
+
+ my $expr =
+ "(rsl.sl_color = rsh.slcolor) AND (rsl.sl_len_cm >= rsh.slminlen_cm)";
+ $dump =~ s/WHERE \(\(\Q$expr\E\)/WHERE ($expr/g;
+
+ $expr =
+ "(rule_and_refint_t3.id3a = new.id3a) AND (rule_and_refint_t3.id3b = new.id3b)";
+ $dump =~ s/WHERE \(\(\Q$expr\E\)/WHERE ($expr/g;
+
+ $expr =
+ "(rule_and_refint_t3_1.id3a = new.id3a) AND (rule_and_refint_t3_1.id3b = new.id3b)";
+ $dump =~ s/WHERE \(\(\Q$expr\E\)/WHERE ($expr/g;
+ }
+
+ 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.
+
+ my $prefix =
+ "CREATE RULE rtest_sys_del AS ON DELETE TO public.rtest_system DO (DELETE FROM public.rtest_interface WHERE
(rtest_interface.sysname= old.sysname);";
+ my $line2 = " DELETE FROM public.rtest_admin";
+ my $line3 = " WHERE (rtest_admin.sysname = old.sysname);";
+ $dump =~
+ s/(?<=\Q$prefix\E)\Q$line2$line3\E \);/\n$line2\n $line3\n);/mg;
+
+ $prefix =
+ "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);";
+ $line2 = " UPDATE public.rtest_admin SET sysname = new.sysname";
+ $line3 = " WHERE (rtest_admin.sysname = old.sysname);";
+ $dump =~
+ s/(?<=\Q$prefix\E)\Q$line2$line3\E \);/\n$line2\n $line3\n);/mg;
+
+ # and there's one place where pre-9.3 uses a different table alias
+ $dump =~ s {^(CREATE\sRULE\srule_and_refint_t3_ins\sAS\s
+ ON\sINSERT\sTO\spublic\.rule_and_refint_t3\s
+ WHERE\s\(EXISTS\s\(SELECT\s1\sFROM\spublic\.rule_and_refint_t3)\s
+ (WHERE\s\(\(rule_and_refint_t3)
+ (\.id3a\s=\snew\.id3a\)\sAND\s\(rule_and_refint_t3)
+ (\.id3b\s=\snew\.id3b\)\sAND\s\(rule_and_refint_t3)}
+ {$1 rule_and_refint_t3_1 $2_1$3_1$4_1}mx;
+
+ # 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, represented as a
+PostgreSQL::Version object.
+
+=item C<dump>: Contents of dump file
+
+=back
+
+Returns the modified dump text.
+
+=cut
+
+sub adjust_new_dumpfile
+{
+ my ($old_version, $dump) = @_;
+
+ # use Unix newlines
+ $dump =~ s/\r\n/\n/g;
+
+ # 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\sOPERATOR\sFAMILY\spublic\.part_test_int4_ops\sUSING\shash\sADD\n
+ \s+FUNCTION\s2\s\(integer,\sinteger\)\spublic\.part_hashint4_noop\(integer,bigint\);} {}mxg;
+ $dump =~
+ s {^ALTER\sOPERATOR\sFAMILY\spublic\.part_test_text_ops\sUSING\shash\sADD\n
+ \s+FUNCTION\s2\s\(text,\stext\)\spublic\.part_hashtext_length\(text,bigint\);} {}mxg;
+ }
+
+ # 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')
+ {
+ my $comment =
+ "-- Name: FUNCTION dblink_connect_u\(.*?\); Type: ACL; Schema: public; Owner: .*";
+ my $sql =
+ "REVOKE ALL ON FUNCTION public\.dblink_connect_u\(.*?\) FROM PUBLIC;";
+ $dump =~ s/^--\n$comment\n--\n+$sql\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-16 15:01:22.502366802 -0500
@@ -323,31 +323,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 +350,21 @@ sub test_upgrade ## no critic (Subrou
print time_str(), "checking upgrade from $oversion to $this_branch ...\n"
if $verbose;
+ # load helper module from source tree
+ unshift(@INC, "$self->{pgsql}/src/test/perl");
+ require PostgreSQL::Test::AdjustUpgrade;
+ PostgreSQL::Test::AdjustUpgrade->import;
+ shift(@INC);
+
+ # if $oversion isn't HEAD, convert it into a PostgreSQL::Version object
+ my $old_version = $oversion;
+ if ($old_version ne 'HEAD')
+ {
+ $old_version =~ s/REL_?(\d+(?:_\d+)?)_STABLE/$1/;
+ $old_version =~ s/_/./;
+ $old_version = PostgreSQL::Version->new($old_version);
+ }
+
rmtree "$other_branch/inst/$upgrade_test";
copydir(
"$other_branch/inst/data-C",
@@ -414,6 +404,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 +416,22 @@ 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'))
+ if ($oversion ne $this_branch)
{
- 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 $?;
- }
+ # obtain and execute commands needed to make old database upgradable.
+ my $adjust_cmds = adjust_database_contents($old_version, %dbnames);
- if ($oversion le 'REL9_4_STABLE')
+ foreach my $updb (keys %$adjust_cmds)
{
- # 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 $?;
- }
+ my $upcmds = join(";\n", @{ $adjust_cmds->{$updb} });
- 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);
+ run_psql("$other_branch/inst/bin/psql", "-e -v ON_ERROR_STOP=1",
+ $upcmds, $updb, "$upgrade_loc/$oversion-fix.log", 1);
return if $?;
}
}
- # 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'))
- {
- # 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'
- );
-
- run_psql("$other_branch/inst/bin/psql", "-e", $prstmt,
- "regression", "$upgrade_loc/$oversion-copy.log", 1);
- return if $?;
- }
-
+ # perform a dump from the old database for comparison purposes.
my $extra_digits = "";
if ( $oversion ne 'HEAD'
@@ -793,65 +620,40 @@ 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 ...
+ # Slurp the pg_dump output files, and filter them if not same version.
+ my $olddumpfile = "$upgrade_loc/origin-$oversion.sql";
+ my $olddump = file_contents($olddumpfile);
- my $contents = file_contents($dump);
+ $olddump = adjust_old_dumpfile($old_version, $olddump)
+ if ($oversion ne $this_branch);
- # 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);
- }
+ my $newdumpfile = "$upgrade_loc/converted-$oversion-to-$this_branch.sql";
+ my $newdump = file_contents($newdumpfile);
- 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" }
- . qq{> "$upgrade_loc/dumpdiff-$oversion" 2>&1});
+ $newdump = adjust_new_dumpfile($old_version, $newdump)
+ if ($oversion ne $this_branch);
- # diff exits with status 1 if files differ
- return if $? >> 8 > 1;
+ # Always write out the filtered files, to aid in diagnosing filter bugs.
+ open(my $odh, '>', "$olddumpfile.fixed")
+ || die "opening $olddumpfile.fixed: $!";
+ print $odh $olddump;
+ close($odh);
+ open(my $ndh, '>', "$newdumpfile.fixed")
+ || die "opening $newdumpfile.fixed: $!";
+ print $ndh $newdump;
+ close($ndh);
- open(my $diffile, '<', "$upgrade_loc/dumpdiff-$oversion")
- || die "opening $upgrade_loc/dumpdiff-$oversion: $!";
- my $difflines = 0;
- while (<$diffile>)
+ # Are the results the same?
+ if ($olddump ne $newdump)
{
- $difflines++ if /^[+-]/;
- }
- 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.
+ # Trouble, so run diff to show the problem.
+ system( qq{diff -u "$olddumpfile.fixed" "$newdumpfile.fixed" }
+ . qq{> "$upgrade_loc/dumpdiff-$oversion" 2>&1});
- 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)
- )
- {
- return 1;
- }
- else
- {
return;
}
+
+ return 1;
}
sub installcheck
I wrote:
> I think we're about ready to go, except for cutting down
> AdjustUpgrade.pm to make versions to put in the back branches.
Hmmm ... so upon trying to test in the back branches, I soon
discovered that PostgreSQL/Version.pm isn't there before v15.
I don't see a good reason why we couldn't back-patch it, though.
Any objections?
regards, tom lane
On 2023-01-16 Mo 18:11, Tom Lane wrote: > I wrote: >> I think we're about ready to go, except for cutting down >> AdjustUpgrade.pm to make versions to put in the back branches. > Hmmm ... so upon trying to test in the back branches, I soon > discovered that PostgreSQL/Version.pm isn't there before v15. > > I don't see a good reason why we couldn't back-patch it, though. > Any objections? > > No, that seems perfectly reasonable. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
I've pushed the per-branch AdjustUpgrade.pm files and tested by performing
a fresh round of buildfarm runs with the patched TestUpgradeXversion.pm
file. I think we're in good shape with this project.
I dunno if we want to stretch buildfarm owners' patience with yet
another BF client release right now. On the other hand, I'm antsy
to see if we can un-revert 1b4d280ea after doing a little more
work in AdjustUpgrade.pm.
regards, tom lane
On Mon, Jan 16, 2023 at 04:48:28PM -0500, Tom Lane wrote: > I'm slightly tempted to back-patch 002_pg_upgrade.pl so that there > is an in-tree way to verify back-branch AdjustUpgrade.pm files. > On the other hand, it's hard to believe that testing that in > HEAD won't be sufficient; I doubt the back-branch copies will > need to change much. Backpatching 002_pg_upgrade.pl requires a bit more than the test: there is one compatibility gotcha as of dc57366. I did not backpatch it because nobody has complained about it until I found out about it, but the test would require it. By the way, thanks for your work on this stuff :) -- Michael
Вложения
On 2023-01-16 Mo 21:58, Tom Lane wrote: > I've pushed the per-branch AdjustUpgrade.pm files and tested by performing > a fresh round of buildfarm runs with the patched TestUpgradeXversion.pm > file. I think we're in good shape with this project. > > I dunno if we want to stretch buildfarm owners' patience with yet > another BF client release right now. On the other hand, I'm antsy > to see if we can un-revert 1b4d280ea after doing a little more > work in AdjustUpgrade.pm. > > It looks like the only animals doing the cross version tests crake, drongo and fairywren. These are all mine, so I don't think we need to do a new release for this. I think the next step is to push the buildfarm client changes, and update those three animals to use it, and make sure nothing breaks. I'll go and do those things now. Then you should be able to try your unrevert. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-16 Mo 21:58, Tom Lane wrote:
>> I dunno if we want to stretch buildfarm owners' patience with yet
>> another BF client release right now. On the other hand, I'm antsy
>> to see if we can un-revert 1b4d280ea after doing a little more
>> work in AdjustUpgrade.pm.
> It looks like the only animals doing the cross version tests crake,
> drongo and fairywren. These are all mine, so I don't think we need to do
> a new release for this.
copperhead, kittiwake, snapper, and tadarida were running them
until fairly recently.
regards, tom lane
On 2023-01-17 Tu 10:18, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 2023-01-16 Mo 21:58, Tom Lane wrote: >>> I dunno if we want to stretch buildfarm owners' patience with yet >>> another BF client release right now. On the other hand, I'm antsy >>> to see if we can un-revert 1b4d280ea after doing a little more >>> work in AdjustUpgrade.pm. >> It looks like the only animals doing the cross version tests crake, >> drongo and fairywren. These are all mine, so I don't think we need to do >> a new release for this. > copperhead, kittiwake, snapper, and tadarida were running them > until fairly recently. > > Ah, yes, true, I didn't look far enough back. The new file can be downloaded from <https://raw.githubusercontent.com/PGBuildFarm/client-code/75efff0fbd70ca89b097593824911ab6ccbd258f/PGBuild/Modules/TestUpgradeXversion.pm> - it's a dropin replacement. FYI crake has just passed the test with flying colours. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> FYI crake has just passed the test with flying colours.
Cool. I await the Windows machines' results with interest.
regards, tom lane
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-16 Mo 21:58, Tom Lane wrote:
>> I dunno if we want to stretch buildfarm owners' patience with yet
>> another BF client release right now. On the other hand, I'm antsy
>> to see if we can un-revert 1b4d280ea after doing a little more
>> work in AdjustUpgrade.pm.
> I think the next step is to push the buildfarm client changes, and
> update those three animals to use it, and make sure nothing breaks. I'll
> go and do those things now. Then you should be able to try your unrevert.
It looks like unrevert will require ~130 lines in AdjustUpgrade.pm,
which is not great but not awful either. I think this is ready to
go once you've vetted your remaining buildfarm animals.
regards, tom lane
diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
index 7cf4ced392..5bed1d6839 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -268,6 +268,12 @@ sub adjust_old_dumpfile
# Version comments will certainly not match.
$dump =~ s/^-- Dumped from database version.*\n//mg;
+ if ($old_version < 16)
+ {
+ # Fix up some view queries that no longer require table-qualification.
+ $dump = _mash_view_qualifiers($dump);
+ }
+
if ($old_version >= 14 && $old_version < 16)
{
# Fix up some privilege-set discrepancies.
@@ -396,6 +402,133 @@ sub adjust_old_dumpfile
return $dump;
}
+
+# Data for _mash_view_qualifiers
+my @_unused_view_qualifiers = (
+ # Present at least since 9.2
+ { obj => 'VIEW public.trigger_test_view', qual => 'trigger_test' },
+ { obj => 'VIEW public.domview', qual => 'domtab' },
+ { obj => 'VIEW public.my_property_normal', qual => 'customer' },
+ { obj => 'VIEW public.my_property_secure', qual => 'customer' },
+ { obj => 'VIEW public.pfield_v1', qual => 'pf' },
+ { obj => 'VIEW public.rtest_v1', qual => 'rtest_t1' },
+ { obj => 'VIEW public.rtest_vview1', qual => 'x' },
+ { obj => 'VIEW public.rtest_vview2', qual => 'rtest_view1' },
+ { obj => 'VIEW public.rtest_vview3', qual => 'x' },
+ { obj => 'VIEW public.rtest_vview5', qual => 'rtest_view1' },
+ { obj => 'VIEW public.shoelace_obsolete', qual => 'shoelace' },
+ { obj => 'VIEW public.shoelace_candelete', qual => 'shoelace_obsolete' },
+ { obj => 'VIEW public.toyemp', qual => 'emp' },
+ { obj => 'VIEW public.xmlview4', qual => 'emp' },
+ # Since 9.3 (some of these were removed in 9.6)
+ { obj => 'VIEW public.tv', qual => 't' },
+ { obj => 'MATERIALIZED VIEW mvschema.tvm', qual => 'tv' },
+ { obj => 'VIEW public.tvv', qual => 'tv' },
+ { obj => 'MATERIALIZED VIEW public.tvvm', qual => 'tvv' },
+ { obj => 'VIEW public.tvvmv', qual => 'tvvm' },
+ { obj => 'MATERIALIZED VIEW public.bb', qual => 'tvvmv' },
+ { obj => 'VIEW public.nums', qual => 'nums' },
+ { obj => 'VIEW public.sums_1_100', qual => 't' },
+ { obj => 'MATERIALIZED VIEW public.tm', qual => 't' },
+ { obj => 'MATERIALIZED VIEW public.tmm', qual => 'tm' },
+ { obj => 'MATERIALIZED VIEW public.tvmm', qual => 'tvm' },
+ # Since 9.4
+ {
+ obj => 'MATERIALIZED VIEW public.citext_matview',
+ qual => 'citext_table'
+ },
+ {
+ obj => 'OR REPLACE VIEW public.key_dependent_view',
+ qual => 'view_base_table'
+ },
+ {
+ obj => 'OR REPLACE VIEW public.key_dependent_view_no_cols',
+ qual => 'view_base_table'
+ },
+ # Since 9.5
+ {
+ obj => 'VIEW public.dummy_seclabel_view1',
+ qual => 'dummy_seclabel_tbl2'
+ },
+ { obj => 'VIEW public.vv', qual => 'test_tablesample' },
+ { obj => 'VIEW public.test_tablesample_v1', qual => 'test_tablesample' },
+ { obj => 'VIEW public.test_tablesample_v2', qual => 'test_tablesample' },
+ # Since 9.6
+ {
+ obj => 'MATERIALIZED VIEW public.test_pg_dump_mv1',
+ qual => 'test_pg_dump_t1'
+ },
+ { obj => 'VIEW public.test_pg_dump_v1', qual => 'test_pg_dump_t1' },
+ { obj => 'VIEW public.mvtest_tv', qual => 'mvtest_t' },
+ {
+ obj => 'MATERIALIZED VIEW mvtest_mvschema.mvtest_tvm',
+ qual => 'mvtest_tv'
+ },
+ { obj => 'VIEW public.mvtest_tvv', qual => 'mvtest_tv' },
+ { obj => 'MATERIALIZED VIEW public.mvtest_tvvm', qual => 'mvtest_tvv' },
+ { obj => 'VIEW public.mvtest_tvvmv', qual => 'mvtest_tvvm' },
+ { obj => 'MATERIALIZED VIEW public.mvtest_bb', qual => 'mvtest_tvvmv' },
+ { obj => 'MATERIALIZED VIEW public.mvtest_tm', qual => 'mvtest_t' },
+ { obj => 'MATERIALIZED VIEW public.mvtest_tmm', qual => 'mvtest_tm' },
+ { obj => 'MATERIALIZED VIEW public.mvtest_tvmm', qual => 'mvtest_tvm' },
+ # Since 10 (some removed in 12)
+ { obj => 'VIEW public.itestv10', qual => 'itest10' },
+ { obj => 'VIEW public.itestv11', qual => 'itest11' },
+ { obj => 'VIEW public.xmltableview2', qual => '"xmltable"' },
+ # Since 12
+ {
+ obj => 'MATERIALIZED VIEW public.tableam_tblmv_heap2',
+ qual => 'tableam_tbl_heap2'
+ },
+ # Since 13
+ { obj => 'VIEW public.limit_thousand_v_1', qual => 'onek' },
+ { obj => 'VIEW public.limit_thousand_v_2', qual => 'onek' },
+ { obj => 'VIEW public.limit_thousand_v_3', qual => 'onek' },
+ { obj => 'VIEW public.limit_thousand_v_4', qual => 'onek' });
+
+# Internal subroutine to remove no-longer-used table qualifiers from
+# CREATE [MATERIALIZED] VIEW commands. See list of targeted views above.
+sub _mash_view_qualifiers
+{
+ my ($dump) = @_;
+
+ for my $uvq (@_unused_view_qualifiers)
+ {
+ my $leader = "CREATE $uvq->{obj} ";
+ my $qualifier = $uvq->{qual};
+ # Note: we loop because there are presently some cases where the same
+ # view name appears in multiple databases. Fortunately, the same
+ # qualifier removal applies or is harmless for each instance ... but
+ # we might want to rename some things to avoid assuming that.
+ my @splitchunks = split $leader, $dump;
+ $dump = shift(@splitchunks);
+ foreach my $chunk (@splitchunks)
+ {
+ my @thischunks = split /;/, $chunk, 2;
+ my $stmt = shift(@thischunks);
+ my $ostmt = $stmt;
+
+ # now $stmt is just the body of the CREATE [MATERIALIZED] VIEW
+ $stmt =~ s/$qualifier\.//g;
+
+ $dump .= $leader . $stmt . ';' . $thischunks[0];
+ }
+ }
+
+ # Further hack a few cases where not all occurrences of the qualifier
+ # should be removed.
+ $dump =~ s {^(CREATE VIEW public\.rtest_vview1 .*?)(a\)\)\);)}
+ {$1x.$2}ms;
+ $dump =~ s {^(CREATE VIEW public\.rtest_vview3 .*?)(a\)\)\);)}
+ {$1x.$2}ms;
+ $dump =~
+ s {^(CREATE VIEW public\.shoelace_obsolete .*?)(sl_color\)\)\)\);)}
+ {$1shoelace.$2}ms;
+
+ 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
On 2023-01-17 Tu 11:30, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> FYI crake has just passed the test with flying colours. > Cool. I await the Windows machines' results with interest. fairwren and drongo are clean except for fairywren upgrading 9.6 to 11. This appears to be a longstanding issue that the fuzz processing was causing us to ignore. See for example <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2022-09-01%2018%3A27%3A28&stg=xversion-upgrade-REL_10_STABLE-REL_11_STABLE> It's somewhat interesting that this doesn't appear to be an issue with the MSVC builds on drongo. And it disappears when upgrading to release 12 or later where we use the extra-float-digits=0 hack. I propose to add this to just the release 11 AdjustUpgrade.pm: # float4 values in this table on Msys can have precision differences # in representation between old and new versions if ($old_version < 10 && $dbnames{contrib_regression_btree_gist} && $^O eq 'msys') { _add_st($result, 'contrib_regression_btree_gist', 'drop table if exists float4tmp'); } cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes: > fairwren and drongo are clean except for fairywren upgrading 9.6 to 11. > This appears to be a longstanding issue that the fuzz processing was > causing us to ignore. See for example > <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2022-09-01%2018%3A27%3A28&stg=xversion-upgrade-REL_10_STABLE-REL_11_STABLE> Interesting. I suspected that removing the fuzz allowance would teach us some things we hadn't known about. > I propose to add this to just the release 11 AdjustUpgrade.pm: > # float4 values in this table on Msys can have precision differences > # in representation between old and new versions > if ($old_version < 10 && $dbnames{contrib_regression_btree_gist} && > $^O eq 'msys') > { > _add_st($result, 'contrib_regression_btree_gist', > 'drop table if exists float4tmp'); > } Seems reasonable (but I wonder if you don't need "$old_version < 11"). A nicer answer would be to apply --extra-float-digits=0 across the board, but pre-v12 pg_dump lacks that switch. regards, tom lane
One more thing before we move on from this topic. I'd been testing
modified versions of the AdjustUpgrade.pm logic by building from a
--from-source source tree, which seemed way easier than dealing
with a private git repo. As it stands, TestUpgradeXversion.pm
refuses to run under $from_source, but I just diked that check out
and it seemed to work fine for my purposes. Now, that's going to be
a regular need going forward, so I'd like to not need a hacked version
of the BF client code to do it.
Also, your committed version of TestUpgradeXversion.pm breaks that
use-case because you did
- unshift(@INC, "$self->{pgsql}/src/test/perl");
+ unshift(@INC, "$self->{buildroot}/$this_branch/pgsql/src/test/perl");
which AFAICS is an empty directory in a $from_source run.
I suppose that the reason for not running under $from_source is to
avoid corrupting the saved installations with unofficial versions.
However, couldn't we skip the "save" step and still run the upgrade
tests against whatever we have saved? (Maybe skip the same-version
test, as it's not quite reflecting any real case then.)
Here's a quick draft patch showing what I have in mind. There may
well be a better way to deal with the wheres-the-source issue than
what is in hunk 2. Also, I didn't reindent the unchanged code in
sub installcheck, and I didn't add anything about skipping
same-version tests.
regards, tom lane
diff --git a/PGBuild/Modules/TestUpgradeXversion.pm b/PGBuild/Modules/TestUpgradeXversion.pm
index a784686..408432d 100644
--- a/PGBuild/Modules/TestUpgradeXversion.pm
+++ b/PGBuild/Modules/TestUpgradeXversion.pm
@@ -51,8 +51,6 @@ sub setup
my $conf = shift; # ref to the whole config object
my $pgsql = shift; # postgres build dir
- return if $from_source;
-
return if $branch !~ /^(?:HEAD|REL_?\d+(?:_\d+)?_STABLE)$/;
my $animal = $conf->{animal};
@@ -351,7 +349,16 @@ sub test_upgrade ## no critic (Subroutines::ProhibitManyArgs)
if $verbose;
# load helper module from source tree
- unshift(@INC, "$self->{buildroot}/$this_branch/pgsql/src/test/perl");
+ my $source_tree;
+ if ($from_source)
+ {
+ $source_tree = $self->{pgsql};
+ }
+ else
+ {
+ $source_tree = "$self->{buildroot}/$this_branch/pgsql";
+ }
+ unshift(@INC, "$source_tree/src/test/perl");
require PostgreSQL::Test::AdjustUpgrade;
PostgreSQL::Test::AdjustUpgrade->import;
shift(@INC);
@@ -694,6 +701,11 @@ sub installcheck
my $upgrade_loc = "$upgrade_install_root/$this_branch";
my $installdir = "$upgrade_loc/inst";
+ # Don't save in a $from_source build: we want the saved trees always
+ # to correspond to branch tips of the animal's standard repo. We can
+ # perform upgrade tests against previously-saved trees, though.
+ if (!$from_source)
+ {
# for saving we need an exclusive lock.
get_lock($self, $this_branch, 1);
@@ -716,6 +728,7 @@ sub installcheck
if ($verbose > 1);
send_result('XversionUpgradeSave', $status, \@saveout) if $status;
$steps_completed .= " XVersionUpgradeSave";
+ }
# in saveonly mode our work is done
return if $ENV{PG_UPGRADE_SAVE_ONLY};
@@ -744,7 +757,7 @@ sub installcheck
# other branch from being removed or changed under us.
get_lock($self, $oversion, 0);
- $status =
+ my $status =
test_upgrade($self, $save_env, $this_branch, $upgrade_install_root,
$dport, $install_loc, $other_branch) ? 0 : 1;
On 2023-01-18 We 14:32, Tom Lane wrote:
> One more thing before we move on from this topic. I'd been testing
> modified versions of the AdjustUpgrade.pm logic by building from a
> --from-source source tree, which seemed way easier than dealing
> with a private git repo. As it stands, TestUpgradeXversion.pm
> refuses to run under $from_source, but I just diked that check out
> and it seemed to work fine for my purposes. Now, that's going to be
> a regular need going forward, so I'd like to not need a hacked version
> of the BF client code to do it.
>
> Also, your committed version of TestUpgradeXversion.pm breaks that
> use-case because you did
>
> - unshift(@INC, "$self->{pgsql}/src/test/perl");
> + unshift(@INC, "$self->{buildroot}/$this_branch/pgsql/src/test/perl");
>
> which AFAICS is an empty directory in a $from_source run.
>
> I suppose that the reason for not running under $from_source is to
> avoid corrupting the saved installations with unofficial versions.
> However, couldn't we skip the "save" step and still run the upgrade
> tests against whatever we have saved? (Maybe skip the same-version
> test, as it's not quite reflecting any real case then.)
>
> Here's a quick draft patch showing what I have in mind. There may
> well be a better way to deal with the wheres-the-source issue than
> what is in hunk 2. Also, I didn't reindent the unchanged code in
> sub installcheck, and I didn't add anything about skipping
> same-version tests.
No that won't work if we're using vpath builds (which was why I changed
it from what you had). $self->{pgsql} is always the build directory.
Something like this should do it:
my $source_tree = $from_source || "$self->{buildroot}/$this_branch/pgsql";
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-18 We 14:32, Tom Lane wrote:
>> I suppose that the reason for not running under $from_source is to
>> avoid corrupting the saved installations with unofficial versions.
>> However, couldn't we skip the "save" step and still run the upgrade
>> tests against whatever we have saved? (Maybe skip the same-version
>> test, as it's not quite reflecting any real case then.)
> Something like this should do it:
> my $source_tree = $from_source || "$self->{buildroot}/$this_branch/pgsql";
Ah, I didn't understand that $from_source is a path not just a bool.
What do you think about the above questions? Is this $from_source
exclusion for the reason I guessed, or some other one?
regards, tom lane
On 2023-01-18 We 16:14, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2023-01-18 We 14:32, Tom Lane wrote:
>>> I suppose that the reason for not running under $from_source is to
>>> avoid corrupting the saved installations with unofficial versions.
>>> However, couldn't we skip the "save" step and still run the upgrade
>>> tests against whatever we have saved? (Maybe skip the same-version
>>> test, as it's not quite reflecting any real case then.)
>> Something like this should do it:
>> my $source_tree = $from_source || "$self->{buildroot}/$this_branch/pgsql";
> Ah, I didn't understand that $from_source is a path not just a bool.
>
> What do you think about the above questions? Is this $from_source
> exclusion for the reason I guessed, or some other one?
>
>
Yes, the reason is that, unlike almost everything else in the buildfarm,
cross version upgrade testing requires saved state (binaries and data
directory), and we don't want from-source builds corrupting that state.
I think we can do what you want but it's a bit harder than what you've
done. If we're not going to save the current run's product then we need
to run the upgrade test from a different directory (probably directly in
"$buildroot/$this_branch/inst"). Otherwise we'll be testing upgrade to
the saved product of a previous run of this branch. I'll take a stab at
it tomorrow if you like.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> I think we can do what you want but it's a bit harder than what you've
> done. If we're not going to save the current run's product then we need
> to run the upgrade test from a different directory (probably directly in
> "$buildroot/$this_branch/inst"). Otherwise we'll be testing upgrade to
> the saved product of a previous run of this branch.
Hmm, maybe that explains some inconsistent results I remember getting.
> I'll take a stab at it tomorrow if you like.
Please do.
regards, tom lane
On 2023-01-18 We 17:14, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> I think we can do what you want but it's a bit harder than what you've >> done. If we're not going to save the current run's product then we need >> to run the upgrade test from a different directory (probably directly in >> "$buildroot/$this_branch/inst"). Otherwise we'll be testing upgrade to >> the saved product of a previous run of this branch. > Hmm, maybe that explains some inconsistent results I remember getting. > >> I'll take a stab at it tomorrow if you like. > Please do. > > See <https://github.com/PGBuildFarm/client-code/commit/9415e1bd415e8c12ad009296eefc4c609ed9f533> I tested it and it seems to be doing the right thing. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes: > See > <https://github.com/PGBuildFarm/client-code/commit/9415e1bd415e8c12ad009296eefc4c609ed9f533> > I tested it and it seems to be doing the right thing. Yeah, seems to do what I want. Thanks! regards, tom lane
On 2023-01-18 We 10:33, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> fairwren and drongo are clean except for fairywren upgrading 9.6 to 11. >> This appears to be a longstanding issue that the fuzz processing was >> causing us to ignore. See for example >> <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2022-09-01%2018%3A27%3A28&stg=xversion-upgrade-REL_10_STABLE-REL_11_STABLE> > Interesting. I suspected that removing the fuzz allowance would teach > us some things we hadn't known about. > >> I propose to add this to just the release 11 AdjustUpgrade.pm: >> # float4 values in this table on Msys can have precision differences >> # in representation between old and new versions >> if ($old_version < 10 && $dbnames{contrib_regression_btree_gist} && >> $^O eq 'msys') >> { >> _add_st($result, 'contrib_regression_btree_gist', >> 'drop table if exists float4tmp'); >> } > Seems reasonable (but I wonder if you don't need "$old_version < 11"). > A nicer answer would be to apply --extra-float-digits=0 across the > board, but pre-v12 pg_dump lacks that switch. > > It turns out this was due to the fact that fairywren's setup changed some time after the EOL of 9.6. I have rebuilt 9.6 and earlier backbranches and there should now be no need for this adjustment. There is still a Windows issue with MSVC builds <= 9.4 that I'm trying to track down. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
I just hit a snag testing this. It turns out that the
PostgreSQL::Version comparison stuff believes that 16beta2 < 16, which
sounds reasonable. However, because of that, the AdjustUpgrade.pm
stanza that tries to drop tables public.gtest_normal_child{2} in
versions earlier than 16 fails, because by 16 these tables are dropped
in the test itself rather than left to linger, as was the case in
versions 15 and earlier.
So, if you try to run the pg_upgrade test with a dump created by
16beta2, it will fail to drop these tables (because they don't exist)
and the whole test fails. Why hasn't the buildfarm detected this
problem? I see that Drongo is happy, but I don't understand why.
Apparently, the AdjustUpgrade.pm stuff leaves no trace.
I can fix this either by using DROP IF EXISTS in that stanza, or by
making AdjustUpgrade use 'version <= 15'. Any opinions on which to
prefer?
(Well, except that the tests added by c66a7d75e65 a few days ago fail
for some different reason -- the tests want pg_upgrade to fail, but it
doesn't fail for me.)
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.
On 2023-07-19 We 07:05, Alvaro Herrera wrote:
I just hit a snag testing this. It turns out that the
PostgreSQL::Version comparison stuff believes that 16beta2 < 16, which
sounds reasonable. However, because of that, the AdjustUpgrade.pm
stanza that tries to drop tables public.gtest_normal_child{2} in
versions earlier than 16 fails, because by 16 these tables are dropped
in the test itself rather than left to linger, as was the case in
versions 15 and earlier.
So, if you try to run the pg_upgrade test with a dump created by
16beta2, it will fail to drop these tables (because they don't exist)
and the whole test fails. Why hasn't the buildfarm detected this
problem? I see that Drongo is happy, but I don't understand why.
Apparently, the AdjustUpgrade.pm stuff leaves no trace.The buildfarm module assumes that no adjustments are necessary if the old and new versions are the same (e.g. HEAD to HEAD). And it never passes in a version like '16beta2'. It extracts the version number from the branch name, e.g. REL_16_STABLE => 16.
I can fix this either by using DROP IF EXISTS in that stanza, or by making AdjustUpgrade use 'version <= 15'. Any opinions on which to prefer?
The trouble is this could well break the next time someone puts in a test like this.
Maybe we need to make AdjustUpgrade just look at the major version, something like:
$old_version = PostgreSQL::Version->new($old_version->major);
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2023-Jul-19, Andrew Dunstan wrote:
>
> On 2023-07-19 We 07:05, Alvaro Herrera wrote:
> > I just hit a snag testing this. It turns out that the
> > PostgreSQL::Version comparison stuff believes that 16beta2 < 16, which
> > sounds reasonable. However, because of that, the AdjustUpgrade.pm
> > stanza that tries to drop tables public.gtest_normal_child{2} in
> > versions earlier than 16 fails, because by 16 these tables are dropped
> > in the test itself rather than left to linger, as was the case in
> > versions 15 and earlier.
> The buildfarm module assumes that no adjustments are necessary if the old
> and new versions are the same (e.g. HEAD to HEAD). And it never passes in a
> version like '16beta2'. It extracts the version number from the branch name,
> e.g. REL_16_STABLE => 16.
Hmm, OK, but I'm not testing the same versions -- I'm testing 16beta2 to
17devel.
> > I can fix this either by using DROP IF EXISTS in that stanza, or by
> > making AdjustUpgrade use 'version <= 15'. Any opinions on which to
> > prefer?
>
> The trouble is this could well break the next time someone puts in a test
> like this.
Hmm, I don't understand what you mean.
> Maybe we need to make AdjustUpgrade just look at the major version,
> something like:
>
> $old_version = PostgreSQL::Version->new($old_version->major);
It seems like that does work, but if we do that, then we also need to
change this line:
if ($old_version lt '9.5')
to
if ($old_version < '9.5')
otherwise you get some really mysterious failures about trying to drop
public.=>, which is in fact no longer accepted syntax since 9.5; and the
stringwise comparison returns the wrong value here.
TBH I'm getting a sense of discomfort with the idea of having developed
a Postgres-version-number Perl module, and in the only place where we
can use it, have to settle for numeric comparison instead.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
¡Ay, ay, ay! Con lo mucho que yo lo quería (bis)
se fue de mi vera ... se fue para siempre, pa toíta ... pa toíta la vida
¡Ay Camarón! ¡Ay Camarón! (Paco de Lucía)
Вложения
On 2023-07-19 We 12:05, Alvaro Herrera wrote:
On 2023-Jul-19, Andrew Dunstan wrote:On 2023-07-19 We 07:05, Alvaro Herrera wrote:I just hit a snag testing this. It turns out that the PostgreSQL::Version comparison stuff believes that 16beta2 < 16, which sounds reasonable. However, because of that, the AdjustUpgrade.pm stanza that tries to drop tables public.gtest_normal_child{2} in versions earlier than 16 fails, because by 16 these tables are dropped in the test itself rather than left to linger, as was the case in versions 15 and earlier.The buildfarm module assumes that no adjustments are necessary if the old and new versions are the same (e.g. HEAD to HEAD). And it never passes in a version like '16beta2'. It extracts the version number from the branch name, e.g. REL_16_STABLE => 16.Hmm, OK, but I'm not testing the same versions -- I'm testing 16beta2 to 17devel.
Yeah, but you asked why the buildfarm didn't see this effect, and the answer is that it never uses version arguments like '16beta2'.
I can fix this either by using DROP IF EXISTS in that stanza, or by making AdjustUpgrade use 'version <= 15'. Any opinions on which to prefer?The trouble is this could well break the next time someone puts in a test like this.Hmm, I don't understand what you mean.
I want to prevent things like this from happening in the future if someone puts a test in the development branch with "if ($oldversion < nn)".
Maybe we need to make AdjustUpgrade just look at the major version, something like: $old_version = PostgreSQL::Version->new($old_version->major);It seems like that does work, but if we do that, then we also need to change this line: if ($old_version lt '9.5') to if ($old_version < '9.5') otherwise you get some really mysterious failures about trying to drop public.=>, which is in fact no longer accepted syntax since 9.5; and the stringwise comparison returns the wrong value here.
That seems odd. String comparison like that is supposed to work. I will do some tests.
TBH I'm getting a sense of discomfort with the idea of having developed a Postgres-version-number Perl module, and in the only place where we can use it, have to settle for numeric comparison instead.
These comparisons only look like that. They are overloaded in PostgreSQL::Version.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2023-07-19 We 15:20, Andrew Dunstan wrote:
On 2023-07-19 We 12:05, Alvaro Herrera wrote:
Maybe we need to make AdjustUpgrade just look at the major version, something like: $old_version = PostgreSQL::Version->new($old_version->major);It seems like that does work, but if we do that, then we also need to change this line: if ($old_version lt '9.5') to if ($old_version < '9.5') otherwise you get some really mysterious failures about trying to drop public.=>, which is in fact no longer accepted syntax since 9.5; and the stringwise comparison returns the wrong value here.
That seems odd. String comparison like that is supposed to work. I will do some tests.
TBH I'm getting a sense of discomfort with the idea of having developed a Postgres-version-number Perl module, and in the only place where we can use it, have to settle for numeric comparison instead.
These comparisons only look like that. They are overloaded in PostgreSQL::Version.
The result you report suggest to me that somehow the old version is no longer a PostgreSQL::Version object. Here's the patch I suggest:
diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
index a241d2ceff..d7a7383deb 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -74,6 +74,11 @@ values are arrayrefs to lists of statements to be run in those databases.
sub adjust_database_contents
{
my ($old_version, %dbnames) = @_;
+
+ die "wrong type for \$old_version\n"
+ unless $old_version->isa("PostgreSQL::Version");
+ $old_version = PostgreSQL::Version->new($old_version->major);
+
my $result = {};
# remove dbs of modules known to cause pg_upgrade to fail
Do you still see errors with that?
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2023-07-19 We 16:44, Andrew Dunstan wrote:
On 2023-07-19 We 15:20, Andrew Dunstan wrote:
On 2023-07-19 We 12:05, Alvaro Herrera wrote:
Maybe we need to make AdjustUpgrade just look at the major version, something like: $old_version = PostgreSQL::Version->new($old_version->major);It seems like that does work, but if we do that, then we also need to change this line: if ($old_version lt '9.5') to if ($old_version < '9.5') otherwise you get some really mysterious failures about trying to drop public.=>, which is in fact no longer accepted syntax since 9.5; and the stringwise comparison returns the wrong value here.
That seems odd. String comparison like that is supposed to work. I will do some tests.
TBH I'm getting a sense of discomfort with the idea of having developed a Postgres-version-number Perl module, and in the only place where we can use it, have to settle for numeric comparison instead.
These comparisons only look like that. They are overloaded in PostgreSQL::Version.
The result you report suggest to me that somehow the old version is no longer a PostgreSQL::Version object. Here's the patch I suggest:
diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
index a241d2ceff..d7a7383deb 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -74,6 +74,11 @@ values are arrayrefs to lists of statements to be run in those databases.
sub adjust_database_contents
{
my ($old_version, %dbnames) = @_;
+
+ die "wrong type for \$old_version\n"
+ unless $old_version->isa("PostgreSQL::Version");
+ $old_version = PostgreSQL::Version->new($old_version->major);
+
my $result = {};
# remove dbs of modules known to cause pg_upgrade to fail
Do you still see errors with that?
Just realized it would need to be applied in all three exported routines.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2023-Jul-19, Andrew Dunstan wrote: > The result you report suggest to me that somehow the old version is no > longer a PostgreSQL::Version object. Here's the patch I suggest: Ahh, okay, that makes more sense; and yes, it does work. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Вложения
On 2023-07-20 Th 05:52, Alvaro Herrera wrote:
On 2023-Jul-19, Andrew Dunstan wrote:The result you report suggest to me that somehow the old version is no longer a PostgreSQL::Version object. Here's the patch I suggest:Ahh, okay, that makes more sense; and yes, it does work.
Your patch LGTM
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2023-Jul-20, Andrew Dunstan wrote: > On 2023-07-20 Th 05:52, Alvaro Herrera wrote: > > On 2023-Jul-19, Andrew Dunstan wrote: > > > > > The result you report suggest to me that somehow the old version is no > > > longer a PostgreSQL::Version object. Here's the patch I suggest: > > Ahh, okay, that makes more sense; and yes, it does work. > > Your patch LGTM Thanks for looking. I pushed it to 16 and master. I considered applying all the way down to 9.2, but I decided it'd be pointless. We can backpatch later if we find there's need. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'. After collecting 500 such letters, he mused, a university somewhere in Arizona would probably grant him a degree. (Don Knuth)