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

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, Dec 2, 2014 at 2:35 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Robert Haas (robertmhaas@gmail.com) wrote:
> >> On Tue, Dec 2, 2014 at 11:29 AM, Stephen Frost <sfrost@snowman.net> wrote:
> >> > It includes the actual error message, unlike what we have today which is
> >> > guidence as to what's required to get past the permission denied error.
> >> >
> >> > In other words, I disagree that the same amount of information is being
> >> > conveyed.
> >>
> >> So, you think that someone might see the message "must be superuser or
> >> replication role to use replication slots" and fail to understand that
> >> they have a permissions problem?
> >
> > I think that the error message about a permissions problem should be
> > that there is a permissions problem, first and foremost.
>
> I can't disagree with that, but it's not like the current message is:
>
> ERROR: I ran into some kind of a problem but I'm not going to tell you
> what it is for a long time OK I guess that's enough well actually you
> see there is a problem and it happens to do have to do with
> permissions and in particular that they are denied.

I agree that doing this would be rather overkill.

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

> > We don't say 'You must have SELECT rights on relation X to select from
> > it.' when you try to do a SELECT that you're not allowed to.  We throw a
> > permissions denied error.  As pointed out up-thread, we do similar
> > things for other cases of permissions denied and even beyond permission
> > denied errors we tend to match up the errmsg() with the errcode(), which
> > makes a great deal of sense to me and is why I'm advocating that
> > position.
>
> I don't thing that trying to match up the errmsg() with the errcode()
> is a useful exercise.  That's just restricting the ways that people
> can write error messages in a way that is otherwise unnecessary.  The
> errmsg() and errcode() have different purposes; the former is to
> provide a concise, human-readable description of the problem, and the
> latter is to group errors into categories so that related errors can
> be handled programmatically.  They need not be, and in many existing
> cases are not, even vaguely similar; and the fact that many of our
> permission denied errors happen to use that phrasing is not a
> sufficient justification for changing the rest unless the new phrasing
> is a bona fide improvement.

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".
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.  Perhaps there are cases where we just know better
or it's a very generic errcode without any real meaning.  I agree that
we want the messaging to be good, I'd like it to also be consistent with
itself and with the errcode.

I've not gone and tried to do a deep look at the errmsg vs errcode, and
that's likely too much to bite off at once anyway, but I have gone and
looked at the role attribute uses and the other permissions denied
errcode cases and we're not at all consistent today and we change from
release-to-release depending on changes to the permissions system (see:
TRUNCATE).  I specifically don't like that.

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.

How would you feel about changing the various usages of
have_createrole_privilege() call sites to say "You must have CREATEROLE
to create role" instead of "permission denied to create role"?  Today,
some of them say one, other times the other, and, for my part at least,
I don't see any particular justification for why it's one way sometimes
and the other other times, which is an example of the inconsistency I'm
complaining about.  At least if we had a policy we would have hope of
getting consistency and we would still have the detail in the
errdetail().  Making these cases two lines instead of one doesn't bother
me one bit, at least.
Thanks,
    Stephen