Re: superuser() shortcuts

Peter,

* Peter Eisentraut (peter_e@gmx.net) wrote:
> On 11/5/14 5:10 PM, Adam Brightwell wrote:
> > Attached is two separate patches to address previous
> > comments/recommendations:
> >
> > * superuser-cleanup-shortcuts_11-5-2014.patch
>
> Seeing that the regression tests had to be changed in this patch
> indicates that there is a change of functionality, even though this
> patch was previously discussed as merely an internal cleanup.  So either
> there is a user-facing change, which would need to be documented (or at
> least mentioned, discussed, and dismissed as minor), or there is a fault
> in the tests, which should be fixed first independently.

It was brought up for discussion- see
http://www.postgresql.org/message-id/20141015052259.GG28859@tamriel.snowman.net

Specifically:
--------------- One curious item to note is that the current if(!superuser()) {} block approach has masked an
inconsistencyin at least the FDW code which required a change to the regression tests- namely, we normally force FDW
ownersto have USAGE rights on the FDW, but that was being bypassed when a superuser makes the call. I seriously doubt
thatwas intentional.  I won't push back if someone really wants it to stay that way though. 
---------------

No one mentioned any concerns with it (and three people replied), so I'm
inclined to think it's an agreeable change.  Adam probably didn't
mention it with this patch simply because it had already been brought
up.

> Which makes me wonder whether the other changes are indeed without
> effect or just not covered by tests.
>
> > * has_privilege-cleanup_11-5-2014.patch
>
> I don't really see the merit of this patch.  A "cleanup" patch that adds
> more code than it removes isn't really a cleanup.  If you want to make
> the APIs consistent, then let's make a patch for that.  If you want to
> make the error messages consistent, then let's make a patch for that.
> There is other work going on replacing these role attributes with
> something more general, so maybe this cleanup should be done as part of
> that other work.

Perhaps 'cleanup' is just an overloaded term.  The patch is to make the
APIs and the error messages consistent.  I'm amazed at how much
discussion and work is going into these exceptionally minor changes
which have been already broken out of the larger and far more
interesting work (imv anyway).  Having two patches, one to simply move
the checks around and then another to make the error messages in those
checks consistent, which will naturally end up depending on each other,
strikes me as overkill, but we can certainly do it.

Andres raised a concern about the specific error message wording (which
was intended to just make it more consistent with the rest of our
permission-check error messages..), are there any other opinions on the
wording?  Would be great to get feedback on that..
Thanks!
    Stephen