Re: superuser() shortcuts - Mailing list pgsql-hackers

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, Dec 2, 2014 at 4:11 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >> It's pretty clear that the current message is complaining about a
> >> permissions problem, so the fact that it doesn't specifically include
> >> the words "permission" and "denied" doesn't bother me.  Let me ask the
> >> question again: what information do you think is conveyed by your
> >> proposed rewrite that is not conveyed by the current message?
> >
> > I do like that it's explicitly stating that the problem is with
> > permissions and not because of some lack-of-capability in the software
> > and I like the consistency of messaging (to the point where I was
> > suggesting somewhere along the way that we update the error messaging
> > documentation to be explicit that the errmsg and the errcode should make
> > sense and match up- NOT that we should use some simple mapping from one
> > to the other as we'd then be reducing the information provided, but I've
> > seen a number of cases where people have the SQL error-code also
> > returned in their psql session; I've never been a fan of that as I much
> > prefer words over error-codes, but I wonder if they've done that
> > because, in some cases, it *isn't* clear what the errcode is from the
> > errmsg..).
>
> I think you're dodging my question.

My answer included "not because of some lack-of-capability" which was
intended to answer your question of "what information do you think is
conveyed by your proposed rewrite that is not converyed by the current
message?"

I have a hard time wrapping my head around what a *lot* of our users ask
when they see a given error message, but if our error message is 'you
must have a pear-shaped object to run this command' then I imagine that
a new-to-PG user might think "well, I can't create pear shaped objects
in PG, so I guess this just isn't supported."  That's not necessairly
any fault of ours, but I do think "permission denied" reduces the chance
that there's any confusion about the situation.

> The answer is that there really
> *isn't* any additional information conveyed by your proposal rewrite;

To be sure it's clear, I *don't* agree with this.  You and I don't see
any additional information in it because we're familiar with the system
and know all about role attributes, the GRANT system, and everything
else.  I'm not looking to change the error message because it's going to
make it clearer to you or to me or to anyone else on this list though.

> the issue is simply that you prefer your version to the current
> version.  Which is fair enough, but my preference is different (as is
> that of Andres), which is also fair.

I do prefer my version but it's not an unfounded preference.

> > It provides a consistency of messaging that I feel is an improvement.
> > That they aren't related in a number of existing cases strikes me as an
> > opportunity to improve rather than cases where we really "know better".
>
> Let's consider this case, which perhaps you haven't examined:
>
>         if (es.buffers && !es.analyze)
>                 ereport(ERROR,
>                                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>                                  errmsg("EXPLAIN option BUFFERS
> requires ANALYZE")));
>
> This is basically analogous to the case your complaining about.  The
> error message is short and to the point and does not include the words
> that appear in the error code.  Now, you could propose to change this,
> but any rewording of it is going to be longer than what we have now
> for no real improvement in clarity, and it's going to force
> translators to do the extra work of retranslating it for no particular
> gain.

I'm on the fence about if this is really the same case.  In this case,
changing 'option' to 'parameter' would make it pretty darn close to the
error code.  I could also see making it longer and more explicit-
'invalid parameter (BUFFERS) for EXPLAIN' with an errdetail of what's
above.

> The burden for changing existing messages is not high, but your desire
> for a different style is not sufficient to go change half the error
> messages in the backend, or even 20% of them, and I think that 20% is
> a conservative estimate of how many we'd have to change to actually
> achieve what you're talking about here.

The "different style" is what's in the error style guidelines.  I agree
that it could lead to a lot of changes and I don't think we'd need to
change them all in one shot or in one release, in part because of the
burden it would put on translators.

> > Perhaps in some of those cases the problem is that there isn't a good
> > errcode, and maybe we should actually add an error code instead of
> > changing the message.
>
> The error codes are mostly taken from the SQL standard.  It is of
> course possible that we should add our own in some cases, but it will
> have the disadvantage of reducing the ability of applications written
> for other systems to understand our error codes.

This, in particular, doesn't bother me terribly much- if there's reason
enough for a new code then it's probably because it's something PG
specific enough to justify it.

> > If we want to say that role-attribute-related error messages should be
> > "You need X to do Y", then I'll go make those changes, removing the
> > 'permission denied' where those are used and be happier that we're at
> > least consistent, but it'll be annoying if we ever make those
> > role-attributes be GRANT'able rights (GRANT .. ON CLUSTER?)  as those
> > errmsg's will then change from "you need X" to "permission denied to do
> > Y".  Having the errdetail change bothers me a lot less.
>
> You can't really put "you need X to do Y" in the error message, I
> think.  That style is appropriate for an error detail, but not an
> error message.

I'm utterly confused by this comment.  The existing error messages that
I want to change are of the 'you need X to do Y' style (eg: "must be
superuser or have the same role to cancel queries running in other
server processes").  If you're just referring to the 'you' word, then
ok, I agree that it goes against the error style guidelines and it would
really be "need X to do Y", but that wasn't what I was trying to get at
with the above comment.  I was saying *let's make it consistent* and go
change the existing cases which say "permission denied to create role"
and similar to, instead, say "must have CREATEROLE to create roles".
Today, we're utterly inconsistent.

> I just don't understand why you want to pointlessly tinker with this.

Because I don't feel it's pointless to improve the consistency of the
error messaging and I don't like that it's inconsistent today.

> If this is integrally related to the introduction of finer-grained
> superuser permissions, then perhaps it is worth doing, but if that's
> the motivation, you haven't made an argument that I can understand as
> to why it's necessary.

It's got nothing to do with the finer-grained superuser permissions.
This particular patch isn't even directly related to it- it's just my
feeling that these changes are in the right direction which we should be
going in and that it's an improvement in its own right to use
has_privs_of_role() instead of GetUserId() in these cases.

> What I think is that the current messages - or
> at least the ones you are complaining about - are fine, and we can
> change them on a case-by-case basis as the evolution of the system
> demands, but that we shouldn't go whack them around just because you
> would have chosen differently from among sane alternatives than what
> the original author chose to do.

I was trying to get to a more consistent system with these changes.  If
no one feels that's worthwhile, then so be it.  I appreciate the enegy
you've spent in responding even if we weren't ultimately able to agree
on the outcome.
Thanks,
    Stephen