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

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

> + 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 &

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..


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:

Yes, we were, however the latter causes a syntax error with initdb. :-/

> ! DATA(insert OID = 10 ( "POSTGRES" PGROLATTRALL -1 _null_ _null_));
> >
> 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.


Adam Brightwell -
Database Engineer -

Reply via email to