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

From: Andrew Dunstan <[email-masked]>
Date: Wed, 19 Jul 2023 09:07:56 -0400

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

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 19 Jul 2023 18:05:02 +0200

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

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

>    $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 <[email-masked]>
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/[email-masked]
---
 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