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
signature.asc
Description: PGP signature