Re: superuser() shortcuts

* 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