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

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.

>> 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:

Total coverage sounds good here.

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

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.

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

That's right. It would be sad for monitoring queries doing large
scans of pg_depend or pg_shdepend to fail in obstructive ways because
of concurrent object drops, because we'd lose information about all
the other objects because of at least one object gone at the moment
where pg_get_acl() is called for its OID retrieved previously.

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

NULL is the correct answer for all that, IMO.

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

Okay. If another committer is interested in that, I'd be OK if there
is a consensus on this point. The fact that I'm not convinced does
not mean that it would show enough value for somebody else.
--
Michael