Re: Role Attribute Bitmask Catalog Representation

Stephen Frost wrote:
> Alvaro,
>
> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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