Re: Role Attribute Bitmask Catalog Representation - Mailing list pgsql-hackers
| From | Adam Brightwell |
|---|---|
| Subject | Re: Role Attribute Bitmask Catalog Representation |
| Date | |
| Msg-id | CAKRt6CSasUEtCY+UfL+YtpVdru=YSCA02K2NTixEcA+2tEW2EQ@mail.gmail.com Whole thread Raw |
| In response to | Re: Role Attribute Bitmask Catalog Representation (Stephen Frost <sfrost@snowman.net>) |
| Responses |
Re: Role Attribute Bitmask Catalog Representation
(Stephen Frost <sfrost@snowman.net>)
|
| List | pgsql-hackers |
Stephen,
--
Thanks for this. Found time to do more of a review and have a few
comments:
Thanks for taking a look at this and for the feedback.
I'd put the superuser_arg() check in role_has_attribute.
Ok. Though, this would affect how CATUPDATE is handled. Peter Eisentraut previously raised a question about whether superuser checks should be included with catupdate which led me to create the following post.
Certainly, we could keep has_rolcatupdate for this case and put the superuser check in role_has_attribute, but it seems like it might be worth taking a look at whether a superuser can bypass catupdate or not. Just a thought.
And then ditch the individual has_X_privilege() calls (I note that you
didn't appear to add back the has_rolcatupdate() function..).
Ok. I had originally thought for this patch that I would try to minimize these types of changes, though perhaps this is that opportunity previously mentioned in refactoring those. However, the catupdate question still remains.
> + static RoleAttr
> + set_role_attribute(RoleAttr attributes, RoleAttr attribute)
> + {
> + return ((attributes & ~(0xFFFFFFFFFFFFFFFF)) | attribute);
> + }
I don't think this is doing what you think it is..? It certainly isn't
working as expected by AlterRole(). Rather than using a function for
these relatively simple operations, why not just have AlterRole() do:
if (isX >= 0)
attributes = (isX > 0) ? attributes | ROLE_ATTR_X
: attributes & ~(ROLE_ATTR_X);
Yes, this was originally my first approach. I'm not recollecting why I decided on the other route, but agree that is preferable and simpler.
Otherwise, you'd probably need to pass a flag into set_role_attribute()
to indicate if you're setting or unsetting the bit, or have an
'unset_role_attribute()' function, but all of that seems like overkill..
Agreed.
We don't bother with the '> 0' in any of the existing bit testing that
goes on in the backend, so I don't think it makes sense to start now.
Just do
if (attributes & ROLE_ATTR_SUPERUSER) ... etc
and be done with it.
Ok, easy enough.
Why not have this as 'pg_has_role_id_attribute' and then have a
'pg_has_role_name_attribute' also? Seems like going with the pg_
namespace is the direction we want to go in, though I'm not inclined to
argue about it if folks prefer has_X.
I have no reason for one over the other, though I did ask myself that question. I did find it curious that in some cases there is "has_X" and then in others "pg_has_X". Perhaps I'm not looking in the right places, but I haven't found anything that helps to distinguish when one vs the other is appropriate (even if it is a general rule of thumb).
Seems like you could just make temp_array be N_ROLE_ATTRIBUTES in
size, instead of building a list, counting it, and then building the
array from that..?
Yes, this is true.
Do we really need to keep has_rolinherit for any reason..? Seems
unnecessary given it's only got one call site and it's nearly the same
as a call to role_has_attribute() anyway. Ditto with
has_rolreplication().
I really don't see any reason and as above, I can certainly do those refactors now if that is what is desired.
Thought we were getting rid of this..?
> ! #define N_ROLE_ATTRIBUTES 8 /* 1 plus the last 1<<x */
> ! #define ROLE_ATTR_NONE 0
> ! #define ROLE_ATTR_ALL 255 /* All currently available attributes. */
Or:
#define ROLE_ATTR_ALL (1<<N_ROLE_ATTRIBUTES)-1
Yes, we were, however the latter causes a syntax error with initdb. :-/
> ! DATA(insert OID = 10 ( "POSTGRES" PGROLATTRALL -1 _null_ _null_));
>
> #define BOOTSTRAP_SUPERUSERID 10
Is it actually necessary to do this substitution when the value is
#define'd in the same .h file...? Might be something to check, if you
haven't already.
Yes, I had previously checked this, I get the following error from initdb.
FATAL: invalid input syntax for integer: "ROLE_ATTR_ALL"
> + #define ROLE_ATTR_SUPERUSER (1<<0)
> + #define ROLE_ATTR_INHERIT (1<<1)
> + #define ROLE_ATTR_CREATEROLE (1<<2)
> + #define ROLE_ATTR_CREATEDB (1<<3)
> + #define ROLE_ATTR_CATUPDATE (1<<4)
> + #define ROLE_ATTR_CANLOGIN (1<<5)
> + #define ROLE_ATTR_REPLICATION (1<<6)
> + #define ROLE_ATTR_BYPASSRLS (1<<7)
> + #define N_ROLE_ATTRIBUTES 8
> + #define ROLE_ATTR_NONE 0
These shouldn't need to be here any more..?
No they shouldn't, not sure how I missed that.
Thanks,
Adam
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
pgsql-hackers by date: