Re: superuser() shortcuts

From: "Brightwell, Adam" <adam(dot)brightwell(at)crunchydatasolutions(dot)com>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: superuser() shortcuts
Date: 2014-10-01 22:37:04
Message-ID: CAKRt6CTaxSNKi=W=-NF4wOHJOMqiyKh4dEvjbLoQ+Ve_xbMvDg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Stephen,

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.

> My 2c about this function is that it should be completely removed
> and the place where it's checked replaced with just the
> 'has_rolreplication' call and error. It's only called in one
> place and it'd be a simple one-liner anyway. As for
> has_rolreplication, I don't understand why it's in miscinit.c when
> the rest of the has_* set is in acl.c.
>

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).

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
* has_inherit_privilege
* has_catupdate_privilege
* has_replication_privilege
* has_???_privilege

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.

Removing these design patterns may also help to avoid ending up with
> more of them in the future as folks copy and/or crib off of what we've
> already done to implement their features...
>

I agree.

-Adam

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

Attachment Content-Type Size
superuser-cleanup-shortcuts_10-1-2014.patch text/x-patch 19.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ants Aasma 2014-10-01 23:42:28 Re: "Value locking" Wiki page
Previous Message Bogdan Pilch 2014-10-01 21:18:46 Re: Time measurement format - more human readable