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

Hello hackers,

Currently, obtaining the Access Control List (ACL) for a database object
requires querying specific pg_catalog tables directly, where the user
needs to know the name of the ACL column for the object.

Consider:

```
CREATE USER test_user;
CREATE USER test_owner;
CREATE SCHEMA test_schema AUTHORIZATION test_owner;
SET ROLE TO test_owner;
CREATE TABLE test_schema.test_table ();
GRANT SELECT ON TABLE test_schema.test_table TO test_user;
```

To get the ACL we can do:

```
SELECT relacl FROM pg_class WHERE oid = 'test_schema.test_table'::regclass::oid;

                         relacl
---------------------------------------------------------
 {test_owner=arwdDxtm/test_owner,test_user=r/test_owner}
```

Attached patch adds a new SQL-callable functoin `pg_get_acl()`, so we can do:

```
SELECT pg_get_acl('pg_class'::regclass, 'test_schema.test_table'::regclass::oid);
                       pg_get_acl
---------------------------------------------------------
 {test_owner=arwdDxtm/test_owner,test_user=r/test_owner}
```

The original idea for this function came from Alvaro Herrera,
in this related discussion:
https://postgr.es/m/261def6b-226e-4238-b7eb-ff240cb9c2c9@www.fastmail.com

On Thu, Mar 25, 2021, at 16:16, Alvaro Herrera wrote:
> On 2021-Mar-25, Joel Jacobson wrote:
>
>> pg_shdepend doesn't contain the aclitem info though,
>> so it won't work for pg_permissions if we want to expose
>> privilege_type, is_grantable and grantor.
>
> Ah, of course -- the only way to obtain the acl columns is by going
> through the catalogs individually, so it won't be possible.  I think
> this could be fixed with some very simple, quick function pg_get_acl()
> that takes a catalog OID and object OID and returns the ACL; then
> use aclexplode() to obtain all those details.

The pg_get_acl() function has been implemented by following
the guidance from Alvaro in the related dicussion:

On Fri, Mar 26, 2021, at 13:43, Alvaro Herrera wrote:
> AFAICS the way to do it is like AlterObjectOwner_internal obtains data
> -- first do get_catalog_object_by_oid (gives you the HeapTuple that
> represents the object), then
> heap_getattr( ..., get_object_attnum_acl(), ..), and there you have the
> ACL which you can "explode" (or maybe just return as-is).
>
> AFAICS if you do this, it's just one cache lookups per object, or
> one indexscan for the cases with no by-OID syscache.  It should be much
> cheaper than the UNION ALL query.  And you use pg_shdepend to guide
> this, so you only do it for the objects that you already know are
> interesting.

Many thanks Alvaro for the very helpful instructions.

This function would then allow users to e.g. create a view to show the privileges
for all database objects, like the pg_privileges system view suggested in the
related discussion.

Tests and docs are added.

Best regards,
Joel Jakobsson

Hi Ranier,

Thanks for looking at this.

I've double-checked the patch I sent, and it works fine.

I think I know the cause of your problem:

Since this is a catalog change, you need to run `make clean`, to ensure the catalog is rebuilt,
followed by the usual `make && make install`.

You also need to run `initdb` to create a new database cluster, with the new catalog version.

Let me know if you need more specific instructions.

Best,
Joel

On Wed, Jun 19, 2024, at 14:59, Ranier Vilela wrote:
> Em qua., 19 de jun. de 2024 às 08:35, Joel Jacobson <joel@compiler.org>
> escreveu:
>> Hello hackers,
>>
>> Currently, obtaining the Access Control List (ACL) for a database object
>> requires querying specific pg_catalog tables directly, where the user
>> needs to know the name of the ACL column for the object.
>>
>> Consider:
>>
>> ```
>> CREATE USER test_user;
>> CREATE USER test_owner;
>> CREATE SCHEMA test_schema AUTHORIZATION test_owner;
>> SET ROLE TO test_owner;
>> CREATE TABLE test_schema.test_table ();
>> GRANT SELECT ON TABLE test_schema.test_table TO test_user;
>> ```
>>
>> To get the ACL we can do:
>>
>> ```
>> SELECT relacl FROM pg_class WHERE oid = 'test_schema.test_table'::regclass::oid;
>>
>>                          relacl
>> ---------------------------------------------------------
>>  {test_owner=arwdDxtm/test_owner,test_user=r/test_owner}
>> ```
>>
>> Attached patch adds a new SQL-callable functoin `pg_get_acl()`, so we can do:
>>
>> ```
>> SELECT pg_get_acl('pg_class'::regclass, 'test_schema.test_table'::regclass::oid);
>>                        pg_get_acl
>> ---------------------------------------------------------
>>  {test_owner=arwdDxtm/test_owner,test_user=r/test_owner}
>> ```
>>
>> The original idea for this function came from Alvaro Herrera,
>> in this related discussion:
>> https://postgr.es/m/261def6b-226e-4238-b7eb-ff240cb9c2c9@www.fastmail.com
>>
>> On Thu, Mar 25, 2021, at 16:16, Alvaro Herrera wrote:
>> > On 2021-Mar-25, Joel Jacobson wrote:
>> >
>> >> pg_shdepend doesn't contain the aclitem info though,
>> >> so it won't work for pg_permissions if we want to expose
>> >> privilege_type, is_grantable and grantor.
>> >
>> > Ah, of course -- the only way to obtain the acl columns is by going
>> > through the catalogs individually, so it won't be possible.  I think
>> > this could be fixed with some very simple, quick function pg_get_acl()
>> > that takes a catalog OID and object OID and returns the ACL; then
>> > use aclexplode() to obtain all those details.
>>
>> The pg_get_acl() function has been implemented by following
>> the guidance from Alvaro in the related dicussion:
>>
>> On Fri, Mar 26, 2021, at 13:43, Alvaro Herrera wrote:
>> > AFAICS the way to do it is like AlterObjectOwner_internal obtains data
>> > -- first do get_catalog_object_by_oid (gives you the HeapTuple that
>> > represents the object), then
>> > heap_getattr( ..., get_object_attnum_acl(), ..), and there you have the
>> > ACL which you can "explode" (or maybe just return as-is).
>> >
>> > AFAICS if you do this, it's just one cache lookups per object, or
>> > one indexscan for the cases with no by-OID syscache.  It should be much
>> > cheaper than the UNION ALL query.  And you use pg_shdepend to guide
>> > this, so you only do it for the objects that you already know are
>> > interesting.
>>
>> Many thanks Alvaro for the very helpful instructions.
>>
>> This function would then allow users to e.g. create a view to show the privileges
>> for all database objects, like the pg_privileges system view suggested in the
>> related discussion.
>>
>> Tests and docs are added.
> Hi,
> For some reason, the function pg_get_acl, does not exist in generated fmgrtab.c
>
> So, when install postgres, the function does not work.
>
> postgres=# SELECT pg_get_acl('pg_class'::regclass,
> 'atest2'::regclass::oid);
> ERROR:  function pg_get_acl(regclass, oid) does not exist
> LINE 1: SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::...
>                ^
> HINT:  No function matches the given name and argument types. You might
> need to add explicit type casts.
>
> best regards,
> Ranier Vilela

--
Kind regards,

Joel

On Wed, Jun 19, 2024, at 15:51, Ranier Vilela wrote:
> Regarding the patch, could it be written in the following style?

Thanks for nice improvement. New version attached.

Best,
Joel

On Wed, Jun 19, 2024, at 16:23, Isaac Morland wrote:
> I have no idea how often this would be useful, but I wonder if it could
> work to have overloaded single-parameter versions for each of
> regprocedure (pg_proc.proacl), regclass (pg_class.relacl), …. To call,
> just cast the OID to the appropriate reg* type.
>
> For example: To get the ACL for table 'example_table', call pg_get_acl
> ('example_table'::regclass)

+1

New patch attached.

I've added overloaded versions for regclass and regproc so far:

\df pg_get_acl
                             List of functions
   Schema   |    Name    | Result data type |  Argument data types   | Type
------------+------------+------------------+------------------------+------
 pg_catalog | pg_get_acl | aclitem[]        | classid oid, objid oid | func
 pg_catalog | pg_get_acl | aclitem[]        | objid regclass         | func
 pg_catalog | pg_get_acl | aclitem[]        | objid regproc          | func
(3 rows)

/Joel

On Thu, Jun 20, 2024 at 08:32:57AM +0200, Joel Jacobson wrote:
> I've added overloaded versions for regclass and regproc so far:
>
> \df pg_get_acl
>                              List of functions
>    Schema   |    Name    | Result data type |  Argument data types   | Type
> ------------+------------+------------------+------------------------+------
>  pg_catalog | pg_get_acl | aclitem[]        | classid oid, objid oid | func
>  pg_catalog | pg_get_acl | aclitem[]        | objid regclass         | func
>  pg_catalog | pg_get_acl | aclitem[]        | objid regproc          | func
> (3 rows)

Interesting idea.

I am not really convinced that the regproc and regclass overloads are
really necessary, considering the fact that one of the goals
mentioned, as far as I understand, is to be able to get an idea of the
ACLs associated to an object with its dependencies in pg_depend and/or
pg_shdepend.  Another one is to reduce the JOIN burden when querying
a set of them, like attribute ACLs.

Perhaps the documentation should add one or two examples to show this
point?

+        tup = get_catalog_object_by_oid(rel, Anum_oid, objectId);
+        if (!HeapTupleIsValid(tup))
+            elog(ERROR, "cache lookup failed for object %u of catalog \"%s\"",
+                objectId, RelationGetRelationName(rel));

get_catalog_object_by_oid() is handled differently here than in
functions line pg_identify_object().  Shouldn't we return NULL for
this case?  That would be more useful when using this function with
one or more large scans.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Jun 20, 2024 at 08:32:57AM +0200, Joel Jacobson wrote:
>> I've added overloaded versions for regclass and regproc so far:
>>
>> \df pg_get_acl
>> List of functions
>> Schema   |    Name    | Result data type |  Argument data types   | Type
>> ------------+------------+------------------+------------------------+------
>> pg_catalog | pg_get_acl | aclitem[]        | classid oid, objid oid | func
>> pg_catalog | pg_get_acl | aclitem[]        | objid regclass         | func
>> pg_catalog | pg_get_acl | aclitem[]        | objid regproc          | func
>> (3 rows)

> Interesting idea.

Doesn't that result in "cannot resolve ambiguous function call"
failures?

            regards, tom lane

On Fri, Jun 21, 2024, at 05:25, Michael Paquier wrote:
> Interesting idea.
>
> I am not really convinced that the regproc and regclass overloads are
> really necessary, considering the fact that one of the goals
> mentioned, as far as I understand, is to be able to get an idea of the
> ACLs associated to an object with its dependencies in pg_depend and/or
> pg_shdepend.  Another one is to reduce the JOIN burden when querying
> a set of them, like attribute ACLs.

Overloads moved to a second patch, which can be applied
on top of the first one. I think they would be quite nice, but I could
also live without them.

> Perhaps the documentation should add one or two examples to show this
> point?

Good point, added.

>
> +        tup = get_catalog_object_by_oid(rel, Anum_oid, objectId);
> +        if (!HeapTupleIsValid(tup))
> +            elog(ERROR, "cache lookup failed for object %u of catalog \"%s\"",
> +                objectId, RelationGetRelationName(rel));
>
> get_catalog_object_by_oid() is handled differently here than in
> functions line pg_identify_object().  Shouldn't we return NULL for
> this case?  That would be more useful when using this function with
> one or more large scans.

Right, I've changed the patch accordingly.

On Fri, Jun 21, 2024, at 05:48, Isaac Morland wrote:
> Those were just examples. I think for completeness there should be 5 overloads:
>
> [input type] → [relation.aclattribute]
> regproc/regprocedure → pg_proc.proacl
> regtype → pg_type.typacl
> regclass → pg_class.relacl
> regnamespace → pg_namespace.nspacl
>
> I believe the remaining reg* types don't correspond to objects with
> ACLs, and the remaining ACL fields are for objects which don't have a
> corresponding reg* type.
>
> In general I believe the reg* types are underutilized. All over the
> place I see examples where people write code to generate SQL statements
> and they take schema and object name and then format with %I.%I when
> all that is needed is a reg* value and then format it with a simple %s
> (of course, need to make sure the SQL will execute with the same
> search_path as when the SQL was generated, or generate with an empty
> search_path).

I've added regtype and regnamespace overloads to the second patch.

On Fri, Jun 21, 2024, at 05:58, Isaac Morland wrote:
> On Thu, 20 Jun 2024 at 23:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Doesn't that result in "cannot resolve ambiguous function call"
>> failures?
>
> If you try to pass an oid directly, as a value of type oid, you should
> get "function is not unique". But if you cast a string or numeric value
> to the appropriate reg* type for the object you are using, it should
> work fine.

Yes, I can confirm that's the case, it works fine when casting a string
to reg* type.

/Joel

On Sat, Jun 22, 2024, at 02:54, Joel Jacobson wrote:
> Attachments:
> * v4-0001-Add-pg_get_acl.patch
> * 0002-Add-pg_get_acl-overloads.patch

Rebase and reduced diff for src/test/regress/sql/privileges.sql between patches.

/Joel

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.

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

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

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

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

On Tue, Jun 25, 2024 at 08:06:41AM +0200, Joel Jacobson wrote:
> 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?

Ah, and here I thought that this was also returning NULL.  My previous
work in this area only focused on the object OIDs, not their classes.
At the end, I'm OK to keep your patch as it is, checking only for the
case of pinned dependencies in pg_depend as we do for
pg_describe_object().

It's still a bit confusing, but we've been living with that for years
now without anybody complaining except me, so perhaps that's fine at
the end to keep that as this is still useful.  If we change that,
applying the same rules across the board would make the most sense.
--
Michael

On Tue, Jun 25, 2024, at 08:42, Michael Paquier wrote:
> On Tue, Jun 25, 2024 at 08:06:41AM +0200, Joel Jacobson wrote:
>> 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?
>
> Ah, and here I thought that this was also returning NULL.  My previous
> work in this area only focused on the object OIDs, not their classes.
> At the end, I'm OK to keep your patch as it is, checking only for the
> case of pinned dependencies in pg_depend as we do for
> pg_describe_object().
>
> It's still a bit confusing, but we've been living with that for years
> now without anybody complaining except me, so perhaps that's fine at
> the end to keep that as this is still useful.  If we change that,
> applying the same rules across the board would make the most sense.

OK, cool.

New version attached that fixes the indentation of the example,
and uses <literal>NULL</literal> instead of just NULL in the doc.

/Joel