From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Adam Brightwell <adam(dot)brightwell(at)crunchydatasolutions(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Role Attribute Bitmask Catalog Representation |
Date: | 2014-12-19 03:24:59 |
Message-ID: | 20141219032459.GF3510@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2014-12-19 03:48:16 | Re: Role Attribute Bitmask Catalog Representation |
Previous Message | Bruce Momjian | 2014-12-19 03:02:41 | Re: Commit fest 2014-12, let's begin! |