From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Andres Freund <andres(at)2ndquadrant(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Adam Brightwell <adam(dot)brightwell(at)crunchydatasolutions(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: superuser() shortcuts |
Date: | 2014-11-26 14:43:07 |
Message-ID: | 20141126144306.GX28859@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2014-11-26 08:33:10 -0500, Stephen Frost wrote:
> > Doesn't that argument then apply to the other messages which I pointed
> > out in my follow-up to Andres, where the detailed info is in the hint
> > and the main error message is essentially 'permission denied'?
>
> A permission error on a relation is easier to understand than one
> for some abstract 'replication' permission.
The more I consider this and review the error message reporting policy,
the more I feel that the original coding was wrong and that this change
*is* the correct one to make.
Even the example in the documentation makes a point of having the
errcode() and the errmsg() match up:
ereport(ERROR,
(errcode(ERRCODE_DIVISION_BY_ZERO),
errmsg("division by zero")));
The second example is similar:
ereport(ERROR,
(errcode(ERRCODE_AMBIGUOUS_FUNCTION),
errmsg("function %s is not unique",
func_signature_string(funcname, nargs,
NIL, actual_arg_types)),
errhint("Unable to choose a best candidate function. "
"You might need to add explicit typecasts.")));
Further, the comments around many of the places which use
ACLCHECK_NOT_OWNER (which also uses the 'must be/have X' style) are
"permissions check for X", and what's more, we've had things go from one
to the other previously even though the user action wasn't changing-
specifically TRUNCATE used to be owner-only and was changed to be
grantable.
The reason for the error *is* permission denied, hence the errcode().
The detail is that the permission is reserved to the owner, or to roles
which have a given attribute. The error isn't "must by X". I'm of the
opinion at this point that we should change the ACLCHECK_NOT_OWNER
branch under aclcheck_error() to match what is proposed by this patch-
have a 'permission denied' error for whatever the action is and then
have errdetail of 'not_owner_msg[objectkind]'.
I don't think we need to include errdetail() for the ACLCHECK_NO_PRIV
case as those permissions are part of the normal GRANT/REVOKE privilege
system. The detail is warranted when we're talking about things which
are outside of the normal privilege system.
If it isn't a permission denied error then it shouldn't be using
ERRCODE_INSUFFICIENT_PRIVILEGE.
Thanks,
Stephen
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2014-11-26 14:52:43 | Re: Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3 |
Previous Message | Adrian Klaver | 2014-11-26 14:37:31 | Re: issue in postgresql 9.1.3 in using arrow key in Solaris platform |