Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 8 Jul 2024 17:34:18 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Joel Jacobson <joel@compiler.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pgsql: Add pg_get_acl() to get the ACL for a database object
Message-ID: <ZoukijRBef4m-VMO@paquier.xyz>
References: <E1sPHWu-000GPc-0z@gemulon.postgresql.org>
 <3408967.1720107891@sss.pgh.pa.us>
 <f2688fb7-add6-43da-b1df-97312a451893@app.fastmail.com>
 <ZoctyZzE3UlUSer_@paquier.xyz>
 <f2539bff-64be-47f0-9f0b-df85d3cc0432@app.fastmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="/DofafYcS8KBaGY/"
Content-Disposition: inline
In-Reply-To: <f2539bff-64be-47f0-9f0b-df85d3cc0432@app.fastmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZoukijRBef4m-VMO%40paquier.xyz>
Precedence: bulk


--/DofafYcS8KBaGY/
Content-Type: multipart/mixed; boundary="qPzVYTtjzI1oPOnz"
Content-Disposition: inline


--qPzVYTtjzI1oPOnz
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> There are now two different code paths for columns and non-columns.
>=20
> 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

--qPzVYTtjzI1oPOnz
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Extend-pg_get_acl-to-handle-sub-object-IDs.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 07b65a78fb660f6a27ef464870a3e27f788c5e28 Mon Sep 17 00:00:00 2001
=46rom: 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 @@
=20
 { oid =3D> '8730', descr =3D> 'get ACL for SQL object',
   proname =3D> 'pg_get_acl', provolatile =3D> 's', prorettype =3D> '_aclit=
em',
-  proargtypes =3D> 'oid oid', proargnames =3D> '{classid,objid}',
+  proargtypes =3D> 'oid oid int4', proargnames =3D> '{classid,objid,objsub=
id}',
   prosrc =3D> 'pg_get_acl' },
=20
 { oid =3D> '3839',
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/obje=
ctaddress.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)
=20
 /*
  * SQL-level callable function to obtain the ACL of a specified object, gi=
ven
- * its catalog OID and object OID.
+ * its catalog OID, object OID and sub-object ID.
  */
 Datum
 pg_get_acl(PG_FUNCTION_ARGS)
 {
 	Oid			classId =3D PG_GETARG_OID(0);
 	Oid			objectId =3D PG_GETARG_OID(1);
+	int32		objsubid =3D PG_GETARG_INT32(2);
 	Oid			catalogId;
 	AttrNumber	Anum_acl;
-	Relation	rel;
-	HeapTuple	tup;
 	Datum		datum;
 	bool		isnull;
+	HeapTuple	tup;
=20
 	/* 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 =3D=3D InvalidAttrNumber)
 		PG_RETURN_NULL();
=20
-	rel =3D table_open(catalogId, AccessShareLock);
-
-	tup =3D 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 =3D=3D RelationRelationId && objsubid !=3D 0)
 	{
-		table_close(rel, AccessShareLock);
-		PG_RETURN_NULL();
-	}
+		AttrNumber	attnum =3D (AttrNumber) objsubid;
+		Form_pg_attribute atttup;
=20
-	datum =3D heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);
-	table_close(rel, AccessShareLock);
+		tup =3D SearchSysCache2(ATTNUM, ObjectIdGetDatum(objectId),
+							  Int16GetDatum(attnum));
+
+		if (!HeapTupleIsValid(tup))
+			PG_RETURN_NULL();
+
+		atttup =3D (Form_pg_attribute) GETSTRUCT(tup);
+
+		/* ignore dropped columns */
+		if (atttup->attisdropped)
+		{
+			ReleaseSysCache(tup);
+			PG_RETURN_NULL();
+		}
+
+		datum =3D SysCacheGetAttr(ATTNUM, tup,
+								Anum_pg_attribute_attacl,
+								&isnull);
+		ReleaseSysCache(tup);
+	}
+	else
+	{
+		Relation	rel;
+
+		rel =3D table_open(catalogId, AccessShareLock);
+
+		tup =3D get_catalog_object_by_oid(rel, get_object_attnum_oid(catalogId),
+										objectId);
+		if (!HeapTupleIsValid(tup))
+		{
+			table_close(rel, AccessShareLock);
+			PG_RETURN_NULL();
+		}
+
+		datum =3D heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);
+		table_close(rel, AccessShareLock);
+	}
=20
 	if (isnull)
 		PG_RETURN_NULL();
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/ex=
pected/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)
=20
 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=20
 ------------
 =20
@@ -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                    =20
 ------------------------------------------------
  regress_priv_user1=3DarwdDxtm/regress_priv_user1
@@ -234,13 +234,13 @@ SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest=
2'::regclass::oid));
 (5 rows)
=20
 -- Invalid inputs
-SELECT pg_get_acl('pg_class'::regclass, 0); -- null
+SELECT pg_get_acl('pg_class'::regclass, 0, 0); -- null
  pg_get_acl=20
 ------------
 =20
 (1 row)
=20
-SELECT pg_get_acl(0, 0); -- null
+SELECT pg_get_acl(0, 0, 0); -- null
  pg_get_acl=20
 ------------
 =20
@@ -653,6 +653,30 @@ CREATE TABLE atest5 (one int, two int unique, three in=
t, 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                  =20
+--------------------------------------------
+ regress_priv_user4=3Dr/regress_priv_user1
+ regress_priv_user3=3Darwx/regress_priv_user1
+(2 rows)
+
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2)=
);
+                 unnest                 =20
+-----------------------------------------
+ regress_priv_user4=3Da/regress_priv_user1
+(1 row)
+
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3)=
);
+                 unnest                 =20
+-----------------------------------------
+ regress_priv_user4=3Dw/regress_priv_user1
+(1 row)
+
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4)=
);
+ unnest=20
+--------
+(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/pri=
vileges.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;
=20
 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)=
);
=20
 -- 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
=20
 GRANT TRUNCATE ON atest2 TO regress_priv_user4 GRANTED BY regress_priv_use=
r5;  -- error
=20
@@ -439,6 +439,10 @@ CREATE TABLE atest5 (one int, two int unique, three in=
t, 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)=
);
=20
 INSERT INTO atest5 VALUES (1,2,3);
=20
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=3D# 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 =3D current_database() AND
--=20
2.45.2


--qPzVYTtjzI1oPOnz--

--/DofafYcS8KBaGY/
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmaLpIoACgkQnvQgOdby
QH1DYw/+JoKviZtNqtWfCXZu3QaD7V/R4I7J/Q3gdg+oNqwonnV16IJ3eBZiU2ds
TQYao4sVwBwkkO+J9rYbzDiegfyC2603mwOdMZxfr02Yboavi3f5o1GKNNCDhSBn
yKYwuWhZV0lYw2ycNepDn6w9miD7cx4MXdOv0Wi0LQtP+f0+zEQcy2KMNYZJY9qF
GKlUUf3moDEkMMZnY5d0lu2ZTRif6FCLVkYicg4txe7KColiq72x2Ms6V0B8um0B
9Z0rGhJE00b23IU5ohr5C99B7n4YvYMl2sW2OyB1eV3xsoNtPSLl4N9V4NtYr5IC
9sfRAciysLP/8UG4+yXyDZoVdigXYYcZhCDmRWC53Inr4T/wGwdFuF8nmgXSxtX4
cxIw0AWRxRFaHHbK/Hx4OEGN0zLFtmUatejQT554SBy0EIcT5od08K3V6R4c1ZZq
XRWN+jNkI16VC6DdFp+utr99bLQr4/4Bn0kVlGmmiA8hHlYVeNyYZfhWGPH+Voy/
CWu/XtkFSZ/TH3v9X9G2UE6gBuIrOSoClIb6xuaMHcyqSuFCGv9VNP5F+BwUsYSt
LTeOxFoQXlDDzCRJX7PGi4Ob8jkICbw0r5ito5wP1yIa+1IFfpcWz33cz5CcQPCg
//au+3g1W+Xy2Vbpm9YwO+4/p0169S6/KdR7PaOPIRvywuAYdJs=
=4833
-----END PGP SIGNATURE-----

--/DofafYcS8KBaGY/--