Re: Role Attribute Bitmask Catalog Representation

Adam,

* Adam Brightwell (adam(dot)brightwell(at)crunchydatasolutions(dot)com) wrote:
> Attached is an updated patch.

Awesome, thanks!

> diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
> *************** pg_extension_ownercheck(Oid ext_oid, Oid
> *** 5051,5102 ****
> }
>
> /*
> ! * Check whether specified role has CREATEROLE privilege (or is a superuser)
> *
> ! * Note: roles do not have owners per se; instead we use this test in
> ! * places where an ownership-like permissions test is needed for a role.
> ! * Be sure to apply it to the role trying to do the operation, not the
> ! * role being operated on! Also note that this generally should not be
> ! * considered enough privilege if the target role is a superuser.
> ! * (We don't handle that consideration here because we want to give a
> ! * separate error message for such cases, so the caller has to deal with it.)
> */

The comment above is pretty big and I don't think we want to completely
remove it. Can you add it as another 'Note' in the 'has_role_attribute'
comment and reword it accordingly?

> *************** AlterRole(AlterRoleStmt *stmt)
> *** 508,513 ****
> --- 512,518 ----
> DefElem *dvalidUntil = NULL;
> DefElem *dbypassRLS = NULL;
> Oid roleid;
> + RoleAttr attributes;

Whitespace issue that should be fixed- attributes should line up with
roleid.

> --- 1405,1421 ----
> FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, active, xmin, catalog_xmin, restart_lsn)
> LEFT JOIN pg_database d ON ((l.datoid = d.oid)));
> pg_roles| SELECT pg_authid.rolname,
> ! pg_check_role_attribute(pg_authid.oid, 'SUPERUSER'::text) AS rolsuper,
> ! pg_check_role_attribute(pg_authid.oid, 'INHERIT'::text) AS rolinherit,
> ! pg_check_role_attribute(pg_authid.oid, 'CREATEROLE'::text) AS rolcreaterole,
> ! pg_check_role_attribute(pg_authid.oid, 'CREATEDB'::text) AS rolcreatedb,
> ! pg_check_role_attribute(pg_authid.oid, 'CATUPDATE'::text) AS rolcatupdate,
> ! pg_check_role_attribute(pg_authid.oid, 'CANLOGIN'::text) AS rolcanlogin,
> ! pg_check_role_attribute(pg_authid.oid, 'REPLICATION'::text) AS rolreplication,
> ! pg_check_role_attribute(pg_authid.oid, 'BYPASSRLS'::text) AS rolbypassrls,
> pg_authid.rolconnlimit,
> '********'::text AS rolpassword,
> pg_authid.rolvaliduntil,
> s.setconfig AS rolconfig,
> pg_authid.oid
> FROM (pg_authid

It occurs to me that in this case (and a few others..), we're doing a
lot of extra work- each call to pg_check_role_attribute() is doing a
lookup on the oid just to get back to what the rolattr value on this row
is. How about a function which takes rolattr and the text
representation of the attribute and returns if the bit is set for that
rolattr value? Then you'd pass pg_authid.rolattr into the function
calls above instead of the role's oid.

I don't see any changes to the regression test files, were they
forgotten in the patch? I would think that at least the view definition
changes would require updates to the regression tests, though perhaps
nothing else.

Overall, I'm pretty happy with the patch and would suggest moving on to
writing up the documentation changes to go along with the code changes.
I'll continue to play around with it but it all seems pretty clean to
me and will allow us to easily add the additiaonl role attributes being
discussed.

Thanks!

Stephen