Re: Add pg_get_acl() function get the ACL for a database object

On Mon, Jun 24, 2024, at 01:46, Michael Paquier wrote:
> Rather unrelated to this patch, still this patch makes the situation
> more complicated in the docs, but wouldn't it be better to add ACL as
> a term in acronyms.sql, and reuse it here?  It would be a doc-only
> patch that applies on top of the rest (could be on a new thread of its
> own), with some <acronym> markups added where needed.

Good idea, I've started a separate thread for this:

https://postgr.es/m/9253b872-dbb1-42a6-a79e-b1e96effc857%40app.fastmail.com

This patch now assumes <acronym>ACL</acronym> will be supported.

> +SELECT
> +    (pg_identify_object(s.classid,s.objid,s.objsubid)).*,
> +    pg_catalog.pg_get_acl(s.classid,s.objid)
> +FROM pg_catalog.pg_shdepend AS s
> +JOIN pg_catalog.pg_database AS d ON d.datname = current_database() AND 
> d.oid = s.dbid
> +JOIN pg_catalog.pg_authid AS a ON a.oid = s.refobjid AND s.refclassid 
> = 'pg_authid'::regclass
> +WHERE s.deptype = 'a';
>
> Could be a bit prettier.  That's a good addition.

How could we make it prettier?

> +    catalogId = (classId == LargeObjectRelationId) ? 
> LargeObjectMetadataRelationId : classId;
>
> Indeed, and we need to live with this tweak as per the reason in
> inv_api.c related to clients, so that's fine.  Still a comment is
> adapted for this particular case?

Thanks, fixed.

> How about adding a bit more coverage?  I'd suggest the following
> additions:

Thanks, good idea. I've added the tests,
but need some help reasoning if the output is expected:

> - class ID as 0 in input.

SELECT pg_get_acl(0, 'atest2'::regclass::oid);
ERROR:  unrecognized class ID: 0

I believe we want an error here, since: an invalid class ID,
like 0, or any other invalid OID, should raise an error,
since classes can't be dropped, so we should never
expect an invalid OID for a class ID.
Please correct me if this reasoning is incorrect.

> - object ID as 0 in input.
SELECT pg_get_acl('pg_class'::regclass, 0);

This returns null, which I believe it should,
since the OID for a database object could
be invalid due to having being dropped concurrently.

> - Both class and object ID as 0 in input.

This returns null, but I'm not sure I think this is correct?
Since if the class ID is zero, i.e. incorrect, that is unexpected,
and wouldn't we want to throw an error in that case,
just like if only the class ID is invalid?

> +SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);
> +                                                                       
>                              pg_get_acl                                 
>                                                                    
>
+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> + 
>
{regress_priv_user1=arwdDxtm/regress_priv_user1,regress_priv_user2=r/regress_priv_user1,regress_priv_user3=w/regress_priv_user1,regress_priv_user4=a/regress_priv_user1,regress_priv_user5=D/regress_priv_user1}
> +(1 row)
>
> This is hard to parse.  I would add an unnest() and order the entries
> so as modifications are easier to catch, with a more predictible
> result.

Thanks, much better, fixed.

> FWIW, I'm still a bit meh with the addition of the functions
> overloading the arguments with reg inputs.  I'm OK with that when we
> know that the input would be of a given object type, like
> pg_partition_ancestors or pg_partition_tree, but for a case as generic
> as this one this is less appealing to me.

I've looked at other occurrences of "<type>reg" in func.sgml,
and I now agree with you we should skip the overloads,
since adding them would seem unconventional.

/Joel