Re: Role Attribute Bitmask Catalog Representation

Adam,

* Adam Brightwell (adam(dot)brightwell(at)crunchydatasolutions(dot)com) wrote:
> I have attached an updated patch with initial documentation changes for
> review.

Awesome, thanks.

I'm going to continue looking at this in more detail, but wanted to
mention a few things I noticed in the documentation changes:

> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
> *** a/doc/src/sgml/catalogs.sgml
> --- b/doc/src/sgml/catalogs.sgml
> --- 1391,1493 ----
> </row>
>
> <row>
> ! <entry><structfield>rolattr</structfield></entry>
> ! <entry><type>bigint</type></entry>
> ! <entry>
> ! Role attributes; see <xref linkend="sql-createrole"> for details
> ! </entry>
> ! </row>

Shouldn't this refer to the table below (which I like..)? Or perhaps to
both the table and sql-createrole? Have you had a chance to actually
build the docs and look at the results to see how things look? I should
have time later tomorrow to do that and look over the results also, but
figured I'd ask.

> ! <table id="catalog-rolattr-bitmap-table">
> ! <title><structfield>rolattr</> bitmap positions</title>

I would call this 'Attributes in rolattr' or similar rather than 'bitmap
positions'.

> ! <tgroup cols="3">
> ! <thead>
> ! <row>
> ! <entry>Position</entry>
> ! <entry>Attribute</entry>
> ! <entry>Description</entry>
> ! </row>
> ! </thead>

There should be a column for 'Option' or something- basically, a clear
tie-back to what CREATE ROLE refers to these as. I'm thinking that
reordering the columns would help here, consider:

Attribute (using the 'Superuser', 'Inherit', etc 'nice' names)
CREATE ROLE Clause (note: that's how CREATE ROLE describes the terms)
Description
Position

> ! <tbody>
> ! <row>
> ! <entry><literal>0</literal></entry>
> ! <entry>Superuser</entry>
> <entry>Role has superuser privileges</entry>
> </row>
>
> <row>
> ! <entry><literal>1</literal></entry>
> ! <entry>Inherit</entry>
> ! <entry>Role automatically inherits privileges of roles it is a member of</entry>
> </row>

This doesn't follow our general convention to wrap lines in the SGML at
80 chars (same as the C code) and, really, if you fix that it looks like
these lines shouldn't even be different at all (see above with the 'Role
has superuser privileges' <entry></entry> line).

Same is true for a few of the other cases.

> <row>
> ! <entry><literal>4</literal></entry>
> ! <entry>Catalog Update</entry>
> <entry>
> ! Role can update system catalogs directly. (Even a superuser cannot do this unless this column is true)
> </entry>
> </row>

I'm really not sure what to do with this one. I don't like leaving it
as-is, but I suppose it's probably the right thing for this patch to do.
Perhaps another patch should be proposed which improves the
documentation around rolcatupdate and its relationship to superuser.

> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> new file mode 100644
> index ef69b94..74bc702
> *** a/doc/src/sgml/func.sgml
> --- b/doc/src/sgml/func.sgml
> + <para>
> + <function>pg_has_role_attribute</function> checks the attribute permissions
> + given to a role. It will always return <literal>true</literal> for roles
> + with superuser privileges unless the attribute being checked is
> + <literal>CATUPDATE</literal> (superuser cannot bypass
> + <literal>CATUPDATE</literal> permissions). The role can be specified by name
> + and by OID. The attribute is specified by a text string which must evaluate
> + to one of the following role attributes:
> + <literal>SUPERUSER</literal>,
> + <literal>INHERIT</literal>,
> + <literal>CREATEROLE</literal>,
> + <literal>CREATEDB</literal>,
> + <literal>CATUPDATE</literal>,
> + <literal>CANLOGIN</literal>,
> + <literal>REPLICATION</literal>, or
> + <literal>BYPASSRLS</literal>.

This should probably refer to CREATE ROLE also as being where the
meaning of these strings is defined.

Otherwise, the docs look pretty good to me.

Thanks!

Stephen