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. http://www.postgresql.org/message-id/cakrt6cqovt2kiykg2gff7h9k8+jvu1149zlb0extkkk7taq...@mail.gmail.com 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.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com