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

On Tue, Jun 25, 2024, at 03:57, Michael Paquier wrote:
> On Tue, Jun 25, 2024 at 01:21:14AM +0200, Joel Jacobson wrote:
>> 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.
>
> Thanks for doing that! That helps in making reviews easier to follow
> for all, attracting the correct audience when necessary.
>
>>> +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?
>
> Perhaps split the two JOIN conditions into two lines each, with a bit
> more indentation to make it render better? Usually I handle that on a
> case-by-case basis while preparing a patch for commit. I'm OK to edit
> that myself with some final touches, FWIW. Depending on the input
> this shows, I'd also look at some LATERAL business, that can be
> cleaner in some cases for the docs.

Thanks, some indentation certainly helped.
Not sure where LATERAL would help, so leaving that part to you.

>> 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.
>
> This is an internal error, so it should never be visible to the end
> user via SQL because it is an unexpected state. See for example
> 2a10fdc4307a, which is similar to what you are doing here.

Thanks for pointing me to that commit, good to learn about missing_ok.

Not sure if I see how to implement it for pg_get_acl() though.

I've had a look at how pg_describe_object() works for this case:

SELECT pg_describe_object(0,'t'::regclass::oid,0);
ERROR: unsupported object class: 0

I suppose this is the error message we want in pg_get_acl() when
the class ID is invalid?

If no, the rest of this email can be skipped.

If yes, then I suppose we should try to see if there is any existing code
in objectaddress.c that we could reuse, that can throw this error message
for us, for an invalid class OID.

There are three places in objectaddress.c currently capable of
throwing a "unsupported object class" error message:

char *
getObjectDescription(const ObjectAddress *object, bool missing_ok)

char *
getObjectTypeDescription(const ObjectAddress *object, bool missing_ok)

char *
getObjectIdentityParts(const ObjectAddress *object,
List **objname, List **objargs,
bool missing_ok)

All three of them contain a `switch (object->classId)` statement,
where the default branch contains the code that throws the error:

default:
elog(ERROR, "unsupported object class: %u", object->classId);

It would be nice to avoid having to copy the long switch statement, with noops for each branch,
except the default branch, to throw an error in case of an invalid class OID,
but I don't see how we can use any of these three functions in pg_get_acl(), since they
do more things than just checking if the class OID is valid?

So not sure what to do here.

Maybe we want a separate new bool helper function to check if a class OID is valid or not?

That helper function would not be useful for the existing three cases where this error is thrown
in objectaddress.c, since they need actual switch branches for each class ID, whereas in pg_get_acl()
we just need to check if it's valid or not.

I haven't checked, but maybe there are other places in the sources where we just want to check
if a class OID is valid or not, that could benefit from such a helper function.

Or perhaps one exist already?

/Joel