Adam, * Adam Brightwell (adam.brightw...@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
signature.asc
Description: Digital signature
