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. 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. 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. For now, attached is a patch to address the issues with test.sh that I am planning to backpatch. This fixes the facility on HEAD, while minimizing the diffs between the dumps. We could do more, like a s/PROCEDURE/FUNCTION/ but that does not make the diffs really unreadable either. I have only tested that on HEAD as new version down to 11 as the oldest version per the business with --wal-segsize. This still needs tests with 12~ as new version though, which is boring but not complicated at all :) -- Michael
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 1ba326decd..8593488907 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -23,7 +23,8 @@ standard_initdb() {
# To increase coverage of non-standard segment size and group access
# without increasing test runtime, run these tests with a custom setting.
# Also, specify "-A trust" explicitly to suppress initdb's warning.
- "$1" -N --wal-segsize 1 -g -A trust
+ # --allow-group-access and --wal-segsize have been added in v11.
+ "$1" -N --wal-segsize 1 --allow-group-access -A trust
if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ]
then
cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf"
@@ -107,6 +108,14 @@ EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --outputdir=$outputdir"
export EXTRA_REGRESS_OPTS
mkdir "$outputdir"
+# pg_regress --make-tablespacedir would take care of that in 14~, but this is
+# still required for older versions where this option is not supported.
+if [ "$newsrc" != "$oldsrc" ]; then
+ mkdir "$outputdir"/testtablespace
+ mkdir "$outputdir"/sql
+ mkdir "$outputdir"/expected
+fi
+
logdir=`pwd`/log
rm -rf "$logdir"
mkdir "$logdir"
@@ -163,20 +172,32 @@ createdb "regression$dbname1" || createdb_status=$?
createdb "regression$dbname2" || createdb_status=$?
createdb "regression$dbname3" || createdb_status=$?
+# Extra options to apply to the dump. This may be changed later.
+extra_dump_options=""
+
if "$MAKE" -C "$oldsrc" installcheck-parallel; then
oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
- # before dumping, get rid of objects not feasible in later versions
+ # Before dumping, tweak the database of the old instance depending
+ # on its version.
if [ "$newsrc" != "$oldsrc" ]; then
fix_sql=""
+ # Get rid of objects not feasible in later versions
case $oldpgversion in
804??)
fix_sql="DROP FUNCTION public.myfunc(integer);"
;;
esac
- fix_sql="$fix_sql
- DROP FUNCTION IF EXISTS
- public.oldstyle_length(integer, text); -- last in 9.6
+
+ # Last appeared in v9.6
+ if [ $oldpgversion -lt 100000 ]; then
+ fix_sql="$fix_sql
+ DROP FUNCTION IF EXISTS
+ public.oldstyle_length(integer, text);"
+ fi
+ # Last appeared in v13
+ if [ $oldpgversion -lt 140000 ]; then
+ fix_sql="$fix_sql
DROP FUNCTION IF EXISTS
public.putenv(text); -- last in v13
DROP OPERATOR IF EXISTS -- last in v13
@@ -184,10 +205,40 @@ if "$MAKE" -C "$oldsrc" installcheck-parallel; then
public.#%# (pg_catalog.int8, NONE),
public.!=- (pg_catalog.int8, NONE),
public.#@%# (pg_catalog.int8, NONE);"
+ fi
psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=$?
+
+ # WITH OIDS is not supported anymore in v12, so remove support
+ # for any relations marked as such.
+ if [ $oldpgversion -lt 120000 ]; then
+ fix_sql="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=$?
+ fi
+
+ # Handling of --extra-float-digits gets messy after v12.
+ # Note that this changes the dumps from the old and new
+ # instances if involving an old cluster of v11 or older.
+ if [ $oldpgversion -lt 120000 ]; then
+ extra_dump_options="--extra-float-digits=0"
+ fi
fi
- pg_dumpall --no-sync -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
+ pg_dumpall $extra_dump_options --no-sync \
+ -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
if [ "$newsrc" != "$oldsrc" ]; then
# update references to old source tree's regress.so etc
@@ -249,7 +300,8 @@ esac
pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w
-pg_dumpall --no-sync -f "$temp_root"/dump2.sql || pg_dumpall2_status=$?
+pg_dumpall $extra_dump_options --no-sync \
+ -f "$temp_root"/dump2.sql || pg_dumpall2_status=$?
pg_ctl -m fast stop
if [ -n "$pg_dumpall2_status" ]; then
signature.asc
Description: PGP signature
