Re: CREATEROLE and role ownership hierarchies

> 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 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.