Date: Sun, 7 Nov 2021 13:22:00 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
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: <20211107192200.GK17618@telsasoft.com>
References: <20210112172753.GC1849@telsasoft.com>
 <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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="g4MvFqI7wmANiPDo"
Content-Disposition: inline
In-Reply-To: <YWPNxDWFia+V4Y4g@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20211107192200.GK17618%40telsasoft.com>
Precedence: bulk


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

On Mon, Oct 11, 2021 at 02:38:12PM +0900, Michael Paquier wrote:
> On Fri, Oct 01, 2021 at 04:58:41PM +0900, Michael Paquier wrote:
> > I was looking at this CF entry, and what you are doing in 0004 to move
> > the tweaks from pg_upgrade's test.sh to a separate SQL script that
> > uses psql's meta-commands like \if to check which version we are on is
> > really interesting.  The patch does not apply anymore, so this needs a
> > rebase.  The entry has been switched as waiting on author by Tom, but
> > you did not update it after sending the new versions in [1].  I am
> > wondering if we could have something cleaner than just a set booleans
> > as you do here for each check, as that does not help with the
> > readability of the tests.
> 
> And so, I am back at this thread, looking at the set of patches
> proposed from 0001 to 0004.  The patches are rather messy and mix many
> things and concepts, but there are basically four things that stand
> out:
> - test.sh is completely broken when using PG >= 14 as new version
> because of the removal of the test tablespace.  Older versions of
> pg_regress don't support --make-tablespacedir so I am fine to stick a
> couple of extra mkdirs for testtablespace/, expected/ and sql/ to
> allow the script to work properly for major upgrades as a workaround,
> but only if we use an old version.  We need to do something here for
> HEAD and REL_14_STABLE.
> - The script would fail when using PG <= 11 as old version because of
> WITH OIDS relations.  We need to do something down to REL_12_STABLE.
> I did not like much the approach taken to stick 4 ALTER TABLE queries
> though (the patch was actually failing here for me), so instead I have
> borrowed what the buildfarm has been doing with a DO block.  That
> works fine, and that's more portable.
> - Not using --extra-float-digits with PG <= 11 as older version causes
> a bunch of diffs in the dumps, making the whole unreadable.  The patch
> was doing that unconditionally for *all version*, which is not good.
> We should only do that on the versions that need it, and we know the
> old version number before taking any dumps so that's easy to check.
> - The addition of --wal-segsize and --allow-group-access breaks the
> script when using PG < 10 at initdb time as these got added in 11.
> With 10 getting EOL'd next year and per the lack of complaints, I am
> not excited to do anything here and I'd rather leave this out so as we
> keep coverage for those options across *all* major versions upgraded
> from 11~.  The buildfarm has tests down to 9.2, but for devs my take
> is that this is enough for now.

Michael handled those in fa66b6d.
Note that the patch assumes that the "old version" being pg_upgraded has
commit 97f73a978: "Work around cross-version-upgrade issues created by commit 9e38c2bb5."

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();"
+                                       DROP TABLE abstime_tbl;"
+                                       DROP TABLE reltime_tbl;"
+                                       DROP TABLE tinterval_tbl;"
+                                       DROP AGGREGATE first_el_agg_any(anyelement);"
+                                       DROP AGGREGATE array_cat_accum(anyarray);"
+                                       DROP OPERATOR @#@(NONE,bigint);"

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:

                my $missing_funcs = q{drop function if exists public.boxarea(box);                                                                                                                             
                              drop function if exists public.funny_dup17();                                                                                                                                    
..                                                                                                                                                                                                             
                        $prstmt = join(';',                                                                                                                                                                    
                                'drop operator @#@ (NONE, bigint)',                                                                                                                                            
..                                                                                                                                                                                                             
                                'drop aggregate if exists public.array_cat_accum(anyarray)',                                                                                                                   

> This is for the basics in terms of fixing test.sh and what should be
> backpatched.  In this aspect patches 0001 and 0002 were a bit
> incorrect.  I am not sure that 0003 is that interesting as designed as
> we would miss any new core types introduced.

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.

+-- And now a test on the previous test, checking that all core types are                                                                                                                                      
+-- included in this table                                                                                                                                                                                     
+-- XXX or some other non-catalog table processed by pg_upgrade                                                                                                                                                
+SELECT oid, typname, typtype, typelem, typarray, typarray FROM pg_type t                                                                                                                                      
+WHERE typtype NOT IN ('p', 'c')                                                                                                                                                                               
+-- reg* which cannot be pg_upgraded                                                                                                                                                                           
+AND oid != ALL(ARRAY['regproc', 'regprocedure', 'regoper', 'regoperator', 'regconfig', 'regdictionary', 'regnamespace', 'regcollation']::regtype[])                                                           
+-- XML might be disabled at compile-time                                                                                                                                                                      
+AND oid != ALL(ARRAY['xml', 'gtsvector', 'pg_node_tree', 'pg_ndistinct', 'pg_dependencies', 'pg_mcv_list', 'pg_brin_bloom_summary', 'pg_brin_minmax_multi_summary']::regtype[])                               
+AND NOT EXISTS (SELECT 1 FROM pg_type u WHERE u.typarray=t.oid) -- exclude arrays                                                                                                                             
+AND NOT EXISTS (SELECT 1 FROM pg_attribute a WHERE a.atttypid=t.oid AND a.attnum>0 AND a.attrelid='manytypes'::regclass);                                                                                     

> 0004 is something I'd like to get done on HEAD to ease the move of the
> pg_upgrade tests to TAP, but it could be made a bit easier to read by
> not having all those oldpgversion_XX_YY flags grouped together for
> one.  So I am going to rewrite portions of it once done with the
> above.


-- 
Justin

--g4MvFqI7wmANiPDo
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v6-0001-pg_upgrade-test-to-exercise-binary-compatibility.patch"