Re: Role Attribute Bitmask Catalog Representation

Stephen,

The comment above is pretty big and I don't think we want to completely
> remove it. Can you add it as another 'Note' in the 'has_role_attribute'
> comment and reword it accordingly?
>

Ok, agreed. Will address.

Whitespace issue that should be fixed- attributes should line up with
> roleid.
>

Ok. Will address.

It occurs to me that in this case (and a few others..), we're doing a
> lot of extra work- each call to pg_check_role_attribute() is doing a
> lookup on the oid just to get back to what the rolattr value on this row
> is. How about a function which takes rolattr and the text
> representation of the attribute and returns if the bit is set for that
> rolattr value? Then you'd pass pg_authid.rolattr into the function
> calls above instead of the role's oid.
>

Makes sense, I'll put that together.

> I don't see any changes to the regression test files, were they
> forgotten in the patch? I would think that at least the view definition
> changes would require updates to the regression tests, though perhaps
> nothing else.
>

Hmmm... :-/ The regression tests that changed were in
'src/test/regress/expected/rules.out' and should be near the bottom of the
patch.

> Overall, I'm pretty happy with the patch and would suggest moving on to
> writing up the documentation changes to go along with the code changes.
> I'll continue to play around with it but it all seems pretty clean to
> me and will allow us to easily add the additiaonl role attributes being
> discussed.

Sounds good. I'll start on those changes next.

Thanks,
Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com