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

On Sun, Jun 23, 2024 at 08:48:46AM +0200, Joel Jacobson wrote:
> On Sat, Jun 22, 2024, at 11:44, Joel Jacobson wrote:
>> * v5-0001-Add-pg_get_acl.patch
>> * v2-0002-Add-pg_get_acl-overloads.patch
>
> Rename files to ensure cfbot applies them in order; both need to
> have same version prefix.

+ <para>
+ Returns the Access Control List (ACL) for a database object,
+ specified by catalog OID and object OID.

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.

+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.

+ 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?

+SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);
+ pg_get_acl
+------------
+
+(1 row)

How about adding a bit more coverage? I'd suggest the following
additions:
- class ID as 0 in input.
- object ID as 0 in input.
- Both class and object ID as 0 in input.

+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.

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.
--
Michael