Adam,
* Brightwell, Adam (adam.brightwell@crunchydatasolutions.com) wrote:
> > We, as a community, have gotten flak from time-to-time about the
> > superuser role. We also tend to wish to avoid unnecessary
> > optimization as it complicates the code base and makes folks reviewing
> > the code wonder at the exceptions.
>
> I have attached a patch for consideration/discussion that addresses these
> items. I have based it off of current master (0c013e0). I have done some
> cursory testing including check-world with success.
Thanks! Please add it to the next commitfest.
> With all the other pg_has_* functions being found in acl.c I agree that it
> would seem odd that this (or any other related function) would be found
> elsewhere. Since aclchk.c also has a couple of functions that follow the
> pattern of "has_<priv>_privilege", I moved this function there, for now, as
> well as modified it to include superuser checks as they were not previously
> there. The only related function I found in acl.c was "has_rolinherit"
> which is a static function. :-/ There is also a static function
> "has_rolcatupdate" in aclchk.c. I would agree that acl.c (or aclchk.c?)
> would be a more appropriate location for these functions, though in some of
> the above cases, it might require exposing them (I'm not sure that I see
> any harm in doing so).
I don't think has_rolinherit or has_rolcatupdate really need to move and
it seems unlikely that they'd be needed from elsewhere.. Is there a
reason you think they'd need to be exposed? I've not looked at the
patch at all though, perhaps that makes it clear.
> Perhaps refactoring to consolidate all of these in either acl.c or aclchk.c
> with the following pattern might be a step in the right direction?
>
> * has_createrole_privilege
> * has_bypassrls_privilege
These are already in the right place, right?
> * has_inherit_privilege
> * has_catupdate_privilege
These probably don't need to move as they're only used in the .c files
that they're defined in (unless there's a reason that needs to change).
> * has_replication_privilege
This is what I was on about, so I agree that it should be created. ;)
> * has_???_privilege
Right, other things might be 'has_backup_privilege', for things like
pg_start/stop_backup and friends.
> In either case, I think following a "convention" on the naming of these
> functions (unless there is semantic reason not to) would also help to
> reduce any confusion or head scratching.
Agreed.
Thanks!
Stephen