Обсуждение: Re: pgsql: Add pg_get_acl() to get the ACL for a database object

(Moving to -hackers)

On Thu, Jul 04, 2024 at 10:53:49PM +0200, Joel Jacobson wrote:
> On Thu, Jul 4, 2024, at 17:44, Tom Lane wrote:
>> Uh, why is it defined like that rather than allowing a subobject?
>> This definition is unable to fetch column-specific ACLs.

Yes, I was wondering about that as well yesterday when looking at the
patch, and saw that this was already a pretty good cut as it covers
most of the objects types and does 90% of the job, so just moved on.

> I wonder if it would be motivated to provide overloads for this function,
> and perhaps even for pg_get_object_address and pg_identify_object_as_address?
>
> That is, two param versions (class OID and object OID),
> and three param versions that in addition also take subobject ID.

I would still stick to only one function, with arguments coming from
scanning pg_[sh]depend.

> I found some code in aclchk.c on line 4452-4468 that seems useful,
> but not sure. Maybe there is some other existing code that is better
> as an inspiration?

My first feelings about that was that subobjids are only used for
pg_attribute, so if we use them as a base, it would look like:
- Adding a new AttrNumber in ObjectProperty to track to which column
the subobjid should apply and its catcache number (ATTNUM for the
attribute number for fast lookup).
- Extend get_catalog_object_by_oid() with more data: the attribute
column for the subobjid scan stored in ObjectProperty, the subobjid
value given by the caller.  If get_catalog_object_by_oid()'s caller
defines a subobjid, use get_object_catcache_oid() on it over the main
OID one.  If it cannot be found in the cache, use a scan key based on
the class OID and the subobjid.

> I guess we need to handle the RelationRelationId separately,
> and handle all other classes using the current code in pg_get_acl()?

But most of that simply blows away because we track the dependency to
the attribute ACLs in pg_shdepend based on pg_class, not pg_attribute.
get_attname() itself relies on ATTNUM, so perhaps that's OK to just
add an exception based on RelationRelationId in the code path of
pg_get_acl().  That makes me a bit uncomfortable to derive more from
the other routines for the object descriptions, but we've been living
with that in getObjectDescription() for years now, as well.
--
Michael

On Fri, Jul 5, 2024, at 01:18, Michael Paquier wrote:
> I would still stick to only one function, with arguments coming from
> scanning pg_[sh]depend.
>
>> I found some code in aclchk.c on line 4452-4468 that seems useful,
>> but not sure. Maybe there is some other existing code that is better
>> as an inspiration?
>
> My first feelings about that was that subobjids are only used for
> pg_attribute, so if we use them as a base, it would look like:
> - Adding a new AttrNumber in ObjectProperty to track to which column
> the subobjid should apply and its catcache number (ATTNUM for the
> attribute number for fast lookup).
> - Extend get_catalog_object_by_oid() with more data: the attribute
> column for the subobjid scan stored in ObjectProperty, the subobjid
> value given by the caller.  If get_catalog_object_by_oid()'s caller
> defines a subobjid, use get_object_catcache_oid() on it over the main
> OID one.  If it cannot be found in the cache, use a scan key based on
> the class OID and the subobjid.
>
>> I guess we need to handle the RelationRelationId separately,
>> and handle all other classes using the current code in pg_get_acl()?
>
> But most of that simply blows away because we track the dependency to
> the attribute ACLs in pg_shdepend based on pg_class, not pg_attribute.
> get_attname() itself relies on ATTNUM, so perhaps that's OK to just
> add an exception based on RelationRelationId in the code path of
> pg_get_acl().  That makes me a bit uncomfortable to derive more from
> the other routines for the object descriptions, but we've been living
> with that in getObjectDescription() for years now, as well.

OK, I made an attempt to implement this, based on adapting code from
recordExtObjInitPriv() in aclchk.c, which retrieves ACL based on ATTNUM.

There are now two different code paths for columns and non-columns.

It sounds like a bigger refactoring in this area would be nice,
which would enable cleaning up code in other functions as well,
but maybe that's better to do as a separate project.

I've also updated func.sgml, adjusted pg_proc.dat, and added
regression tests for column privileges.

Regards,
Joel

On Fri, Jul 05, 2024 at 10:40:39AM +0200, Joel Jacobson wrote:
> OK, I made an attempt to implement this, based on adapting code from
> recordExtObjInitPriv() in aclchk.c, which retrieves ACL based on ATTNUM.
>
> There are now two different code paths for columns and non-columns.
>
> It sounds like a bigger refactoring in this area would be nice,
> which would enable cleaning up code in other functions as well,
> but maybe that's better to do as a separate project.

Thanks for the patch.  I have been looking at it for a few hours,
eyeing a bit on the ObjectProperty parts a bit if we were to extend it
for sub-object IDs, and did not like the complexity this introduces,
so I'd be OK to live with the extra handling in pg_get_acl() itself.

+       /* ignore dropped columns */
+       if (atttup->attisdropped)
+       {
+           ReleaseSysCache(tup);
+           PG_RETURN_NULL();
+       }

Hmm.  This is an important bit and did not consider it first.  That
makes the use of ObjectProperty less flexible because we want to look
at the data in the pg_attribute tuple to be able to return NULL in
this case.

I've tweaked a bit what you are proposing, simplifying the code and
removing the first batch of queries in the tests as these were less
interesting.  How does that look?
--
Michael

On Mon, Jul 8, 2024, at 10:34, Michael Paquier wrote:
> Thanks for the patch.  I have been looking at it for a few hours,
> eyeing a bit on the ObjectProperty parts a bit if we were to extend it
> for sub-object IDs, and did not like the complexity this introduces,
> so I'd be OK to live with the extra handling in pg_get_acl() itself.
>
> +       /* ignore dropped columns */
> +       if (atttup->attisdropped)
> +       {
> +           ReleaseSysCache(tup);
> +           PG_RETURN_NULL();
> +       }
>
> Hmm.  This is an important bit and did not consider it first.  That
> makes the use of ObjectProperty less flexible because we want to look
> at the data in the pg_attribute tuple to be able to return NULL in
> this case.
>
> I've tweaked a bit what you are proposing, simplifying the code and
> removing the first batch of queries in the tests as these were less
> interesting.  How does that look?

Thanks, nice simplifications.
I agree the tests you removed are not that interesting.

Looks good to me.

Patch didn't apply to HEAD nor on top of any of the previous commits either,
but I couldn't figure out why based on the .rej files, strange.
I copy/pasted the parts from the patch to test it. Let me know if you need a
rebased version of it, in case you will need to do the same, to save some work.

Also noted the below in your last commit:

    a6417078c414 has introduced as project policy that new features
    committed during the development cycle should use new OIDs in the
    [8000,9999] range.

    4564f1cebd43 did not respect that rule, so let's renumber pg_get_acl()
    to use an OID in the correct range.

Thanks for the info!
Will make sure to use the `src/include/catalog/renumber_oids.pl` tool
for future patches.

Regards,
Joel

On Mon, Jul 08, 2024 at 11:55:28AM +0200, Joel Jacobson wrote:
> Patch didn't apply to HEAD nor on top of any of the previous commits either,
> but I couldn't figure out why based on the .rej files, strange.
> I copy/pasted the parts from the patch to test it. Let me know if you need a
> rebased version of it, in case you will need to do the same, to save some work.

Strange.  I have just downloaded v2 again.  `patch -p1` and `git am`
are both working here.

> Also noted the below in your last commit:
>
>     a6417078c414 has introduced as project policy that new features
>     committed during the development cycle should use new OIDs in the
>     [8000,9999] range.
>
>     4564f1cebd43 did not respect that rule, so let's renumber pg_get_acl()
>     to use an OID in the correct range.
>
> Thanks for the info!
> Will make sure to use the `src/include/catalog/renumber_oids.pl` tool
> for future patches.

No problem.  This is the kind of tweaks that are really easy to
forget.
--
Michael

On Mon, Jul 08, 2024 at 11:55:28AM +0200, Joel Jacobson wrote:
> Thanks, nice simplifications.
> I agree the tests you removed are not that interesting.
>
> Looks good to me.

On second review, I have spotted a use-after-free for the case of an
attribute ACL in both v1 and v2, as we would finish by releasing the
syscache entry before returning the ACL datum.  For builds with the
flags -DRELCACHE_FORCE_RELEASE -DCATCACHE_FORCE_RELEASE, like the
buildfarm member prion, this would cause lookup failures in the
function when building the result because the datum is borked.

At the end, I have settled down on using SearchSysCacheCopyAttNum().
It requires a heap_freetuple(), which we already don't care much about
in the existing callers of get_catalog_object_by_oid() because these
are short-lived, with bonus points because this routine uses
SearchSysCacheAttNum() that has a check for attisdropped, making the
code of pg_get_acl() even simpler.

The usual trick I use to check a fix with this configuration is to use
two builds because initdb is insanely slow if caches are released: one
with the flags and one without them.  I have used the build without
the flags to initialize a cluster with the objects and their ACLs.
Then, the second build is used only to execute all the scenarios of
pg_get_acl(), which are much cheaper to execute.
--
Michael