Date: Wed, 17 Nov 2021 16:01:19 +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: <YZSov33oDwW44uMs@paquier.xyz>
References: <20210112175356.GP4320@momjian.us>
 <66753439-83ae-7be5-6e41-9537a6041a39@dunslane.net>
 <d9f14840-9f6e-183d-f43a-9f9b9e1b6d19@enterprisedb.com>
 <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="1JmQlAWzRGz5Hm5r"
Content-Disposition: inline
In-Reply-To: <20211107192200.GK17618@telsasoft.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YZSov33oDwW44uMs%40paquier.xyz>
Precedence: bulk


--1JmQlAWzRGz5Hm5r
Content-Type: multipart/mixed; boundary="j2QComljfBWbZHe2"
Content-Disposition: inline


--j2QComljfBWbZHe2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Nov 07, 2021 at 01:22:00PM -0600, Justin Pryzby wrote:
> That may be good enough for test.sh, but if the kludges were moved to a .sql
> script which was also run by the buildfarm (in stead of its hardcoded kludges), then
> it might be necessary to handle the additional stuff my patch did, like:
>
> +                                       DROP TRANSFORM FOR integer LANGUAGE sql CASCADE;"
> +                                       DROP FUNCTION boxarea(box);"
> +                                       DROP FUNCTION funny_dup17();"

These apply for an old version <= v10.

> +                                       DROP TABLE abstime_tbl;"
> +                                       DROP TABLE reltime_tbl;"
> +                                       DROP TABLE tinterval_tbl;"

old version <= 9.3.

> +                                       DROP AGGREGATE first_el_agg_any(anyelement);"

Not sure about this one.

> +                                       DROP AGGREGATE array_cat_accum(anyarray);"
> +                                       DROP OPERATOR @#@(NONE,bigint);"

These are on 9.4.  It is worth noting that TestUpgradeXversion.pm
recreates those objects.  I'd agree to close the gap completely rather
than just moving what test.sh does to wipe out a maximum client code
for the buildfarm.

> Or, maybe it's guaranteed that the animals all run latest version of old
> branches, in which case I think some of the BF's existing logic could be
> dropped, which would help to reconcile these two scripts:

That seems like a worthy goal to reduce the amount of duplication with
the buildfarm code, while allowing tests from upgrades with older
versions (the WAL segment size and group permission issue in test.sh
had better be addressed in a better way, perhaps once the pg_upgrade
tests are moved to TAP).  There are also things specific to contrib/
modules with older versions, but that may be too specific for this
exercise.

+\if :oldpgversion_le84
+DROP FUNCTION public.myfunc(integer);
+\endif

The oldest version tested by the buildfarm is 9.2, so we could ignore
this part I guess?

Andrew, what do you think about this part?  Based on my read of this
thread, there is an agreement that this approach makes the buildfarm
code more manageable so as committers would not need to patch the
buildfarm code if their test fail.  I agree with this conclusion, but
I wanted to double-check with you first.  This would need a backpatch
down to 10 so as we could clean up a maximum of code in
TestUpgradeXversion.pm without waiting for an extra 5 years.  Please
note that I am fine to send a patch for the buildfarm client.

> We wouldn't miss new core types, because of the 2nd part of type_sanity which
> tests that each core type was included in the "manytypes" table.

Thanks, I see your point now after a closer read.

There is still a pending question for contrib modules, but I think
that we need to think larger here with a better integration of
contrib/ modules in the upgrade testing process.  Making that cheap
would require running the set of regression tests on the instance
to-be-upgraded first.  I think that one step in this direction would
be to have unique databases for each contrib/ modules, so as there is
no overlap with objects dropped?

Having some checks with code types looks fine as a first step, so
let's do that.  I have reviewed 0001, rewrote a couple of comments.
All the comments from upthread seem to be covered with that.  So I'd
like to get that applied on HEAD.  We could as well be less
conservative and backpatch that down to 12 to follow on 7c15cef so we
would be more careful with 15~ already (a backpatch down to 14 would
be enough for this purpose, actually thanks to the 14->15 upgrade
path).
--
Michael

--j2QComljfBWbZHe2
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0001-pg_upgrade-test-to-exercise-binary-compatibility.patch"
Content-Transfer-Encoding: quoted-printable

=46rom c4d766f9a461dad2d51cba3cb8d7d0c523267716 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Wed, 17 Nov 2021 15:57:04 +0900
Subject: [PATCH v7] pg_upgrade: test to exercise binary compatibility

Creating a table with columns of many different datatypes to notice if the
binary format is accidentally changed again, as happened at:
7c15cef86 Base information_schema.sql_identifier domain on name, not varcha=
r.

I checked that if I cherry-pick to v11, and comment out
old_11_check_for_sql_identifier_data_type_usage(), then pg_upgrade/test.sh
detects the original problem:
pg_dump: error: Error message from server: ERROR:  invalid memory alloc req=
uest size 18446744073709551613

I understand the buildfarm has its own cross-version-upgrade test, which I
think would catch this on its own.
---
 src/test/regress/expected/sanity_check.out |   1 +
 src/test/regress/expected/type_sanity.out  | 102 +++++++++++++++++++++
 src/test/regress/sql/type_sanity.sql       | 100 ++++++++++++++++++++
 3 files changed, 203 insertions(+)

diff --git a/src/test/regress/expected/sanity_check.out b/src/test/regress/=
expected/sanity_check.out
index d04dc66db9..63706a28cc 100644
--- a/src/test/regress/expected/sanity_check.out
+++ b/src/test/regress/expected/sanity_check.out
@@ -185,6 +185,7 @@ sql_parts|f
 sql_sizing|f
 stud_emp|f
 student|f
+tab_core_types|f
 tableam_parted_a_heap2|f
 tableam_parted_b_heap2|f
 tableam_parted_c_heap2|f
diff --git a/src/test/regress/expected/type_sanity.out b/src/test/regress/e=
xpected/type_sanity.out
index f567fd378e..3ffd9d0d71 100644
--- a/src/test/regress/expected/type_sanity.out
+++ b/src/test/regress/expected/type_sanity.out
@@ -674,3 +674,105 @@ WHERE p1.rngmultitypid IS NULL OR p1.rngmultitypid =
=3D 0;
 ----------+------------+---------------
 (0 rows)
=20
+-- Create a table that holds all the known in-core data types and leave it
+-- around so as pg_upgrade is able to test their binary compatibility.
+CREATE TABLE tab_core_types AS SELECT
+  '(11,12)'::point,
+  '(1,1),(2,2)'::line,
+  '((11,11),(12,12))'::lseg,
+  '((11,11),(13,13))'::box,
+  '((11,12),(13,13),(14,14))'::path AS openedpath,
+  '[(11,12),(13,13),(14,14)]'::path AS closedpath,
+  '((11,12),(13,13),(14,14))'::polygon,
+  '1,1,1'::circle,
+  'today'::date,
+  'now'::time,
+  'now'::timestamp,
+  'now'::timetz,
+  'now'::timestamptz,
+  '12 seconds'::interval,
+  '{"reason":"because"}'::json,
+  '{"when":"now"}'::jsonb,
+  '$.a[*] ? (@ > 2)'::jsonpath,
+  '127.0.0.1'::inet,
+  '127.0.0.0/8'::cidr,
+  '00:01:03:86:1c:ba'::macaddr8,
+  '00:01:03:86:1c:ba'::macaddr,
+  2::int2, 4::int4, 8::int8,
+  4::float4, '8'::float8, pi()::numeric,
+  'foo'::"char",
+  'c'::bpchar,
+  'abc'::varchar,
+  'name'::name,
+  'txt'::text,
+  true::bool,
+  E'\\xDEADBEEF'::bytea,
+  B'10001'::bit,
+  B'10001'::varbit AS varbit,
+  '12.34'::money,
+  'abc'::refcursor,
+  '1 2'::int2vector,
+  '1 2'::oidvector,
+  format('%s=3DUC/%s', USER, USER)::aclitem,
+  'a fat cat sat on a mat and ate a fat rat'::tsvector,
+  'fat & rat'::tsquery,
+  'a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11'::uuid,
+  '11'::xid8,
+  'pg_class'::regclass,
+  'regtype'::regtype type,
+  'pg_monitor'::regrole,
+  'pg_class'::regclass::oid,
+  '(1,1)'::tid, '2'::xid, '3'::cid,
+  '10:20:10,14,15'::txid_snapshot,
+  '10:20:10,14,15'::pg_snapshot,
+  '16/B374D848'::pg_lsn,
+  1::information_schema.cardinal_number,
+  'l'::information_schema.character_data,
+  'n'::information_schema.sql_identifier,
+  'now'::information_schema.time_stamp,
+  'YES'::information_schema.yes_or_no,
+  'venus'::planets,
+  'i16'::insenum,
+  '(1,2)'::int4range, '{(1,2)}'::int4multirange,
+  '(3,4)'::int8range, '{(3,4)}'::int8multirange,
+  '(1,2)'::float8range, '{(1,2)}'::float8multirange,
+  '(3,4)'::numrange, '{(3,4)}'::nummultirange,
+  '(a,b)'::textrange, '{(a,b)}'::textmultirange,
+  '(12.34, 56.78)'::cashrange, '{(12.34, 56.78)}'::cashmultirange,
+  '(2020-01-02, 2021-02-03)'::daterange,
+  '{(2020-01-02, 2021-02-03)}'::datemultirange,
+  '(2020-01-02 03:04:05, 2021-02-03 06:07:08)'::tsrange,
+  '{(2020-01-02 03:04:05, 2021-02-03 06:07:08)}'::tsmultirange,
+  '(2020-01-02 03:04:05, 2021-02-03 06:07:08)'::tstzrange,
+  '{(2020-01-02 03:04:05, 2021-02-03 06:07:08)}'::tstzmultirange,
+  arrayrange(ARRAY[1,2], ARRAY[2,1]),
+  arraymultirange(arrayrange(ARRAY[1,2], ARRAY[2,1]));
+-- Sanity check on the previous table, checking that all core types are
+-- included in this table.
+SELECT oid, typname, typtype, typelem, typarray, typarray
+  FROM pg_type t
+  WHERE typtype NOT IN ('p', 'c') AND
+    -- reg* types cannot be pg_upgraded, so discard them.
+    oid !=3D ALL(ARRAY['regproc', 'regprocedure', 'regoper',
+                     'regoperator', 'regconfig', 'regdictionary',
+                     'regnamespace', 'regcollation']::regtype[]) AND
+    -- Discard types that do not accept input values as these cannot be
+    -- tested easily.
+    -- Note: XML might be disabled at compile-time.
+    oid !=3D ALL(ARRAY['gtsvector', 'pg_node_tree',
+                     'pg_ndistinct', 'pg_dependencies', 'pg_mcv_list',
+                     'pg_brin_bloom_summary',
+                     'pg_brin_minmax_multi_summary', 'xml']::regtype[]) AND
+    -- Discard arrays.
+    NOT EXISTS (SELECT 1 FROM pg_type u WHERE u.typarray =3D t.oid)
+    -- Exclude everything from the table created above.  This checks
+    -- that no in-core types are missing in tab_core_types.
+    AND NOT EXISTS (SELECT 1
+                    FROM pg_attribute a
+                    WHERE a.atttypid=3Dt.oid AND
+                          a.attnum > 0 AND
+                          a.attrelid=3D'tab_core_types'::regclass);
+ oid | typname | typtype | typelem | typarray | typarray=20
+-----+---------+---------+---------+----------+----------
+(0 rows)
+
diff --git a/src/test/regress/sql/type_sanity.sql b/src/test/regress/sql/ty=
pe_sanity.sql
index 404c3a2043..f92773b75e 100644
--- a/src/test/regress/sql/type_sanity.sql
+++ b/src/test/regress/sql/type_sanity.sql
@@ -495,3 +495,103 @@ WHERE pronargs !=3D 2
 SELECT p1.rngtypid, p1.rngsubtype, p1.rngmultitypid
 FROM pg_range p1
 WHERE p1.rngmultitypid IS NULL OR p1.rngmultitypid =3D 0;
+
+-- Create a table that holds all the known in-core data types and leave it
+-- around so as pg_upgrade is able to test their binary compatibility.
+CREATE TABLE tab_core_types AS SELECT
+  '(11,12)'::point,
+  '(1,1),(2,2)'::line,
+  '((11,11),(12,12))'::lseg,
+  '((11,11),(13,13))'::box,
+  '((11,12),(13,13),(14,14))'::path AS openedpath,
+  '[(11,12),(13,13),(14,14)]'::path AS closedpath,
+  '((11,12),(13,13),(14,14))'::polygon,
+  '1,1,1'::circle,
+  'today'::date,
+  'now'::time,
+  'now'::timestamp,
+  'now'::timetz,
+  'now'::timestamptz,
+  '12 seconds'::interval,
+  '{"reason":"because"}'::json,
+  '{"when":"now"}'::jsonb,
+  '$.a[*] ? (@ > 2)'::jsonpath,
+  '127.0.0.1'::inet,
+  '127.0.0.0/8'::cidr,
+  '00:01:03:86:1c:ba'::macaddr8,
+  '00:01:03:86:1c:ba'::macaddr,
+  2::int2, 4::int4, 8::int8,
+  4::float4, '8'::float8, pi()::numeric,
+  'foo'::"char",
+  'c'::bpchar,
+  'abc'::varchar,
+  'name'::name,
+  'txt'::text,
+  true::bool,
+  E'\\xDEADBEEF'::bytea,
+  B'10001'::bit,
+  B'10001'::varbit AS varbit,
+  '12.34'::money,
+  'abc'::refcursor,
+  '1 2'::int2vector,
+  '1 2'::oidvector,
+  format('%s=3DUC/%s', USER, USER)::aclitem,
+  'a fat cat sat on a mat and ate a fat rat'::tsvector,
+  'fat & rat'::tsquery,
+  'a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11'::uuid,
+  '11'::xid8,
+  'pg_class'::regclass,
+  'regtype'::regtype type,
+  'pg_monitor'::regrole,
+  'pg_class'::regclass::oid,
+  '(1,1)'::tid, '2'::xid, '3'::cid,
+  '10:20:10,14,15'::txid_snapshot,
+  '10:20:10,14,15'::pg_snapshot,
+  '16/B374D848'::pg_lsn,
+  1::information_schema.cardinal_number,
+  'l'::information_schema.character_data,
+  'n'::information_schema.sql_identifier,
+  'now'::information_schema.time_stamp,
+  'YES'::information_schema.yes_or_no,
+  'venus'::planets,
+  'i16'::insenum,
+  '(1,2)'::int4range, '{(1,2)}'::int4multirange,
+  '(3,4)'::int8range, '{(3,4)}'::int8multirange,
+  '(1,2)'::float8range, '{(1,2)}'::float8multirange,
+  '(3,4)'::numrange, '{(3,4)}'::nummultirange,
+  '(a,b)'::textrange, '{(a,b)}'::textmultirange,
+  '(12.34, 56.78)'::cashrange, '{(12.34, 56.78)}'::cashmultirange,
+  '(2020-01-02, 2021-02-03)'::daterange,
+  '{(2020-01-02, 2021-02-03)}'::datemultirange,
+  '(2020-01-02 03:04:05, 2021-02-03 06:07:08)'::tsrange,
+  '{(2020-01-02 03:04:05, 2021-02-03 06:07:08)}'::tsmultirange,
+  '(2020-01-02 03:04:05, 2021-02-03 06:07:08)'::tstzrange,
+  '{(2020-01-02 03:04:05, 2021-02-03 06:07:08)}'::tstzmultirange,
+  arrayrange(ARRAY[1,2], ARRAY[2,1]),
+  arraymultirange(arrayrange(ARRAY[1,2], ARRAY[2,1]));
+
+-- Sanity check on the previous table, checking that all core types are
+-- included in this table.
+SELECT oid, typname, typtype, typelem, typarray, typarray
+  FROM pg_type t
+  WHERE typtype NOT IN ('p', 'c') AND
+    -- reg* types cannot be pg_upgraded, so discard them.
+    oid !=3D ALL(ARRAY['regproc', 'regprocedure', 'regoper',
+                     'regoperator', 'regconfig', 'regdictionary',
+                     'regnamespace', 'regcollation']::regtype[]) AND
+    -- Discard types that do not accept input values as these cannot be
+    -- tested easily.
+    -- Note: XML might be disabled at compile-time.
+    oid !=3D ALL(ARRAY['gtsvector', 'pg_node_tree',
+                     'pg_ndistinct', 'pg_dependencies', 'pg_mcv_list',
+                     'pg_brin_bloom_summary',
+                     'pg_brin_minmax_multi_summary', 'xml']::regtype[]) AND
+    -- Discard arrays.
+    NOT EXISTS (SELECT 1 FROM pg_type u WHERE u.typarray =3D t.oid)
+    -- Exclude everything from the table created above.  This checks
+    -- that no in-core types are missing in tab_core_types.
+    AND NOT EXISTS (SELECT 1
+                    FROM pg_attribute a
+                    WHERE a.atttypid=3Dt.oid AND
+                          a.attnum > 0 AND
+                          a.attrelid=3D'tab_core_types'::regclass);
--=20
2.33.1


--j2QComljfBWbZHe2--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmGUqL8ACgkQnvQgOdby
QH0E4hAApNfO36YLcQB5VpnDuX57MH5elVXhbxIOL503CVxYS2riXXwbPXMd7Ia4
SvjyGnvCrhALFxzKBIrHSVOiOmL+iyJSs827elr02+G2/EMMwdtzsXZ4Ef9W5SIm
lFWYLzydDCB7uCVJsdl2GClawgRciAWNccIb2JenU5nyN5Uf4q4aOz+DuTT+S5RH
ldwx7cinRh4eNkdBTXHKr/+aqM6pItO74lswKO3P194R1rsxPxENqvf4og+g/7wV
rXRqfqLgWQpITZuVCeeJvc4ZitRP7leMjgYr+DFpj0NH7fjG0IFN4RArHJZ3a2Oh
xgiYm1NaYtIVzq0PwLb8we00KQWQvql7hc9jUtSrL/43kkr44xa+lWnQfQuHffHC
TsZD874Or4oPX/e83Tyj4iSQerM96qh8ejdO3K+8U/0RiZqFpALmJo/RsdZW2kmf
ZVdTFQAUz274fkAP1VkyX/wRQPaOQ6MY7sqbxynWJ+TJLNRUd6QFuKwNrTuFBqkn
tsV81wpr7l0AW6wzlpbgUqfE2wtEkvz2TCXDoYoZQNjt5Ldp7x0qtvjqQn2XmLF4
WqrEdYX6MDF4MCDIgiZu7w1Ns3VoI2XNv0YkCu61YLLs0SBobUxXkU6NZ8a8H0rD
IWsjQzw0fINIE+QRIFsposu5BZqlkThnqIqVOQPqk8hbSXA7UTs=
=JB4I
-----END PGP SIGNATURE-----

--1JmQlAWzRGz5Hm5r--