> On Jan 10, 2022, at 2:34 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> In general this looks good. Some nitpicks:
Thanks. Some responses...
> +/*
> + * Ownership check for a role (specified by OID)
> + */
> +bool
> +pg_role_ownercheck(Oid role_oid, Oid roleid)
>
>
> This is a bit confusing. Let's rename these params so it's clear which
> is the owner and which the owned role.
Yeah, I wondered about that when I was writing it. All the neighboring functions follow the pattern:
(Oid <something>_oid, Oid roleid)
so I followed that, but it isn't great. I've changed that in v5-0002 to use
(Oid owned_role_oid, Oid owner_roleid)
I wouldn't choose this naming in a green field, but I'm trying to stay close to the naming scheme of the surrounding
functions.
> + * Note: In versions prior to PostgreSQL version 15, roles did not have
> owners
> + * per se; instead we used this test in places where an ownership-like
> + * permissions test was needed for a role.
>
>
> No need to talk about what we used to do. People who want to know can
> look back at older branches.
Removed in v5-0003.
> +bool
> +has_rolinherit_privilege(Oid roleid)
> +{
>
>
> This and similar functions should have header comments.
Header comments added for this and similar functions in v5-0004. This function was misnamed in prior patch sets; the
privilegeis INHERIT, not ROLINHERIT, so I also fixed the name in v5-0004.
> + /* Owners of roles have every privilge the owned role has */
>
> s/privlge/privilege/
Fixed in v5-0003.
> +CREATE ROLE regress_role_1 CREATEDB CREATEROLE REPLICATION BYPASSRLS;
>
>
> I don't really like this business of just numbering large numbers of
> roles in the tests. Let's give them more meaningful names.
Changed in v5-0001.
> + Role owners can change any of these settings on roles they own except
>
>
> I would say "on roles they directly or indirectly own", here and
> similarly in one or two other places.
Changed a few sentences of doc/src/sgml/ref/alter_role.sgml in v5-0004 as you suggest. Please advise if you have other
locationsin mind. A quick grep -i 'role owner' doesn't show any other relevant locations.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company