pgsql: Add pg_get_acl() to get the ACL for a database object
Add pg_get_acl() to get the ACL for a database object
This function returns the ACL for a database object, specified by
catalog OID and object OID. This is useful to be able to
retrieve the ACL associated to an object specified with a
(class_id,objid) couple, similarly to the other functions for object
identification, when joined with pg_depend or pg_shdepend.
Original idea by Álvaro Herrera.
Bump catalog version.
Author: Joel Jacobson
Reviewed-by: Isaac Morland, Michael Paquier, Ranier Vilela
Discussion: /messages/by-id/80b16434-b9b1-4c3d-8f28-569f21c2c102@app.fastmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/4564f1cebd437d93590027c9ff46ef60bc3286ae
Modified Files
--------------
doc/src/sgml/func.sgml | 41 +++++++++++++++++++++++++++
src/backend/catalog/objectaddress.c | 48 ++++++++++++++++++++++++++++++++
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat | 5 ++++
src/test/regress/expected/privileges.out | 29 +++++++++++++++++++
src/test/regress/sql/privileges.sql | 6 ++++
6 files changed, 130 insertions(+), 1 deletion(-)
Michael Paquier <michael@paquier.xyz> writes:
Add pg_get_acl() to get the ACL for a database object
This function returns the ACL for a database object, specified by
catalog OID and object OID.
Uh, why is it defined like that rather than allowing a subobject?
This definition is unable to fetch column-specific ACLs.
regards, tom lane
On Thu, Jul 4, 2024, at 17:44, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
Add pg_get_acl() to get the ACL for a database object
This function returns the ACL for a database object, specified by
catalog OID and object OID.Uh, why is it defined like that rather than allowing a subobject?
This definition is unable to fetch column-specific ACLs.
Good point, that's surely an important missing feature,
that I hadn't thought about up until now.
Probably because all object classes, except columns, don't have subobjects.
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.
Why I think this could be motivated, is since during discussion,
some even wanted reg* overloads, to avoid having to pass the class OID.
As a middle ground, maybe users would appreciate if they at least
didn't have pass in the extra 0, since it's meaningless anyway,
most of the times (for all classes except columns)?
Anyway, that's just an idea. We still need support for subobject,
so I had a look on how to implement it.
Unfortunately, the AlterObjectOwner_internal function in alter.c,
which pg_get_acl in objectaddress.c is based upon,
doesn't deal with subobjects.
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?
I guess we need to handle the RelationRelationId separately,
and handle all other classes using the current code in pg_get_acl()?
Regards,
Joel
(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
Attachments:
0001-Extend-pg_get_acl-to-handle-sub-object-IDs.patchapplication/octet-stream; name="=?UTF-8?Q?0001-Extend-pg=5Fget=5Facl-to-handle-sub-object-IDs.patch?="Download
From 6245502f9fc981d613c70016b45d6e2674d7d0cb Mon Sep 17 00:00:00 2001
From: Joel Jakobsson <github@compiler.org>
Date: Thu, 4 Jul 2024 22:56:37 +0200
Subject: [PATCH] Extend pg_get_acl to handle sub-object IDs.
This patch modifies the pg_get_acl function to accept a third objsubid param.
This enables the retrieval of ACLs for columns within a table.
Detailed changes:
- Updated func.sgml to document the new parameter and its usage.
- Modified pg_get_acl function in objectaddress.c to handle sub-object IDs.
- Adjusted the catalog entry in pg_proc.dat to include the new parameter.
- Enhanced regression tests in privileges.sql to test the new functionality.
---
doc/src/sgml/func.sgml | 6 +--
src/backend/catalog/objectaddress.c | 67 +++++++++++++++++++-----
src/include/catalog/pg_proc.dat | 2 +-
src/test/regress/expected/privileges.out | 56 ++++++++++++++++++--
src/test/regress/sql/privileges.sql | 16 ++++--
5 files changed, 123 insertions(+), 24 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 93ee3d4b60..aad944979f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -26592,12 +26592,12 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
<indexterm>
<primary>pg_get_acl</primary>
</indexterm>
- <function>pg_get_acl</function> ( <parameter>classid</parameter> <type>oid</type>, <parameter>objid</parameter> <type>oid</type> )
+ <function>pg_get_acl</function> ( <parameter>classid</parameter> <type>oid</type>, <parameter>objid</parameter> <type>oid</type>, <parameter>objsubid</parameter> <type>integer</type> )
<returnvalue>aclitem[]</returnvalue>
</para>
<para>
Returns the <acronym>ACL</acronym> for a database object, specified
- by catalog OID and object OID. This function returns
+ by catalog OID, object OID and sub-object ID. This function returns
<literal>NULL</literal> values for undefined objects.
</para></entry>
</row>
@@ -26723,7 +26723,7 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
<programlisting>
postgres=# SELECT
(pg_identify_object(s.classid,s.objid,s.objsubid)).*,
- pg_catalog.pg_get_acl(s.classid,s.objid) AS acl
+ pg_catalog.pg_get_acl(s.classid,s.objid,s.objsubid) AS acl
FROM pg_catalog.pg_shdepend AS s
JOIN pg_catalog.pg_database AS d
ON d.datname = current_database() AND
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 2983b9180f..659512b254 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -4364,13 +4364,14 @@ pg_identify_object_as_address(PG_FUNCTION_ARGS)
/*
* SQL-level callable function to obtain the ACL of a specified object, given
- * its catalog OID and object OID.
+ * its catalog OID, object OID and sub-object ID.
*/
Datum
pg_get_acl(PG_FUNCTION_ARGS)
{
Oid classId = PG_GETARG_OID(0);
Oid objectId = PG_GETARG_OID(1);
+ int32 objsubid = PG_GETARG_INT32(2);
Oid catalogId;
AttrNumber Anum_acl;
Relation rel;
@@ -4391,21 +4392,63 @@ pg_get_acl(PG_FUNCTION_ARGS)
if (Anum_acl == InvalidAttrNumber)
PG_RETURN_NULL();
- rel = table_open(catalogId, AccessShareLock);
-
- tup = get_catalog_object_by_oid(rel, get_object_attnum_oid(catalogId),
- objectId);
- if (!HeapTupleIsValid(tup))
+ /*
+ * The procedure to retrive the ACL is different if the object is a table
+ * and the sub-object is non-zero, which means we're dealing with a column
+ * of a table. In this case, the objsubid is the attnum for the column.
+ */
+ if (classId == RelationRelationId && objsubid != 0)
{
- table_close(rel, AccessShareLock);
- PG_RETURN_NULL();
+ AttrNumber attnum = (AttrNumber) objsubid;
+
+ HeapTuple attTuple;
+ bool isNull;
+
+ attTuple = SearchSysCache2(ATTNUM,
+ ObjectIdGetDatum(objectId),
+ Int16GetDatum(attnum));
+
+ if (!HeapTupleIsValid(attTuple))
+ PG_RETURN_NULL();
+
+ /* ignore dropped columns */
+ if (((Form_pg_attribute) GETSTRUCT(attTuple))->attisdropped)
+ {
+ ReleaseSysCache(attTuple);
+ PG_RETURN_NULL();
+ }
+
+ datum = SysCacheGetAttr(ATTNUM, attTuple,
+ Anum_pg_attribute_attacl,
+ &isNull);
+
+ /* no need to do anything for a NULL ACL */
+ if (isNull)
+ {
+ ReleaseSysCache(attTuple);
+ PG_RETURN_NULL();
+ }
+
+ ReleaseSysCache(attTuple);
}
+ else
+ {
+ rel = table_open(catalogId, AccessShareLock);
- datum = heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);
- table_close(rel, AccessShareLock);
+ tup = get_catalog_object_by_oid(rel, get_object_attnum_oid(catalogId),
+ objectId);
+ if (!HeapTupleIsValid(tup))
+ {
+ table_close(rel, AccessShareLock);
+ PG_RETURN_NULL();
+ }
- if (isnull)
- PG_RETURN_NULL();
+ datum = heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);
+ table_close(rel, AccessShareLock);
+
+ if (isnull)
+ PG_RETURN_NULL();
+ }
PG_RETURN_DATUM(datum);
}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index e1001a4822..9654b3ba4e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6364,7 +6364,7 @@
{ oid => '6347', descr => 'get ACL for SQL object',
proname => 'pg_get_acl', provolatile => 's', prorettype => '_aclitem',
- proargtypes => 'oid oid', proargnames => '{classid,objid}',
+ proargtypes => 'oid oid int4', proargnames => '{classid,objid,objsubid}',
prosrc => 'pg_get_acl' },
{ oid => '3839',
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 332bc584eb..c904cbef84 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -213,7 +213,7 @@ SELECT * FROM atest1;
(0 rows)
CREATE TABLE atest2 (col1 varchar(10), col2 boolean);
-SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);
+SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0);
pg_get_acl
------------
@@ -223,7 +223,7 @@ GRANT SELECT ON atest2 TO regress_priv_user2;
GRANT UPDATE ON atest2 TO regress_priv_user3;
GRANT INSERT ON atest2 TO regress_priv_user4 GRANTED BY CURRENT_USER;
GRANT TRUNCATE ON atest2 TO regress_priv_user5 GRANTED BY CURRENT_ROLE;
-SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0));
unnest
------------------------------------------------
regress_priv_user1=arwdDxtm/regress_priv_user1
@@ -234,13 +234,13 @@ SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid));
(5 rows)
-- Invalid inputs
-SELECT pg_get_acl('pg_class'::regclass, 0); -- null
+SELECT pg_get_acl('pg_class'::regclass, 0, 0); -- null
pg_get_acl
------------
(1 row)
-SELECT pg_get_acl(0, 0); -- null
+SELECT pg_get_acl(0, 0, 0); -- null
pg_get_acl
------------
@@ -651,8 +651,56 @@ ERROR: permission denied for table atest2
SET SESSION AUTHORIZATION regress_priv_user1;
CREATE TABLE atest5 (one int, two int unique, three int, four int unique);
CREATE TABLE atest6 (one int, two int, blue int);
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1);
+ pg_get_acl
+------------
+
+(1 row)
+
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2);
+ pg_get_acl
+------------
+
+(1 row)
+
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3);
+ pg_get_acl
+------------
+
+(1 row)
+
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4);
+ pg_get_acl
+------------
+
+(1 row)
+
GRANT SELECT (one), INSERT (two), UPDATE (three) ON atest5 TO regress_priv_user4;
GRANT ALL (one) ON atest5 TO regress_priv_user3;
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1));
+ unnest
+--------------------------------------------
+ regress_priv_user4=r/regress_priv_user1
+ regress_priv_user3=arwx/regress_priv_user1
+(2 rows)
+
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2));
+ unnest
+-----------------------------------------
+ regress_priv_user4=a/regress_priv_user1
+(1 row)
+
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3));
+ unnest
+-----------------------------------------
+ regress_priv_user4=w/regress_priv_user1
+(1 row)
+
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4));
+ unnest
+--------
+(0 rows)
+
INSERT INTO atest5 VALUES (1,2,3);
SET SESSION AUTHORIZATION regress_priv_user4;
SELECT * FROM atest5; -- fail
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 980d19bde5..ea5c93cc57 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -183,16 +183,16 @@ GRANT SELECT ON atest1 TO regress_priv_user3, regress_priv_user4;
SELECT * FROM atest1;
CREATE TABLE atest2 (col1 varchar(10), col2 boolean);
-SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);
+SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0);
GRANT SELECT ON atest2 TO regress_priv_user2;
GRANT UPDATE ON atest2 TO regress_priv_user3;
GRANT INSERT ON atest2 TO regress_priv_user4 GRANTED BY CURRENT_USER;
GRANT TRUNCATE ON atest2 TO regress_priv_user5 GRANTED BY CURRENT_ROLE;
-SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0));
-- Invalid inputs
-SELECT pg_get_acl('pg_class'::regclass, 0); -- null
-SELECT pg_get_acl(0, 0); -- null
+SELECT pg_get_acl('pg_class'::regclass, 0, 0); -- null
+SELECT pg_get_acl(0, 0, 0); -- null
GRANT TRUNCATE ON atest2 TO regress_priv_user4 GRANTED BY regress_priv_user5; -- error
@@ -437,8 +437,16 @@ SELECT * FROM atestv2; -- fail (even though regress_priv_user2 can access underl
SET SESSION AUTHORIZATION regress_priv_user1;
CREATE TABLE atest5 (one int, two int unique, three int, four int unique);
CREATE TABLE atest6 (one int, two int, blue int);
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1);
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2);
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3);
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4);
GRANT SELECT (one), INSERT (two), UPDATE (three) ON atest5 TO regress_priv_user4;
GRANT ALL (one) ON atest5 TO regress_priv_user3;
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4));
INSERT INTO atest5 VALUES (1,2,3);
--
2.45.1
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
Attachments:
v2-0001-Extend-pg_get_acl-to-handle-sub-object-IDs.patchtext/x-diff; charset=us-asciiDownload
From 07b65a78fb660f6a27ef464870a3e27f788c5e28 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 8 Jul 2024 16:24:42 +0900
Subject: [PATCH v2] Extend pg_get_acl to handle sub-object IDs.
This patch modifies the pg_get_acl function to accept a third objsubid
param. This enables the retrieval of ACLs for columns within a table.
---
src/include/catalog/pg_proc.dat | 2 +-
src/backend/catalog/objectaddress.c | 60 +++++++++++++++++++-----
src/test/regress/expected/privileges.out | 32 +++++++++++--
src/test/regress/sql/privileges.sql | 12 +++--
doc/src/sgml/func.sgml | 6 +--
5 files changed, 87 insertions(+), 25 deletions(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 0bf413fe05..0a0f6aa198 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6364,7 +6364,7 @@
{ oid => '8730', descr => 'get ACL for SQL object',
proname => 'pg_get_acl', provolatile => 's', prorettype => '_aclitem',
- proargtypes => 'oid oid', proargnames => '{classid,objid}',
+ proargtypes => 'oid oid int4', proargnames => '{classid,objid,objsubid}',
prosrc => 'pg_get_acl' },
{ oid => '3839',
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 2983b9180f..fac3922964 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -4364,19 +4364,19 @@ pg_identify_object_as_address(PG_FUNCTION_ARGS)
/*
* SQL-level callable function to obtain the ACL of a specified object, given
- * its catalog OID and object OID.
+ * its catalog OID, object OID and sub-object ID.
*/
Datum
pg_get_acl(PG_FUNCTION_ARGS)
{
Oid classId = PG_GETARG_OID(0);
Oid objectId = PG_GETARG_OID(1);
+ int32 objsubid = PG_GETARG_INT32(2);
Oid catalogId;
AttrNumber Anum_acl;
- Relation rel;
- HeapTuple tup;
Datum datum;
bool isnull;
+ HeapTuple tup;
/* for "pinned" items in pg_depend, return null */
if (!OidIsValid(classId) && !OidIsValid(objectId))
@@ -4391,18 +4391,52 @@ pg_get_acl(PG_FUNCTION_ARGS)
if (Anum_acl == InvalidAttrNumber)
PG_RETURN_NULL();
- rel = table_open(catalogId, AccessShareLock);
-
- tup = get_catalog_object_by_oid(rel, get_object_attnum_oid(catalogId),
- objectId);
- if (!HeapTupleIsValid(tup))
+ /*
+ * If dealing with a relation's attribute (objsubid is set), the ACL is
+ * retrieved from pg_attribute.
+ */
+ if (classId == RelationRelationId && objsubid != 0)
{
- table_close(rel, AccessShareLock);
- PG_RETURN_NULL();
- }
+ AttrNumber attnum = (AttrNumber) objsubid;
+ Form_pg_attribute atttup;
- datum = heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);
- table_close(rel, AccessShareLock);
+ tup = SearchSysCache2(ATTNUM, ObjectIdGetDatum(objectId),
+ Int16GetDatum(attnum));
+
+ if (!HeapTupleIsValid(tup))
+ PG_RETURN_NULL();
+
+ atttup = (Form_pg_attribute) GETSTRUCT(tup);
+
+ /* ignore dropped columns */
+ if (atttup->attisdropped)
+ {
+ ReleaseSysCache(tup);
+ PG_RETURN_NULL();
+ }
+
+ datum = SysCacheGetAttr(ATTNUM, tup,
+ Anum_pg_attribute_attacl,
+ &isnull);
+ ReleaseSysCache(tup);
+ }
+ else
+ {
+ Relation rel;
+
+ rel = table_open(catalogId, AccessShareLock);
+
+ tup = get_catalog_object_by_oid(rel, get_object_attnum_oid(catalogId),
+ objectId);
+ if (!HeapTupleIsValid(tup))
+ {
+ table_close(rel, AccessShareLock);
+ PG_RETURN_NULL();
+ }
+
+ datum = heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);
+ table_close(rel, AccessShareLock);
+ }
if (isnull)
PG_RETURN_NULL();
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 332bc584eb..fab0cc800f 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -213,7 +213,7 @@ SELECT * FROM atest1;
(0 rows)
CREATE TABLE atest2 (col1 varchar(10), col2 boolean);
-SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);
+SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0);
pg_get_acl
------------
@@ -223,7 +223,7 @@ GRANT SELECT ON atest2 TO regress_priv_user2;
GRANT UPDATE ON atest2 TO regress_priv_user3;
GRANT INSERT ON atest2 TO regress_priv_user4 GRANTED BY CURRENT_USER;
GRANT TRUNCATE ON atest2 TO regress_priv_user5 GRANTED BY CURRENT_ROLE;
-SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0));
unnest
------------------------------------------------
regress_priv_user1=arwdDxtm/regress_priv_user1
@@ -234,13 +234,13 @@ SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid));
(5 rows)
-- Invalid inputs
-SELECT pg_get_acl('pg_class'::regclass, 0); -- null
+SELECT pg_get_acl('pg_class'::regclass, 0, 0); -- null
pg_get_acl
------------
(1 row)
-SELECT pg_get_acl(0, 0); -- null
+SELECT pg_get_acl(0, 0, 0); -- null
pg_get_acl
------------
@@ -653,6 +653,30 @@ CREATE TABLE atest5 (one int, two int unique, three int, four int unique);
CREATE TABLE atest6 (one int, two int, blue int);
GRANT SELECT (one), INSERT (two), UPDATE (three) ON atest5 TO regress_priv_user4;
GRANT ALL (one) ON atest5 TO regress_priv_user3;
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1));
+ unnest
+--------------------------------------------
+ regress_priv_user4=r/regress_priv_user1
+ regress_priv_user3=arwx/regress_priv_user1
+(2 rows)
+
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2));
+ unnest
+-----------------------------------------
+ regress_priv_user4=a/regress_priv_user1
+(1 row)
+
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3));
+ unnest
+-----------------------------------------
+ regress_priv_user4=w/regress_priv_user1
+(1 row)
+
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4));
+ unnest
+--------
+(0 rows)
+
INSERT INTO atest5 VALUES (1,2,3);
SET SESSION AUTHORIZATION regress_priv_user4;
SELECT * FROM atest5; -- fail
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 980d19bde5..ae338e8cc8 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -183,16 +183,16 @@ GRANT SELECT ON atest1 TO regress_priv_user3, regress_priv_user4;
SELECT * FROM atest1;
CREATE TABLE atest2 (col1 varchar(10), col2 boolean);
-SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);
+SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0);
GRANT SELECT ON atest2 TO regress_priv_user2;
GRANT UPDATE ON atest2 TO regress_priv_user3;
GRANT INSERT ON atest2 TO regress_priv_user4 GRANTED BY CURRENT_USER;
GRANT TRUNCATE ON atest2 TO regress_priv_user5 GRANTED BY CURRENT_ROLE;
-SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0));
-- Invalid inputs
-SELECT pg_get_acl('pg_class'::regclass, 0); -- null
-SELECT pg_get_acl(0, 0); -- null
+SELECT pg_get_acl('pg_class'::regclass, 0, 0); -- null
+SELECT pg_get_acl(0, 0, 0); -- null
GRANT TRUNCATE ON atest2 TO regress_priv_user4 GRANTED BY regress_priv_user5; -- error
@@ -439,6 +439,10 @@ CREATE TABLE atest5 (one int, two int unique, three int, four int unique);
CREATE TABLE atest6 (one int, two int, blue int);
GRANT SELECT (one), INSERT (two), UPDATE (three) ON atest5 TO regress_priv_user4;
GRANT ALL (one) ON atest5 TO regress_priv_user3;
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3));
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4));
INSERT INTO atest5 VALUES (1,2,3);
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 93ee3d4b60..aad944979f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -26592,12 +26592,12 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
<indexterm>
<primary>pg_get_acl</primary>
</indexterm>
- <function>pg_get_acl</function> ( <parameter>classid</parameter> <type>oid</type>, <parameter>objid</parameter> <type>oid</type> )
+ <function>pg_get_acl</function> ( <parameter>classid</parameter> <type>oid</type>, <parameter>objid</parameter> <type>oid</type>, <parameter>objsubid</parameter> <type>integer</type> )
<returnvalue>aclitem[]</returnvalue>
</para>
<para>
Returns the <acronym>ACL</acronym> for a database object, specified
- by catalog OID and object OID. This function returns
+ by catalog OID, object OID and sub-object ID. This function returns
<literal>NULL</literal> values for undefined objects.
</para></entry>
</row>
@@ -26723,7 +26723,7 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
<programlisting>
postgres=# SELECT
(pg_identify_object(s.classid,s.objid,s.objsubid)).*,
- pg_catalog.pg_get_acl(s.classid,s.objid) AS acl
+ pg_catalog.pg_get_acl(s.classid,s.objid,s.objsubid) AS acl
FROM pg_catalog.pg_shdepend AS s
JOIN pg_catalog.pg_database AS d
ON d.datname = current_database() AND
--
2.45.2
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