Re: Role Attribute Bitmask Catalog Representation - Mailing list pgsql-hackers

Adam,

* Adam Brightwell (adam.brightwell@crunchydatasolutions.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