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

[Alvaro Herrera]

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)

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

[Andrew Dunstan]

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

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

[Alvaro Herrera]

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/
>From 055b0d89f7c6667b79ba0b97f54ea5aef64dfbb1 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 19 Jul 2023 17:54:09 +0200
Subject: [PATCH v2] Compare only major versions in AdjustUpgrade.pm

Because PostgreSQL::Version is very nuanced about development version
numbers, the comparison to 16beta2 makes it think that that release is
older than 16, therefore applying a database tweak that doesn't work
there.  Fix by having AdjustUpgrade create a separate
PostgreSQL::Version object that only contains the major version number.

While at it, have it ensure that the objects given are of the expected
type.

Co-authored-by: Andrew Dunstan 
Discussion: https://postgr.es/m/20230719110504.zbu74o54bqqlsufb@alvherre.pgsql
---
 src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm | 12 
 1 file changed, 12 insertions(+)

diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
index a241d2ceff..64305fdcce 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -76,6 +76,10 @@ sub adjust_database_contents
 	my ($old_version, %dbnames) = @_;
 	my $result = {};
 
+	die "wrong type for \$old_version\n"
+		unless $old_version->isa("PostgreSQL::Version");
+	$old_version = PostgreSQL::Version->new($old_version->major);
+
 	# 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')
@@ -262,6 +266,10 @@ sub adjust_old_dumpfile
 {
 	my ($old_version, $dump) = @_;
 
+	die "wrong type for \$old_version\n"
+		unless $old_version->isa("PostgreSQL::Version");
+	$old_version = PostgreSQL::Version->new($old_version->major);
+
 	# use Unix newlines
 	$dump =~ s/\r\n/\n/g;
 
@@ -579,6 +587,10 @@ sub adjust_new_dumpfile
 {
 	my ($old_version, $dump) = @_;
 
+	die "wrong type for \$old_version\n"
+		unless $old_version->isa("PostgreSQL::Version");
+	$old_version = PostgreSQL::Version->new($old_version->major);
+
 	# use Unix newlines
 	$dump =~ s/\r\n/\n/g;
 
-- 
2.39.2

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

[Andrew Dunstan]

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

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

[Andrew Dunstan]

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

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

[Andrew Dunstan]

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

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

[Alvaro Herrera]

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)
>From 6922ab1621f4429b059e82845ea9d6143f3eb9cd Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 19 Jul 2023 17:54:09 +0200
Subject: [PATCH] Use numeric comparison instead of PostgreSQL::Version

This seems like the wrong approach to deal with the problem.

Discussion: https://postgr.es/m/20230719110504.zbu74o54bqqlsufb@alvherre.pgsql
---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 2 +-
 src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index a5688a1cf2..75c9ee9b73 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -242,7 +242,7 @@ if (defined($ENV{oldinstall}))
 	do { $dbnames{$_} = 1; }
 	  foreach split /\s+/s, $dbnames;
 	my $adjust_cmds =
-	  adjust_database_contents($oldnode->pg_version, %dbnames);
+	  adjust_database_contents($oldnode->pg_version->major, %dbnames);
 
 	foreach my $updb (keys %$adjust_cmds)
 	{
diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
index a241d2ceff..301acbd01c 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -200,7 +200,7 @@ sub adjust_database_contents
 			'drop function oldstyle_length(integer, text)');
 	}
 
-	if ($old_version lt '9.5')
+	if ($old_version < '9.5')
 	{
 		# cope with changes of underlying functions
 		_add_st(
-- 
2.39.2

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

[Andrew Dunstan]

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

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

[Alvaro Herrera]

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 HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
 It does it in a really, really complicated way
 why does it need to be complicated?
 Because it's MakeMaker.

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

[Andrew Dunstan]

On 2023-01-18 We 10:33, Tom Lane wrote:
> Andrew Dunstan  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
>> 
> 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

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

[Tom Lane]

Andrew Dunstan  writes:
> See
> 
> I tested it and it seems to be doing the right thing.

Yeah, seems to do what I want.  Thanks!

regards, tom lane

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

[Andrew Dunstan]

On 2023-01-18 We 17:14, Tom Lane wrote:
> Andrew Dunstan  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



I tested it and it seems to be doing the right thing.


cheers


andrew

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

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

[Tom Lane]

Andrew Dunstan  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

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

[Andrew Dunstan]

On 2023-01-18 We 16:14, Tom Lane wrote:
> Andrew Dunstan  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

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

[Tom Lane]

Andrew Dunstan  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

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

[Andrew Dunstan]

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

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

[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;
 

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

[Tom Lane]

Andrew Dunstan  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
> 

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

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

[Andrew Dunstan]

On 2023-01-17 Tu 11:30, Tom Lane wrote:
> Andrew Dunstan  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


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

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

[Tom Lane]

Andrew Dunstan  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 

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

[Tom Lane]

Andrew Dunstan  writes:
> FYI crake has just passed the test with flying colours.

Cool.  I await the Windows machines' results with interest.

regards, tom lane

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

[Andrew Dunstan]

On 2023-01-17 Tu 10:18, Tom Lane wrote:
> Andrew Dunstan  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

- it's a dropin replacement.

FYI crake has just passed the test with flying colours.


cheers


andrew

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

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

[Tom Lane]

Andrew Dunstan  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

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

[Andrew Dunstan]

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

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

[Michael Paquier]

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


signature.asc
Description: PGP signature

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

[Tom Lane]

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

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

[Andrew Dunstan]

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

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

[Tom Lane]

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

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

[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.pm
 line 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 

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

[Tom Lane]

Andrew Dunstan  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

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

[Andrew Dunstan]

On 2023-01-16 Mo 14:34, Tom Lane wrote:
> Andrew Dunstan  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

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

[Tom Lane]

Andrew Dunstan  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

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

[Andrew Dunstan]

On 2023-01-15 Su 18:37, Tom Lane wrote:
> Andrew Dunstan  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
diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
new file mode 100644
index 00..2134afd64e
--- /dev/null
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -0,0 +1,550 @@
+
+# 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 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: 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: 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 

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

[Michael Paquier]

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


signature.asc
Description: PGP signature

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

[Tom Lane]

Andrew Dunstan  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

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

[Andrew Dunstan]

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

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

[Tom Lane]

Michael Paquier  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

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

[Michael Paquier]

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


signature.asc
Description: PGP signature

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

[Tom Lane]

Andrew Dunstan  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

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

[Andrew Dunstan]

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

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

[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 00..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 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: 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: 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 

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

[Tom Lane]

Andrew Dunstan  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

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

[Andrew Dunstan]

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

Extracting cross-version-upgrade knowledge from buildfarm client

[Tom Lane]

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 00..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 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: Branch we are upgrading from, e.g. 'REL_11_STABLE'
+
+=item C: Object with a method C
+to perform SQL against the old installation, returning psql's exit status
+
+=item C: 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