Thread: remove open-coded popcount in acl.c
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
Attachment
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/
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
Attachment
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
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
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