Re: [PATCHES] Users/Groups -> Roles - Mailing list pgsql-hackers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> 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
aboutif it needed to be changed in some way (ie: Int32GetDatum -> ObjectIdGetDatum, etc).  I had planned on changing
someof 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.
Usersmust be able to create objects owned by a role they're in (as opposed to owned only by themselves).  Without this
thereis no way for a given role to allow other roles to perform owner-level actions on objects which they create.  The
pointof adding roles was to allow owner-level actions on objects to more than a single user or the superuser.
Requiringthe superuser to get involved with every table creation defeats much of the point.  This should really be
possibleeither by explicitly changing the  ownership of an object using ALTER ... OWNER, or by a SET ROLE  followed by
CREATETABLE, etc.  SET ROLE is defined by the SQL  specification, though we don't support it specifically yet
(shouldn'tbe too difficult to add now though).  Certainly if we accept that  SET ROLE should be supported and that
objectsthen 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,
whichhas 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
ifthat was intentional or not (I wouldn't think so...  I do wish ecpg could just be the differences necessary for ecpg
andbe based off the main parser somehow, but that'd be a rather large change).  Oh, and in that same boat,
src/tools/pgindent/pgindentalso 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'tfeel that's justification for having it (I feel I've addressed that above) but it certainly would have been nice
tobe aware of that earlier and perhaps to have discussed the issues around it a bit more before being so close to the
featurefreeze (I know, alot my fault, but there it is). 
     Thanks,
    Stephen