Skip site navigation (1) Skip section navigation (2)

Re: [PATCHES] Users/Groups -> Roles

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCHES] Users/Groups -> Roles
Date: 2005-06-28 16:04:14
Message-ID: 20050628160414.GG24207@ns.snowman.net (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-patches
* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)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 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

In response to

Responses

pgsql-hackers by date

Next:From: Stephen FrostDate: 2005-06-28 16:13:59
Subject: Re: [PATCHES] Users/Groups -> Roles
Previous:From: Alvaro HerreraDate: 2005-06-28 15:56:22
Subject: Re: commit_delay, siblings

pgsql-patches by date

Next:From: Stephen FrostDate: 2005-06-28 16:13:59
Subject: Re: [PATCHES] Users/Groups -> Roles
Previous:From: Karl O. PincDate: 2005-06-28 15:42:32
Subject: Fwd: Re: [PERFORM] Performance analysis of plpgsql code [kop@meme.com]

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group