Re: wrong relkind error messages

On Mon, Apr 13, 2020 at 11:13:15AM -0400, Tom Lane wrote:
> Fixing this while avoiding your concern about proliferation of messages
> seems a bit difficult though. The best I can do after a couple minutes'
> thought is
>
> ERROR: cannot define statistics for relation "ti"
> DETAIL: "ti" is an index, and this operation is not supported for that
> kind of relation.
>
> which seems a little long and awkward. Another idea is
>
> ERROR: cannot define statistics for relation "ti"
> DETAIL: This operation is not supported for indexes.
>
> which still leaves implicit that "ti" is an index, but probably that's
> something the user can figure out.
>
> Maybe someone else can do better?

"This operation is not supported for put_relkind_here \"%s\"."? I
think that it is better to provide a relation name in the error
message (even optionally a namespace). That's less to guess for the
user.

+int
+errdetail_relkind(const char *relname, char relkind)
+{
+ switch (relkind)
+ {
+ case RELKIND_RELATION:
+ return errdetail("\"%s\" is a table.", relname);
+ case RELKIND_INDEX:
It seems to me that we should optionally add the namespace in the
error message, or just have a separate routine for that. I think that
it would be useful in some cases (see for example the part about the
statistics in the patch), still annoying in some others (instability
in test output for temporary schemas for example) so there is a point
for both in my view.

- if (rel->rd_rel->relkind != RELKIND_VIEW &&
- rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE &&
- rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
- rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
- {
+ if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
RelationDropStorage(rel);
These should be applied separately in my opinion. Nice catch.

- errmsg("\"%s\" is not a table, view, materialized view, sequence, or foreign table",
- rv->relname)));
+ errmsg("cannot change schema of relation \"%s\"",
+ rv->relname),
+ (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX ? errhint("Change the schema of the table instead.") :
+ (relkind == RELKIND_COMPOSITE_TYPE ? errhint("Use ALTER TYPE instead.") : 0))));

This is not great style either and reduces readability, so I would
recommend to split the errhint generation using a switch/case.

+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("action cannot be performed on relation
\"%s\"",
+ RelationGetRelationName(rel)),
Echoing Robert upthread, "action" is not really useful for the user,
and it seems to me that it should be reworked as "cannot perform foo
on relation \"hoge\""

+ errmsg("relation \"%s\" does not support comments",
+ RelationGetRelationName(relation)),
This is not project-style as full sentences cannot be used in error
messages, no? The former is not that good either, still, while this
is getting touched... Say, "cannot use COMMENT on relation \"%s\""?

Overall +1 on the patch by the way. Thanks for sending something to
improve the situation
--
Michael