Hi Rui,
Thanks for the review and the test-only patch. v9 attached, all four
points addressed; your patch is in as-is. pg_dump.c is unchanged from v8,
so this is a test and commit-message revision.
> 1. Add the test to src/bin/pg_dump/meson.build.
Done. Confirmed "meson test --list | grep -c dangling" prints 0 without
the line and 1 with it.
For the backpatch: meson.build does not exist before v16, so that hunk is
master/v18/v17/v16 only, and the insertion point differs per branch. On
v15 the make glob still picks the test up.
> 2. Case 9 [...] does not catch the first of those two lines, which is
> the one that matters.
Confirmed with your mutation: reverting only the grantor half of
SAFE_INITPRIVS leaves case 9's GRANT/REVOKE assertion passing while your
new SET SESSION AUTHORIZATION assertion fails. v8's mutation testing could
not have caught this.
> 3. Say in the commit message that when every item of an object is
> dangling, its pg_init_privs row goes away and does not come back.
Added. I reproduced it with a real extension and got your exact shape --
after upgrade, no pg_init_privs row, correct privileges, and a dump
carrying both statements where a fresh install carries CREATE EXTENSION
alone.
I added one more sentence to that paragraph. Without the fix, this same
case does not just look untidy -- the upgrade fails:
ERROR: role "16389" does not exist
So the patch replaces a failed upgrade with a dump that restores correctly
but carries two extra lines. I wanted the message to say that, because on
its own "the row goes away and does not come back" reads like a problem the
patch introduces.
> 4. Reconsider "Backpatch-through: 14".
Agreed, changed to 15. Checked each claim: getAdditionalACLs() has 0
occurrences in v14's pg_dump.c and 4 in v15+; v14 joins pg_init_privs at
twelve sites, and dumpTable's runs once per table; pg_upgrade does use
new_cluster.bindir.
Regards,
Demir.
Rui Zhao <zhaorui126@gmail.com>, 10 Ağu 2026 Pzt, 18:48 tarihinde şunu yazdı:
>
> Hi Hüseyin,
>
> v8 covers both of my v7 points.
>
> Four things I'd raise.
>
> 1. Add the test to src/bin/pg_dump/meson.build. That file lists the TAP tests
> one at a time and this one is not in it, so only make runs it (prove_check
> globs t/*.pl) -- "meson test --list | grep -c dangling" prints 0, so CI never
> runs it at all.
>
> 2. Case 9 looks for a numeric OID in a GRANT or a REVOKE, and a dangling
> grantor never lands there: it lands in the SET SESSION AUTHORIZATION that
> dumpACL() wraps the initial-privileges block in, which is the command the
> reporter's restore died on. With only the grantor half of SAFE_INITPRIVS
> removed, case 9 passes on a dump containing
>
> SET SESSION AUTHORIZATION "16386";
> GRANT ALL ON FUNCTION public.test_func_public_ghost() TO PUBLIC;
>
> Case 9 does not catch the first of those two lines, which is the one that
> matters.
>
> 3. Say in the commit message that when every item of an object is dangling,
> its pg_init_privs row goes away and does not come back. I had an extension
> whose script does
>
> REVOKE ALL ON FUNCTION orphan_reset() FROM PUBLIC;
> GRANT SELECT ON orphan_view TO PUBLIC;
>
> installed by a role that was then dropped, leaving
>
> objoid | classoid | privtype | initprivs
> --------+----------+----------+---------------------------------
> 16387 | pg_proc | e | {16384=X/16384}
> 16388 | pg_class | e | {16384=arwdDxtm/16384,=r/16384}
>
> After the upgrade the new cluster has no pg_init_privs row for either, the
> ACLs are right, and a plain pg_dump of it emits
>
> CREATE EXTENSION IF NOT EXISTS orphantest WITH SCHEMA public;
> REVOKE ALL ON FUNCTION public.orphan_reset() FROM PUBLIC;
> GRANT SELECT ON TABLE public.orphan_view TO PUBLIC;
>
> where the same database with the extension freshly installed emits the
> CREATE EXTENSION line alone: with no initprivs to diff against, pg_dump falls
> back to the type default and re-emits what the extension script already does.
> Harmless on restore, but permanent, and it makes the members look like
> someone changed their privileges.
>
> 4. Reconsider "Backpatch-through: 14". getAdditionalACLs() does not exist
> before v15 (0c9d84427f); v14's pg_dump LEFT JOINs pg_init_privs in a dozen
> per-object queries instead. What makes the filter cheap here is that
> getAdditionalACLs() runs once per dump; v14 has no such place, so
> SAFE_INITPRIVS would land in queries returning a row per object -- the v4
> shape, and what drew the performance objection. v15 and up should take this
> nearly as-is. And since pg_upgrade runs the new cluster's pg_dump, the
> reported v14 -> v18 case is fixed by the v18 copy alone; a v14 backpatch only
> helps pg_dump run out of a v14 installation.
>
> The attached test-only patch does 1 and 2. make check passes in
> src/bin/pg_dump and src/bin/pg_upgrade, and under meson once the test is
> registered.
>
> Thanks,
> Rui
