Re: CREATEROLE and role ownership hierarchies

> On Jan 10, 2022, at 2:34 PM, Andrew Dunstan <andrew(at)dunslane(dot)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 privilege is 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 locations in mind. A quick grep -i 'role owner' doesn't show any other relevant locations.