Date: Wed, 1 Dec 2021 16:19:44 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Jacob Champion <pchampion@vmware.com>, tgl@sss.pgh.pa.us,
	peter.eisentraut@enterprisedb.com,
	pgsql-hackers@lists.postgresql.org, buschmann@nidsa.net,
	andrew@dunslane.net, noah@leadboat.com,
	tomas.vondra@2ndquadrant.com, bruce@momjian.us, andres@anarazel.de
Subject: Re: pg_upgrade test for binary compatibility of core data types
Message-ID: <YaciEMkHNumImMpk@paquier.xyz>
References: <1575064.1615060903@sss.pgh.pa.us>
 <20210430183348.GQ27406@telsasoft.com>
 <a31d593da98da15fc2302a16b3f3b32e491d3e49.camel@vmware.com>
 <20210912005116.GF26465@telsasoft.com>
 <YVa/se5gxr1PsXDy@paquier.xyz>
 <YWPNxDWFia+V4Y4g@paquier.xyz>
 <20211107192200.GK17618@telsasoft.com>
 <YZSov33oDwW44uMs@paquier.xyz>
 <20211118044728.GI17618@telsasoft.com>
 <YZX5ijyrhvEgPAoE@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="waaQaPmu2eMYg4G/"
Content-Disposition: inline
In-Reply-To: <YZX5ijyrhvEgPAoE@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/YaciEMkHNumImMpk%40paquier.xyz>
Precedence: bulk


--waaQaPmu2eMYg4G/
Content-Type: multipart/mixed; boundary="ej/I6xxrY15LWg29"
Content-Disposition: inline


--ej/I6xxrY15LWg29
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Nov 18, 2021 at 03:58:18PM +0900, Michael Paquier wrote:
> +   ver >=3D 905 AND ver <=3D 1300 AS oldpgversion_95_13,
> +   ver >=3D 906 AND ver <=3D 1300 AS oldpgversion_96_13,
> +   ver >=3D 906 AND ver <=3D 1000 AS oldpgversion_96_10,
> So here, we have the choice between conditions that play with version
> ranges or we could make those checks simpler but compensate with a set
> of IF EXISTS queries.  I think that your choice is right.  The
> buildfarm mixes both styles to compensate with the cases where the
> objects are created after a drop.

So, I have come back to this part of the patch set, that moves the SQL
queries doing the pre-upgrade cleanups in the old version we upgrade
=66rom, and decided to go with what looks like the simplest approach,
relying on some IFEs depending on the object types if they don't
exist for some cases.

While checking the whole thing, I have noticed that some of the
operations were not really necessary.  The result is rather clean now,
with a linear organization of the version logic, so as it is a
no-brainer to get that done in back-branches per the
backward-compatibility argument.

I'll get that done down to 10 to maximize its influence, then I'll
move on with the buildfarm code and send a patch to plug this and
reduce the dependencies between core and the buildfarm code.
--
Michael

--ej/I6xxrY15LWg29
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Move-SQLs-for-cleanups-before-cross-version-upgrades.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 9bfe54d8867c9c05a36976f01ed65e5b8da442f7 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Wed, 1 Dec 2021 16:08:01 +0900
Subject: [PATCH] Move SQLs for cleanups before cross-version upgrades into a
 new file

The plan is to make the buildfarm code re-use this code, and test.sh
held a duplicated logic for this work.  Separating those SQLs into a new
file with a set of \if clauses to do version checks with the old version
upgrading will allow the buildfarm to reuse that.  An extra
simplification is that committers will be able to control the objects
cleaned up without any need to tweak the buldfarm code, at least for the
main regression test suite.

Backpatch down to 10, to maximize its effects.
---
 src/bin/pg_upgrade/test.sh           | 52 ++--------------
 src/bin/pg_upgrade/upgrade_adapt.sql | 92 ++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 47 deletions(-)
 create mode 100644 src/bin/pg_upgrade/upgrade_adapt.sql

diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 8593488907..54c02bc65b 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -181,53 +181,11 @@ if "$MAKE" -C "$oldsrc" installcheck-parallel; then
 	# Before dumping, tweak the database of the old instance depending
 	# on its version.
 	if [ "$newsrc" !=3D "$oldsrc" ]; then
-		fix_sql=3D""
-		# Get rid of objects not feasible in later versions
-		case $oldpgversion in
-			804??)
-				fix_sql=3D"DROP FUNCTION public.myfunc(integer);"
-				;;
-		esac
-
-		# Last appeared in v9.6
-		if [ $oldpgversion -lt 100000 ]; then
-			fix_sql=3D"$fix_sql
-					 DROP FUNCTION IF EXISTS
-						public.oldstyle_length(integer, text);"
-		fi
-		# Last appeared in v13
-		if [ $oldpgversion -lt 140000 ]; then
-			fix_sql=3D"$fix_sql
-				 DROP FUNCTION IF EXISTS
-					public.putenv(text);	-- last in v13
-				 DROP OPERATOR IF EXISTS	-- last in v13
-					public.#@# (pg_catalog.int8, NONE),
-					public.#%# (pg_catalog.int8, NONE),
-					public.!=3D- (pg_catalog.int8, NONE),
-					public.#@%# (pg_catalog.int8, NONE);"
-		fi
-		psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=3D$?
-
-		# WITH OIDS is not supported anymore in v12, so remove support
-		# for any relations marked as such.
-		if [ $oldpgversion -lt 120000 ]; then
-			fix_sql=3D"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';
-				END LOOP;
-				END; \$stmt\$;"
-			psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=3D$?
-		fi
+		# This SQL script has its own idea of the cleanup that needs to be
+		# done and embeds version checks.  Note that this uses the script
+		# stored on the new branch.
+		psql -X -d regression -f "$newsrc/src/bin/pg_upgrade/upgrade_adapt.sql" \
+			|| psql_fix_sql_status=3D$?
=20
 		# Handling of --extra-float-digits gets messy after v12.
 		# Note that this changes the dumps from the old and new
diff --git a/src/bin/pg_upgrade/upgrade_adapt.sql b/src/bin/pg_upgrade/upgr=
ade_adapt.sql
new file mode 100644
index 0000000000..175b2ebe2e
--- /dev/null
+++ b/src/bin/pg_upgrade/upgrade_adapt.sql
@@ -0,0 +1,92 @@
+--
+-- SQL queries for upgrade tests across different major versions.
+--
+-- This file includes a set of SQL queries to make a cluster to-be-upgraded
+-- compatible with the version this file is based on.  Note that this
+-- requires psql, as per-version queries are controlled with a set of \if
+-- clauses.
+
+-- This script is backward-compatible, so it is able to work with any vers=
ion
+-- newer than 9.2 we are upgrading from, up to the branch this script is s=
tored
+-- on (even if this would not run if running pg_upgrade with the same vers=
ion
+-- for the origin and the target).
+
+-- \if accepts a simple boolean value, so all the version checks are
+-- done based on this assumption.
+SELECT
+  ver <=3D 902 AS oldpgversion_le92,
+  ver <=3D 904 AS oldpgversion_le94,
+  ver <=3D 906 AS oldpgversion_le96,
+  ver <=3D 1000 AS oldpgversion_le10,
+  ver <=3D 1100 AS oldpgversion_le11,
+  ver <=3D 1300 AS oldpgversion_le13
+  FROM (SELECT current_setting('server_version_num')::int / 100 AS ver) AS=
 v;
+\gset
+
+-- Objects last appearing in 9.2.
+\if :oldpgversion_le92
+-- Note that those tables are removed from the regression tests in 9.3
+-- and newer versions.
+DROP TABLE abstime_tbl;
+DROP TABLE reltime_tbl;
+DROP TABLE tinterval_tbl;
+\endif
+
+-- Objects last appearing in 9.4.
+\if :oldpgversion_le94
+-- This aggregate has been fixed in 9.5 and later versions, so drop
+-- and re-create it.
+DROP AGGREGATE array_cat_accum(anyarray);
+CREATE AGGREGATE array_larger_accum (anyarray) (
+                  sfunc =3D array_larger,
+                  stype =3D anyarray,
+                  initcond =3D $${}$$);
+-- This operator has been fixed in 9.5 and later versions, so drop and
+-- re-create it.
+DROP OPERATOR @#@ (NONE, bigint);
+CREATE OPERATOR @#@ (PROCEDURE =3D factorial,
+                     RIGHTARG =3D bigint);
+\endif
+
+-- Objects last appearing in 9.6.
+\if :oldpgversion_le96
+DROP FUNCTION public.oldstyle_length(integer, text);
+\endif
+
+-- Objects last appearing in 10.
+\if :oldpgversion_le10
+DROP FUNCTION IF EXISTS boxarea(box);
+DROP FUNCTION IF EXISTS funny_dup17();
+\endif
+
+-- Objects last appearing in 11.
+\if :oldpgversion_le11
+-- WITH OIDS is supported until v11, so remove its support for any
+-- relations marked as such.
+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';
+  END LOOP;
+  END; $stmt$;
+\endif
+
+-- Objects last appearing in 13.
+\if :oldpgversion_le13
+DROP FUNCTION IF EXISTS public.putenv(text);
+-- Until v10, operators could only be dropped one at a time, so be careful
+-- to stick with one command for each drop here.
+DROP OPERATOR public.#@# (pg_catalog.int8, NONE);
+DROP OPERATOR public.#%# (pg_catalog.int8, NONE);
+DROP OPERATOR public.!=3D- (pg_catalog.int8, NONE);
+DROP OPERATOR public.#@%# (pg_catalog.int8, NONE);
+\endif
--=20
2.34.0


--ej/I6xxrY15LWg29--

--waaQaPmu2eMYg4G/
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmGnIhAACgkQnvQgOdby
QH1XAA/7BL8YWZVn+lA2klsPGoZjVbIEGUdDDXOcJWHzQoAw9psTZC7zdKUbL3ws
1ok0XuDSpLp+r7skr1P837z+rla3UwpAngPq6i28UcskvHAO8bioiusSYOfzK6OX
glA3VxSIAXRwDM8mhK2FTcoE017hxtkzPlLIw6hgLSD2cqagQrsfqLiSPxhLTWoI
04UdfEPG6+jQi23wpm9MBWweYtcFEvARHm4PDA8zCCbwXQsjei+pVxgkSxAhHYv7
QkBY69l2b2nG4YWHKMc6ikEnCCX2xTWJh9XMtYJheU/K5vOE8neCFdveKk36S1L8
haVAcnK34KdmEeYoux7WLbtNKs1sPMAnj576s3lLMfU1AUYCx3wVxe1mr7rN9C9M
Y1xjlkiTjtovZ6kkAcPaIx8v6Yln/0Rp367FZpVWDbHBPJuIsMgHKAlLYv5pmPTP
6gMfUTKp21GietsAGJ4pwHhbR7sCNeWRkehF4oRMrTw7zZ/9V9nwCTWwLRLCjTVK
bgP05p9TVSlW3Q7ddq1G8jhOzXeavQetq9EwsFt211113BrhTKN+M/YCVpePNpqI
sddlnYzdF7rP0cujBONRQSVHg/RyaaGCilvXAhw55oPxHFJeONuTbvnmHKjMfSUF
JAd5aa7o/R0DpGRqdVDYxqkoOvUf6TlhmRcquaZ0CYg8oWxW/lA=
=x9lQ
-----END PGP SIGNATURE-----

--waaQaPmu2eMYg4G/--