Date: Wed, 12 Mar 2025 13:19:09 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: =?iso-8859-1?Q?=C1lvaro?= Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@postgresql.org
Subject: Re: remove open-coded popcount in acl.c
Message-ID: <Z9HQHX21PVAK-xea@nathan>
References: <Z9GtL7Nm6hsYyJnF@nathan>
 <202503121623.i4enp6airsfl@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="1vQ1UQFsHousOb+M"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202503121623.i4enp6airsfl@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/Z9HQHX21PVAK-xea%40nathan>
Precedence: bulk


--1vQ1UQFsHousOb+M
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Wed, Mar 12, 2025 at 05:23:25PM +0100, Álvaro Herrera wrote:
> Strange: this code is not covered by any tests.
> 
> https://coverage.postgresql.org/src/backend/utils/adt/acl.c.gcov.html#5533
> https://coverage.postgresql.org/src/backend/utils/adt/acl.c.gcov.html#5438

Huh.  Well, it's easy enough to add some basic tests for the grantor
selection machinery.  Here's a first try.

-- 
nathan

--1vQ1UQFsHousOb+M
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Remove-open-coded-popcount-in-acl.c.patch"

From d3cf9ca237f647ebcca20c55c8302f00f716c459 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 12 Mar 2025 10:45:12 -0500
Subject: [PATCH v2 1/1] Remove open-coded popcount in acl.c.

---
 src/backend/utils/adt/acl.c              | 20 +-----------------
 src/test/regress/expected/privileges.out | 27 ++++++++++++++++++++++++
 src/test/regress/sql/privileges.sql      | 22 +++++++++++++++++++
 3 files changed, 50 insertions(+), 19 deletions(-)

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 6a76550a5e2..ba14713fef2 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -5432,24 +5432,6 @@ select_best_admin(Oid member, Oid role)
 	return admin_role;
 }
 
-
-/* does what it says ... */
-static int
-count_one_bits(AclMode mask)
-{
-	int			nbits = 0;
-
-	/* this code relies on AclMode being an unsigned type */
-	while (mask)
-	{
-		if (mask & 1)
-			nbits++;
-		mask >>= 1;
-	}
-	return nbits;
-}
-
-
 /*
  * Select the effective grantor ID for a GRANT or REVOKE operation.
  *
@@ -5532,7 +5514,7 @@ select_best_grantor(Oid roleId, AclMode privileges,
 		 */
 		if (otherprivs != ACL_NO_RIGHTS)
 		{
-			int			nnewrights = count_one_bits(otherprivs);
+			int			nnewrights = pg_popcount64(otherprivs);
 
 			if (nnewrights > nrights)
 			{
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index a76256405fe..954f549555e 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -3293,3 +3293,30 @@ DROP SCHEMA reindex_test;
 DROP ROLE regress_no_maintain;
 DROP ROLE regress_maintain;
 DROP ROLE regress_maintain_all;
+-- grantor selection
+CREATE ROLE regress_grantor1;
+CREATE ROLE regress_grantor2 ROLE regress_grantor1;
+CREATE ROLE regress_grantor3;
+CREATE TABLE grantor_test1 ();
+CREATE TABLE grantor_test2 ();
+CREATE TABLE grantor_test3 ();
+GRANT SELECT ON grantor_test2 TO regress_grantor1 WITH GRANT OPTION;
+GRANT SELECT, UPDATE ON grantor_test3 TO regress_grantor2 WITH GRANT OPTION;
+SET ROLE regress_grantor1;
+GRANT SELECT, UPDATE ON grantor_test1 TO regress_grantor3;
+ERROR:  permission denied for table grantor_test1
+GRANT SELECT, UPDATE ON grantor_test2 TO regress_grantor3;
+WARNING:  not all privileges were granted for "grantor_test2"
+GRANT SELECT, UPDATE ON grantor_test3 TO regress_grantor3;
+RESET ROLE;
+SELECT * FROM information_schema.table_privileges t
+	WHERE grantor LIKE 'regress_grantor%' ORDER BY ROW(t.*);
+     grantor      |     grantee      | table_catalog | table_schema |  table_name   | privilege_type | is_grantable | with_hierarchy 
+------------------+------------------+---------------+--------------+---------------+----------------+--------------+----------------
+ regress_grantor1 | regress_grantor3 | regression    | public       | grantor_test2 | SELECT         | NO           | YES
+ regress_grantor2 | regress_grantor3 | regression    | public       | grantor_test3 | SELECT         | NO           | YES
+ regress_grantor2 | regress_grantor3 | regression    | public       | grantor_test3 | UPDATE         | NO           | NO
+(3 rows)
+
+DROP TABLE grantor_test1, grantor_test2, grantor_test3;
+DROP ROLE regress_grantor1, regress_grantor2, regress_grantor3;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index d195aaf1377..b81694c24f2 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -2042,3 +2042,25 @@ DROP SCHEMA reindex_test;
 DROP ROLE regress_no_maintain;
 DROP ROLE regress_maintain;
 DROP ROLE regress_maintain_all;
+
+-- grantor selection
+CREATE ROLE regress_grantor1;
+CREATE ROLE regress_grantor2 ROLE regress_grantor1;
+CREATE ROLE regress_grantor3;
+CREATE TABLE grantor_test1 ();
+CREATE TABLE grantor_test2 ();
+CREATE TABLE grantor_test3 ();
+GRANT SELECT ON grantor_test2 TO regress_grantor1 WITH GRANT OPTION;
+GRANT SELECT, UPDATE ON grantor_test3 TO regress_grantor2 WITH GRANT OPTION;
+
+SET ROLE regress_grantor1;
+GRANT SELECT, UPDATE ON grantor_test1 TO regress_grantor3;
+GRANT SELECT, UPDATE ON grantor_test2 TO regress_grantor3;
+GRANT SELECT, UPDATE ON grantor_test3 TO regress_grantor3;
+RESET ROLE;
+
+SELECT * FROM information_schema.table_privileges t
+	WHERE grantor LIKE 'regress_grantor%' ORDER BY ROW(t.*);
+
+DROP TABLE grantor_test1, grantor_test2, grantor_test3;
+DROP ROLE regress_grantor1, regress_grantor2, regress_grantor3;
-- 
2.39.5 (Apple Git-154)


--1vQ1UQFsHousOb+M--