Re: Role Attribute Bitmask Catalog Representation

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+JVU1149zLb0EXTkKk7TAQGMQ@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(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com