Hello Jelte, everyone,
v11 applies cleanly to master (09a579abaca) and CI is green, so I went
looking at the one part that CI cannot reach. In June 2024 you wrote:
> I also think I changed the pg_upgrade to do the correct thing, but I'm
> not sure how to test this (even manually). Because part of it would
> only be relevant once we support upgrading from PG18. So for now the
> upgrade_code I haven't actually run.
That is still true, and unfortunately that code does not work. It is
easy to run now: pg_upgrade invokes the NEW pg_dump against the OLD
server, so all it takes is a real PG18 cluster and the patched binary.
Everything below was measured on master 09a579abaca with clean builds
(--enable-cassert), against a PG18.6 cluster built from REL_18_STABLE,
with unpatched master as the control. Scripts attached.
1. pg_dump against any older server fails
------------------------------------------
getExtensions() asks:
if (fout->remoteVersion >= 180000)
appendPQExpBufferStr(query, ", x.extownedschema ");
The patch was written when 18 was the development target. master is
20devel now, so the column would appear in 20, not in 18. Against an
18 or 19 server the query asks for a column that does not exist:
pg_dump (master), PG18 server: ok
pg_dump (v11), PG18 server: ERROR: column x.extownedschema
does not exist
pg_dumpall (v11), PG18 server: same error
pg_upgrade PG18 -> v11: Failure, exiting
This is not only pg_upgrade. getExtensions() runs for every pg_dump,
so dumping an older server with new binaries - the normal way to move
between major versions - fails too.
The one-line change is attached as nocfbot-fix-remoteVersion.diff.txt
(180000 -> 200000). With it, on the same clusters:
pg_dump (fixed), PG18 server: ok, same output as master except
for the random \restrict token
pg_upgrade PG18 -> fixed: rc=0
It might be worth a comment there, since this number has to be bumped
again every time the patch misses a release.
2. Every pg_dump of a database with an owned-schema extension warns
--------------------------------------------------------------------
pg_dump: warning: could not resolve dependency loop among these items:
pg_dump: detail: EXTENSION loopdemo_owned (ID 2 OID 16386)
pg_dump: detail: SCHEMA loopdemo_owned (ID 8 OID 16385)
Control: the same trivial extension installed twice, where the only
difference is owned_schema in the control file.
owned_schema = true -> warning on every dump (plain and
--binary-upgrade)
owned_schema = false -> no warning
The dump still restores correctly, because pg_dump breaks the loop on
its own, but it is a loop it knows nothing about, and users of the
feature would see this on every single dump.
The loop is real: the extension is in the schema, and the schema is a
member of the extension. pg_dump_sort.c already repairs the loops it
knows (type/function, view/rule), so the second attached diff adds
this case: drop the extension's dependency on the schema, since CREATE
EXTENSION is what creates the schema. That is the direction that can
go, and it leaves the schema-is-a-member dependency intact.
With that diff the warning is gone, and make check, src/test/modules,
src/bin/pg_dump and src/bin/pg_upgrade all still pass.
3. test_pg_dump cannot be run on its own any more
--------------------------------------------------
make -C src/test/modules/test_pg_dump check
master: PASS (1044 tests)
v11: FAIL - extension "test_ext_owned_schema" is not available
The new case in test_pg_dump/t/001_base.pl installs an extension that
lives in src/test/modules/test_extensions, and nothing makes that
module available to this one. Running the whole src/test/modules
directory passes, which is presumably why CI never noticed.
4. What I checked that does work
---------------------------------
Same-version pg_upgrade of a cluster with an owned-schema extension
installed keeps the catalog right (extownedschema still true, schema
still the extension's own), the --binary-upgrade dump emits the
expected binary_upgrade_create_empty_extension(..., true, ...) and no
separate CREATE SCHEMA, and a plain dump/restore round trip works.
make check, src/test/modules and src/bin/pg_dump pass with v11 as is.
I have not reviewed the security model itself; the thread has already
covered that ground at length and I did not want to reopen it. These
are only the things I could measure.
Regards,
Manu
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 72ed8fc..4492479 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6222,7 +6222,7 @@ getExtensions(Archive *fout, int *numExtensions)
"x.extname, n.nspname,
x.extrelocatable, x.extversion, x.extconfig, x.extcondition "
);
- if (fout->remoteVersion >= 180000)
+ if (fout->remoteVersion >= 200000)
appendPQExpBufferStr(query, ", x.extownedschema ");
else
appendPQExpBufferStr(query, ", false AS extownedschema ");
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 4f3469d..b10ec6b 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -174,6 +174,8 @@ static int findLoop(DumpableObject *obj,
DumpId *searchFailed,
DumpableObject **workspace,
int depth);
+static void repairExtensionOwnedSchemaLoop(DumpableObject *extobj,
+
DumpableObject *nspobj);
static void repairDependencyLoop(DumpableObject **loop,
int nLoop);
static void describeDumpableObject(DumpableObject *obj,
@@ -928,6 +930,24 @@ findLoop(DumpableObject *obj,
return 0;
}
+/*
+ * An extension whose control file sets owned_schema=true has a dependency
+ * loop with its schema: the extension is in the schema, and the schema is a
+ * member of the extension. Break the loop by dropping the extension's
+ * dependency on the schema, since CREATE EXTENSION creates the schema.
+ */
+static void
+repairExtensionOwnedSchemaLoop(DumpableObject *extobj, DumpableObject *nspobj)
+{
+ /*
+ * An extension with owned_schema=true creates its schema itself, as
part
+ * of CREATE EXTENSION, so the extension need not wait for the schema to
+ * be created first. (The schema is a member of the extension, hence
the
+ * dependency in the other direction, which is the one we keep.)
+ */
+ removeObjectDependency(extobj, nspobj->dumpId);
+}
+
/*
* A user-defined datatype will have a dependency loop with each of its
* I/O functions (since those have the datatype as input or output).
@@ -1191,6 +1211,22 @@ repairDependencyLoop(DumpableObject **loop,
return;
}
+ /* Extension and the schema that the extension itself creates */
+ if (nLoop == 2 &&
+ loop[0]->objType == DO_EXTENSION &&
+ loop[1]->objType == DO_NAMESPACE)
+ {
+ repairExtensionOwnedSchemaLoop(loop[0], loop[1]);
+ return;
+ }
+ if (nLoop == 2 &&
+ loop[1]->objType == DO_EXTENSION &&
+ loop[0]->objType == DO_NAMESPACE)
+ {
+ repairExtensionOwnedSchemaLoop(loop[1], loop[0]);
+ return;
+ }
+
/* View (including matview) and its ON SELECT rule */
if (nLoop == 2 &&
loop[0]->objType == DO_TABLE &&
#!/usr/bin/env bash
# #5018: el camino de pg_upgrade que el autor dijo (2024-06-19) no haber
# ejecutado nunca: "I'm not sure how to test this (even manually) ... the
# upgrade code I haven't actually run."
#
# pg_upgrade corre el pg_dump NUEVO contra el servidor VIEJO. Asi que basta
# un cluster PG18 real y el pg_dump del build parchado.
#
# Control: el mismo pg_dump de master (sin el patch) contra el mismo cluster.
set -u
BASE=$HOME/pg5018
D18=$BASE/data18
P18=55418
rm -rf "$D18"
"$BASE/i-rel18/bin/initdb" -D "$D18" -U postgres --no-sync -A trust >
"$BASE/initdb18.log" 2>&1 \
|| { echo "initdb 18 FALLO"; tail -5 "$BASE/initdb18.log"; exit 1; }
"$BASE/i-rel18/bin/pg_ctl" -D "$D18" -o "-p $P18" -l "$BASE/pg18.log" -w start
> /dev/null 2>&1 \
|| { echo "start 18 FALLO"; tail -5 "$BASE/pg18.log"; exit 1; }
echo "== servidor viejo: $("$BASE/i-rel18/bin/psql" -p $P18 -U postgres -tAc
'select version()' | cut -c1-40)"
# Una extension cualquiera, de las que trae el propio arbol.
"$BASE/i-rel18/bin/psql" -p $P18 -U postgres -qc 'CREATE EXTENSION IF NOT
EXISTS plpgsql' 2>/dev/null
"$BASE/i-rel18/bin/psql" -p $P18 -U postgres -tAc \
"select extname from pg_extension order by 1" | sed 's/^/ extension: /'
for w in master v11; do
echo "== pg_dump --binary-upgrade del build '$w' contra el servidor PG18"
if "$BASE/i-$w/bin/pg_dump" -p $P18 -U postgres --binary-upgrade -s -d
postgres \
> "$BASE/dump_${w}_desde18.sql" 2> "$BASE/dump_${w}_desde18.err"; then
echo " OK ($(wc -l < "$BASE/dump_${w}_desde18.sql") lineas)"
else
echo " FALLA:"
sed 's/^/ /' "$BASE/dump_${w}_desde18.err" | head -5
fi
done
"$BASE/i-rel18/bin/pg_ctl" -D "$D18" -w stop > /dev/null 2>&1
echo "== FIN"
#!/usr/bin/env bash
# Control limpio del warning "could not resolve dependency loop":
# la MISMA extension trivial, dos veces, y lo unico que cambia entre las dos
# es owned_schema = true|false en el control file.
set -u
B=$HOME/pg5018/i-fix
EXT=$B/share/postgresql/extension
D=$HOME/pg5018/data_ctl
P=55422
for n in loopdemo_owned loopdemo_plain; do
cat > "$EXT/$n--1.0.sql" <<EOF
CREATE FUNCTION hola() RETURNS int LANGUAGE sql AS 'SELECT 1';
EOF
done
cat > "$EXT/loopdemo_owned.control" <<'EOF'
comment = 'demo con schema propio'
default_version = '1.0'
relocatable = false
owned_schema = true
schema = 'loopdemo_owned'
EOF
cat > "$EXT/loopdemo_plain.control" <<'EOF'
comment = 'demo sin schema propio'
default_version = '1.0'
relocatable = false
schema = 'loopdemo_plain'
EOF
rm -rf "$D"; "$B/bin/initdb" -D "$D" -U postgres --no-sync -A trust > /dev/null
2>&1
"$B/bin/pg_ctl" -D "$D" -o "-p $P" -l "$HOME/pg5018/ctl.log" -w start >
/dev/null 2>&1
for n in loopdemo_owned loopdemo_plain; do
"$B/bin/psql" -p $P -U postgres -qc "CREATE DATABASE d_$n" 2>/dev/null
# la extension "plain" necesita su schema creado a mano
[ "$n" = loopdemo_plain ] && "$B/bin/psql" -p $P -U postgres -d "d_$n" -qc
"CREATE SCHEMA $n" 2>/dev/null
if ! "$B/bin/psql" -p $P -U postgres -d "d_$n" -qc "CREATE EXTENSION $n"
2>"/tmp/ce_$n.err"; then
echo " $n: no se pudo crear -> $(head -1 "/tmp/ce_$n.err")"; continue
fi
"$B/bin/pg_dump" -p $P -U postgres -d "d_$n" > "/tmp/dump_$n.sql" 2>
"/tmp/dump_$n.err"
printf " %-16s owned_schema=%-5s -> " "$n" "$([ "$n" = loopdemo_owned ] &&
echo true || echo false)"
if [ -s "/tmp/dump_$n.err" ]; then
echo "WARNING"; sed 's/^/ /' "/tmp/dump_$n.err" | head -4
else
echo "sin warnings"
fi
done
"$B/bin/pg_ctl" -D "$D" -w stop > /dev/null 2>&1
rm -f "$EXT"/loopdemo_*
echo "== FIN"
