pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[PATCH v1 1/1] Remove open-coded popcount in acl.c.
7+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2025-03-12 025e7e1eb4 Remove count_one_bits() in acl.c.
* [PATCH v1 1/1] Remove open-coded popcount in acl.c.
@ 2025-03-12 15:45 Nathan Bossart <nathan@postgresql.org>
0 siblings, 0 replies; 7+ messages in thread
From: Nathan Bossart @ 2025-03-12 15:45 UTC (permalink / raw)
---
src/backend/utils/adt/acl.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
inline patch 1: src/backend/utils/adt/acl.c (+1/-19 lines) download inline-1.patch
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)
{
--
2.39.5 (Apple Git-154)
* remove open-coded popcount in acl.c
@ 2025-03-12 15:50 Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 16:23 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Bossart @ 2025-03-12 15:50 UTC (permalink / raw)
To: pgsql-hackers
There's a count_one_bits() function in acl.c that can be replaced with a
call to pg_popcount64(). This isn't performance-critical code, but IMHO we
might as well use the centralized implementation.
--
nathan
Attachments:
v1-0001-Remove-open-coded-popcount-in-acl.c.patch (text/plain, 1.2K) download
From 932fac13bf168571b145a54c29d9ac28ca2a070f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 12 Mar 2025 10:45:12 -0500
Subject: [PATCH v1 1/1] Remove open-coded popcount in acl.c.
---
src/backend/utils/adt/acl.c | 20 +-------------------
1 file changed, 1 insertion(+), 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)
{
--
2.39.5 (Apple Git-154)
* Re: remove open-coded popcount in acl.c
2025-03-12 15:50 remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
@ 2025-03-12 16:23 ` Álvaro Herrera <alvherre@alvh.no-ip.org>
2025-03-12 18:19 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Álvaro Herrera @ 2025-03-12 16:23 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
On 2025-Mar-12, Nathan Bossart wrote:
> There's a count_one_bits() function in acl.c that can be replaced with a
> call to pg_popcount64(). This isn't performance-critical code, but IMHO we
> might as well use the centralized implementation.
Makes sense. Patch looks good to me.
> @@ -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);
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
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: remove open-coded popcount in acl.c
2025-03-12 15:50 remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 16:23 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
@ 2025-03-12 18:19 ` Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 18:34 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Bossart @ 2025-03-12 18:19 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-hackers
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.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)
* Re: remove open-coded popcount in acl.c
2025-03-12 15:50 remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 16:23 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
2025-03-12 18:19 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
@ 2025-03-12 18:34 ` Álvaro Herrera <alvherre@alvh.no-ip.org>
2025-03-12 18:35 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Álvaro Herrera @ 2025-03-12 18:34 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
On 2025-Mar-12, Nathan Bossart wrote:
> 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.
Thanks :-) I confirm that this covers the code in select_best_grantor
that you're modifying.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Ed is the standard text editor."
http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: remove open-coded popcount in acl.c
2025-03-12 15:50 remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 16:23 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
2025-03-12 18:19 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 18:34 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
@ 2025-03-12 18:35 ` Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 20:04 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Bossart @ 2025-03-12 18:35 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-hackers
On Wed, Mar 12, 2025 at 07:34:16PM +0100, Álvaro Herrera wrote:
> Thanks :-) I confirm that this covers the code in select_best_grantor
> that you're modifying.
Thanks for the quick review. I'll plan on committing this shortly if CI is
happy.
--
nathan
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: remove open-coded popcount in acl.c
2025-03-12 15:50 remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 16:23 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
2025-03-12 18:19 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 18:34 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
2025-03-12 18:35 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
@ 2025-03-12 20:04 ` Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 7+ messages in thread
From: Nathan Bossart @ 2025-03-12 20:04 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-hackers
On Wed, Mar 12, 2025 at 01:35:39PM -0500, Nathan Bossart wrote:
> Thanks for the quick review. I'll plan on committing this shortly if CI is
> happy.
Committed.
--
nathan
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2025-03-12 20:04 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-03-12 15:45 [PATCH v1 1/1] Remove open-coded popcount in acl.c. Nathan Bossart <nathan@postgresql.org>
2025-03-12 15:50 remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 16:23 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
2025-03-12 18:19 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 18:34 ` Re: remove open-coded popcount in acl.c Álvaro Herrera <alvherre@alvh.no-ip.org>
2025-03-12 18:35 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
2025-03-12 20:04 ` Re: remove open-coded popcount in acl.c Nathan Bossart <nathandbossart@gmail.com>
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