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.
> 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..?
Thanks!
Stephen
