* Andres Freund (and...@2ndquadrant.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
signature.asc
Description: Digital signature
