* Tom Lane ([EMAIL PROTECTED]) wrote: > Stephen Frost <[EMAIL PROTECTED]> writes: > > Attached please find files and patches associated with moving from the > > User/Group system currently in place to Roles, as discussed > > previously. > > I have cleaned this up a bit and committed it. I normally wouldn't > commit an incomplete patch, but this change is blocking Alvaro's work > on dependencies for shared objects, so I felt it was best to get the > catalog changes in now. That will let Alvaro work on dependencies > while I sort out the unfinished bits of roles, which I intend to do > over the next day or so.
Great, glad to hear it. I hope you got a chance to look over the open items in the 'milestones' file. I'd really like to see the grammar be fixed to match SQL spec for GRANT ROLE/REVOKE ROLE. I think an approach to take there might be to try and get GrantRoleStmt and GrantStmt to use the same productions at the end of the line if possible or something along those lines. Also, I've been looking through the diff between my tree and what you committed to CVS and had a couple comments (just my 2c: I think it would have been alot easier using SVN to see exaclty what was different from my patch vs. other changes since my last CVS up): First, sorry about the gratuitous name changes, it helped me find every place I needed to look at the code and think about if it needed to be changed in some way (ie: Int32GetDatum -> ObjectIdGetDatum, etc). I had planned on changing some of them back to minimize the patch but kind of ran out of time. Second, looks like I missed fixing an owner check in pg_proc.c Current CVS has, line 269: if (GetUserId() != oldproc->proowner && !superuser()) Which is not a sufficient owner check. This should by fixed by doing a proper pg_proc_ownercheck, ie: if (!pg_proc_ownercheck(HeapTupleGetOid(oldtup), GetUserId())) Third, I feel it's incorrect to only allow superuser() to change ownership of objects under a role-based system. Users must be able to create objects owned by a role they're in (as opposed to owned only by themselves). Without this there is no way for a given role to allow other roles to perform owner-level actions on objects which they create. The point of adding roles was to allow owner-level actions on objects to more than a single user or the superuser. Requiring the superuser to get involved with every table creation defeats much of the point. This should really be possible either by explicitly changing the ownership of an object using ALTER ... OWNER, or by a SET ROLE followed by CREATE TABLE, etc. SET ROLE is defined by the SQL specification, though we don't support it specifically yet (shouldn't be too difficult to add now though). Certainly if we accept that SET ROLE should be supported and that objects then created should be owned by the role set in SET ROLE we should be willing to support non-superusers doing ALTER ... OWNER given that they could effectively do the same thing via SET ROLE (though with much more difficulty, which has no appreciable gain). Fourth, not that I use it, but, it looks like my changes to src/interfaces/ecpg/preproc/preproc.y were lost. Not sure if that was intentional or not (I wouldn't think so... I do wish ecpg could just be the differences necessary for ecpg and be based off the main parser somehow, but that'd be a rather large change). Oh, and in that same boat, src/tools/pgindent/pgindent also appears to not have gotten the changes that I made. > Many thanks for your work on this! Happy to have helped though frustrated that you seem to have removed the part that I was originally looking for. I don't feel that's justification for having it (I feel I've addressed that above) but it certainly would have been nice to be aware of that earlier and perhaps to have discussed the issues around it a bit more before being so close to the feature freeze (I know, alot my fault, but there it is). Thanks, Stephen
signature.asc
Description: Digital signature