Thread: Some coverage for DROP OWNED BY with pg_default_acl

Hi all,

I was looking again at the thread that reported a problem when using
ALTER DEFAULT PRIVILEGES with duplicated object names:
https://www.postgresql.org/message-id/ae2a7dc1-9d71-8cba-3bb9-e4cb7eb1f44e@hot.ee

And while reviewing the thing, I have spotted that there is a specific
path for pg_default_acl in RemoveRoleFromObjectACL() that has zero
coverage.  This can be triggered with DROP OWNED BY, and it is
actually safe to run as long as this is done in a separate transaction
to avoid any interactions with parallel regression sessions.
privileges.sql already has similar tests, so I'd like to add some
coverage as per the attached (the duplicated role name is wanted).

Thoughts?
--
Michael

On 2021-Jan-19, Michael Paquier wrote:

> And while reviewing the thing, I have spotted that there is a specific
> path for pg_default_acl in RemoveRoleFromObjectACL() that has zero
> coverage.  This can be triggered with DROP OWNED BY, and it is
> actually safe to run as long as this is done in a separate transaction
> to avoid any interactions with parallel regression sessions.
> privileges.sql already has similar tests, so I'd like to add some
> coverage as per the attached (the duplicated role name is wanted).

Heh, interesting case.  Added coverage is good, so +1.
Since the role regress_priv_user2 is "private" to the privileges.sql
script, there's no danger of a concurrent test getting the added lines
in trouble AFAICS.

> +SELECT count(*) FROM pg_shdepend
> +  WHERE deptype = 'a' AND
> +        refobjid = 'regress_priv_user2'::regrole AND
> +    classid = 'pg_default_acl'::regclass;
> + count 
> +-------
> +     5
> +(1 row)

Shrug.  Seems sufficient.

-- 
Álvaro Herrera       Valdivia, Chile

On Tue, Jan 19, 2021 at 05:49:03PM -0300, Alvaro Herrera wrote:
> Heh, interesting case.  Added coverage is good, so +1.

Thanks.  I read through it again and applied the test.

> Since the role regress_priv_user2 is "private" to the privileges.sql
> script, there's no danger of a concurrent test getting the added lines
> in trouble AFAICS.

It seems to me that it could lead to some trouble if a test running in
parallel expects a set of ACLs with no extra noise, as this stuff adds
data to the catalogs for all objects created while the default
permissions are visible.  Perhaps that's an over-defensive position,
but it does not hurt either to be careful similarly to the test run a
couple of lines above.
--
Michael