From: Nathan Bossart <nathandbossart@gmail.com>
To: Álvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@postgresql.org
Subject: Re: remove open-coded popcount in acl.c
Date: Wed, 12 Mar 2025 13:19:09 -0500
Message-ID: <Z9HQHX21PVAK-xea@nathan> (raw)
In-Reply-To: <202503121623.i4enp6airsfl@alvherre.pgsql>
References: <Z9GtL7Nm6hsYyJnF@nathan>
<202503121623.i4enp6airsfl@alvherre.pgsql>
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
Attachments:v2-0001-Remove-open-coded-popcount-in-acl.c.patch (text/plain, 4.4K) download
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.cindex 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.outindex 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.sqlindex 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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, alvherre@alvh.no-ip.org
Subject: Re: remove open-coded popcount in acl.c
In-Reply-To: <Z9HQHX21PVAK-xea@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox