Re: Role Attribute Bitmask Catalog Representation

Stephen Frost wrote:
> Alvaro,
> 
> * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> > FWIW I've been giving this patch a look and and adjusting some coding
> > details here and there.  Do you intend to commit it yourself?  You're
> > not listed as reviewer or committer for it in the commitfest app, FWIW.
> 
> Oh, great, thanks!  And, yeah, I'm not as good as I should be about
> updating the commitfest app.  As for committing it, I was thinking I
> would but you're certainly welcome to if you're interested.  I would
> like this to be committed soonish as it will then allow Adam to rebase
> the patch which adds the various role attributes discussed previously on
> top of the representation changes.  I suspect he's done some work in
> that direction already, but I keep asking for changes to this patch
> which would then ripple down into the other.

Sure, I will go over it a bit more and merge changes from Adam to the
docs as they come through, and commit soon.

> > One thing I don't very much like is that check_role_attribute() receives
> > a RoleAttr but nowhere it checks that only one bit is set in
> > 'attribute'.  From the coding, this routine would return true if just
> > one of those bits is set, which is probably not what is intended.  Now I
> > realize that current callers all pass a bitmask with a single bit set,
> > but I think it'd be better to have some protection against that, for
> > possible future coding mistakes.
> 
> That's certainly a good point.  I'm inclined to suggest that there be an
> Assert() check in check_role_attribute(), or were you thinking of
> something else..?

Yeah, an Assert() is what I had in mind.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services