> I sat down to write the two PUBLIC cases I suggested, and hit something bigger
> on the way. As it stands the test does not distinguish the fixed pg_dump from
> the broken one.
>
> I took v7, reverted just the two SAFE_INITPRIVS call sites in pg_dump.c, left
> the rest in place, rebuilt, and re-ran the test as posted -- with nothing added
> but the quote_ident() from my last mail, so that it runs at all:
>
> t/008_pg_dump_dangling_initprivs.pl .. ok
> All tests successful.
> Files=1, Tests=12
>
> The objects that carry the dangling entries all keep their default ACL, so
> pg_dump emits no GRANT/REVOKE for them at all, patched or not, and the
> assertions that matter are all unlike() -- they hold vacuously.
> --binary-upgrade doesn't change that.
>
> One added line per case is enough to make it bite: give the object a non-default
> ACL. For case 1,
>
> CREATE FUNCTION public.test_func_grantee() RETURNS int LANGUAGE
> sql AS 'SELECT 1';
> REVOKE ALL ON FUNCTION public.test_func_grantee() FROM PUBLIC;
>
> and case 1 then fails on the reverted build and passes on v7. The dump from the
> reverted build is the reported breakage:
>
> REVOKE ALL ON FUNCTION public.test_func_grantee() FROM "16385";
> GRANT ALL ON FUNCTION public.test_func_grantee() TO "rui.zhao";
>
> against v7's:
>
> REVOKE ALL ON FUNCTION public.test_func_grantee() FROM PUBLIC;
>
> That also shows the catch-all assertion is too narrow: it looks for
> GRANT ... TO "digits", but the bare OID here lands in a
> REVOKE ... FROM "digits".
>
> Built the same way, the two PUBLIC cases from my last mail do bite as well: a
> PUBLIC entry with a dangling grantor fails on the reverted build, and a valid
> PUBLIC entry stops being dumped if the "ace.grantee <> 0" guard is removed.
>
> Attached is a test-only patch on top of v7 that does all of the above:
> quote_ident() on the four literals, a non-default ACL for the objects in cases
> 1 to 3, the widened catch-all, and the two PUBLIC cases. With that patch in
> place the same three builds give:
>
> v7's pg_dump as posted 14/14 pass
> without the SAFE_INITPRIVS calls cases 1, 2, 3, 6 and the catch-all fail
> without "ace.grantee <> 0" case 7 fails
>
> One I left alone: case 4, the spurious-REVOKE one. Its premise is that proacl is
> NULL, and that is exactly what stops pg_dump emitting anything for the object,
> so I could not give it a non-default ACL the way I did for the others without
> turning it into case 1. I did try to reproduce the spurious REVOKE with proacl
> NULL a few other ways -- on a plain function, on a pinned catalog function, and
> on a real extension member, plain and with --binary-upgrade -- and got no ACL
> output at all in any of them. Where a bare OID does turn up in a REVOKE is when
> the object has a non-default ACL, which cases 1 to 3 now cover. So the
> precondition in that part of the commit message may not be quite right; you'd
> know better whether there's a path I'm missing.
Hi,
Thanks for the feedback. I improved the TAP test and v8 attached.
In case 4: you were right that it couldn't be reproduced. A function
with proacl IS NULL never produces ACL output at all, regardless of
pg_init_privs — that's why none of your repro attempts worked.
The same bug is real for table columns, though. A column with no
grants but a fully-dangling pg_init_privs entry can produce a broken
REVOKE ... FROM "<oid>". I replaced case 4 with that scenario.
Also added two cases: the same dangling-grantee bug on an aggregate,
and a run under --binary-upgrade. Both were untested paths in pg_dump.
Every assertion, old and new, is mutation-tested.
Regards,
Demir
