wrong relkind error messages

On Mon, Apr 13, 2020 at 9:55 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Attached is another attempt to improve this.

Nice effort. Most of these seem like clear improvements, but some I don't like:

+ errmsg("relation \"%s\" is of unsupported kind",
+ RelationGetRelationName(rel)),
+ errdetail_relkind(RelationGetRelationName(rel), rel->rd_rel->relkind)));

It would help to work "pgstattuple" into the message somehow. "cannot
use pgstattuple on relation \"%s\"", perhaps?

+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("action cannot be performed on relation \"%s\"",
+ RelationGetRelationName(rel)),

Super-vague.

+ errmsg("cannot set relation options of relation \"%s\"",
+ RelationGetRelationName(rel)),

I suggest "cannot set options for relation \"%s\""; that is, use "for"
instead of "of", and don't say "relation" twice.

+ errmsg("cannot create trigger on relation \"%s\"",
+ RelationGetRelationName(rel)),
+ errmsg("relation \"%s\" cannot have triggers",
+ RelationGetRelationName(rel)),
+ errmsg("relation \"%s\" cannot have triggers",
+ rv->relname),

Maybe use the second wording for all three? And similarly for rules?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I'm not a fan of error messages like
relation "%s" is not a table, foreign table, or materialized view

Agreed, they're not great.

For example:

-ERROR:  relation "ti" is not a table, foreign table, or materialized view
+ERROR:  cannot define statistics for relation "ti"
+DETAIL:  "ti" is an index.

I see where you'e going, and it seems like a generally-better idea,
but I feel like this phrasing is omitting some critical background
information that users don't necessarily have. At the very least
it's not stating clearly that the failure is *because* ti is an
index. More generally, the whole concept that statistics can only
be defined for certain kinds of relations has disappeared from view.
I fear that users who're less deeply into Postgres hacking than we
are might not have that concept at all, or at least it might not
come to mind immediately when they get this message.

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?

regards, tom lane

On 4/13/20 11:13 AM, Tom Lane wrote:

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.

+1 for this. It's clear and succinct.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 2020-Apr-14, Michael Paquier wrote:

On Mon, Apr 13, 2020 at 11:13:15AM -0400, Tom Lane wrote:

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.

But the relation name is already in the ERROR line -- why do you care so
much about also having it in the DETAIL? Besides, I think part of the
point Tom was making is that if you say "not supported for the index
foo" is that the user is left wondering whether the operation is not
supported for that particular index only or for any index.

Tom's other proposal

DETAIL: "ti" is an index, and this operation is not supported for that kind of relation.

addresses that problem, but seems excessively verbose.

Also, elsewhere Peter said[1]/messages/by-id/1293803569.19789.6.camel@fsopti579.F-Secure.com that we should not try to list the things
that would be allowed, so it's pointless to try to list the relkinds for
which the operation is permissible.

So I +1 this idea:

ERROR: cannot define statistics for relation "ti"
DETAIL: This operation is not supported for indexes.

[1]: /messages/by-id/1293803569.19789.6.camel@fsopti579.F-Secure.com

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-Apr-13, Robert Haas wrote:

+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("action cannot be performed on relation \"%s\"",
+ RelationGetRelationName(rel)),

Super-vague.

Maybe, but note that the patch proposed to replace this current error
message:
ERROR: foo is not an index or foreign table
with
ERROR: action cannot be performed on "foo"
DETAIL: "foo" is a materialized view.

or, if we're to adopt Tom's proposed wording,

ERROR: cannot perform action on relation "ti"
DETAIL: This operation is not supported for materialized views.

so it's not like this is making things any worse; the error was already
super-vague.

This could be improved if we had stringification of ALTER TABLE
subcommand types:

ERROR: ALTER TABLE ... ADD COLUMN cannot be performed on "foo"
DETAIL: "foo" is a gummy bear.
or
ERROR: ALTER TABLE ... ADD COLUMN cannot be performed on foo
DETAIL: This action cannot be performed on gummy bears.

but that seems material for a different patch.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2020-Apr-13, Robert Haas wrote:

+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("action cannot be performed on relation \"%s\"",
+ RelationGetRelationName(rel)),

Super-vague.

Maybe, but note that the patch proposed to replace this current error
message:
ERROR: foo is not an index or foreign table
...
so it's not like this is making things any worse; the error was already
super-vague.

Yeah. I share Robert's feeling that "action" is not really desirable
here, but I have to concur that this is an improvement on the existing
text, which also fails to mention what command is being rejected.

This could be improved if we had stringification of ALTER TABLE
subcommand types:
ERROR: ALTER TABLE ... ADD COLUMN cannot be performed on "foo"

In the meantime could we at least say "ALTER TABLE action cannot
be performed"? The worst aspect of the existing text is that if
an error comes out of a script with a lot of different commands,
it doesn't give you any hint at all about which command failed.

regards, tom lane

On Tue, Apr 14, 2020 at 7:02 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2020-Apr-13, Robert Haas wrote:

+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("action cannot be performed on relation \"%s\"",
+ RelationGetRelationName(rel)),

Super-vague.

Maybe, but note that the patch proposed to replace this current error
message:
ERROR: foo is not an index or foreign table
with
ERROR: action cannot be performed on "foo"
DETAIL: "foo" is a materialized view.

Sure, but the point is that this case is not improved nearly as much
as most of the others. In a whole bunch of cases, he made the error
message describe the attempted operation, but here he didn't. I'm not
saying that makes it worse than what we had before, just that it would
be better if we could make this look like the other cases the patch
also changes.

This could be improved if we had stringification of ALTER TABLE
subcommand types:

ERROR: ALTER TABLE ... ADD COLUMN cannot be performed on "foo"
DETAIL: "foo" is a gummy bear.
or
ERROR: ALTER TABLE ... ADD COLUMN cannot be performed on foo
DETAIL: This action cannot be performed on gummy bears.

but that seems material for a different patch.

Even without that, you could at least say "this form of ALTER TABLE is
not supported for foo" or something like that.

I'm not trying to block the patch. I think it's a good patch. I was
just making an observation about some parts of it where it seems like
we could try slightly harder to do better.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2020-Apr-15, Robert Haas wrote:

[good arguments]

I don't disagree with anything you said, and I don't have anything to
add for now.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-04-15 02:15, Tom Lane wrote:

In the meantime could we at least say "ALTER TABLE action cannot
be performed"?

We don't know whether ALTER TABLE was the command. For example, in one
of the affected regression test cases, the command is ALTER VIEW.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2020-04-15 02:15, Tom Lane wrote:

In the meantime could we at least say "ALTER TABLE action cannot
be performed"?

We don't know whether ALTER TABLE was the command. For example, in one
of the affected regression test cases, the command is ALTER VIEW.

Maybe just "ALTER action cannot be performed"? I share Robert's
dislike of being so vague as to just say "action".

regards, tom lane

On 13.04.20 15:54, Peter Eisentraut wrote:

I'm not a fan of error messages like

    relation "%s" is not a table, foreign table, or materialized view

It doesn't tell me what's wrong, it only tells me what else could have
worked.  It's also tedious to maintain and the number of combinations
grows over time.

Another go at this. I believe in the attached patch I have addressed
all the feedback during this thread last year. In particular, I have
rephrased the detail message per discussion, and I have improved the
messages produced by ATSimplePermissions() with more details. Examples:

  CREATE STATISTICS tststats.s2 ON a, b FROM tststats.ti;
-ERROR:  relation "ti" is not a table, foreign table, or materialized view
+ERROR:  cannot define statistics for relation "ti"
+DETAIL:  This operation is not supported for indexes.

  ALTER FOREIGN TABLE ft1 ALTER CONSTRAINT ft1_c9_check DEFERRABLE; -- ERROR
-ERROR:  "ft1" is not a table
+ERROR:  ALTER action ALTER CONSTRAINT cannot be performed on relation "ft1"
+DETAIL:  This operation is not supported for foreign tables.

There might be room for some wordsmithing in a few places, but generally
I think this is complete.

On Thu, Jun 24, 2021 at 10:12:49AM +0200, Peter Eisentraut wrote:

There might be room for some wordsmithing in a few places, but generally I
think this is complete.

I have been looking at that, and it seems to me that you nailed it.
That's a nice improvement compared to the existing error handling with
multiple relkinds.

+           ereport(ERROR,
+                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                    errmsg("ALTER action %s cannot be performed on relation \"%s\"",
+                           action_str, RelationGetRelationName(rel)),
+                    errdetail_relkind_not_supported(rel->rd_rel->relkind)));
Perhaps the result of alter_table_type_to_string() is worth a note for
translators?

+       case AT_DetachPartitionFinalize:
+           return "DETACH PARTITION FINALIZE";
To be exact, I think that this one should be "DETACH PARTITION
... FINALIZE".

+       if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
+           ereport(ERROR,
+                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                    errmsg("cannot change schema of index \"%s\"",
+                           rv->relname),
+                    errhint("Change the schema of the table instead.")));
+       else if (relkind == RELKIND_COMPOSITE_TYPE)
+           ereport(ERROR,
+                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                    errmsg("cannot change schema of composite type
\"%s\"",
+                           rv->relname),
+                    errhint("Use ALTER TYPE instead.")));
I would simplify this part by removing the errhint(), and use "cannot
change schema of relation .." as error string, with a dose of
errdetail_relkind_not_supported().

+                errmsg("relation \"%s\" cannot have triggers",
+                       RelationGetRelationName(rel)),
Better as "cannot create/rename/remove triggers on relation \"%s\""
for the three code paths of trigger.c?

+                errmsg("relation \"%s\" cannot have rules",
[...]
+                errmsg("relation \"%s\" cannot have rules",
For rewriteDefine.c, this could be "cannot create/rename rules on
relation".
--
Michael

On 02.07.21 08:25, Michael Paquier wrote:

+           ereport(ERROR,
+                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                    errmsg("ALTER action %s cannot be performed on relation \"%s\"",
+                           action_str, RelationGetRelationName(rel)),
+                    errdetail_relkind_not_supported(rel->rd_rel->relkind)));
Perhaps the result of alter_table_type_to_string() is worth a note for
translators?

ok

+       case AT_DetachPartitionFinalize:
+           return "DETACH PARTITION FINALIZE";
To be exact, I think that this one should be "DETACH PARTITION
... FINALIZE".

ok

+       if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
+           ereport(ERROR,
+                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                    errmsg("cannot change schema of index \"%s\"",
+                           rv->relname),
+                    errhint("Change the schema of the table instead.")));
+       else if (relkind == RELKIND_COMPOSITE_TYPE)
+           ereport(ERROR,
+                   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                    errmsg("cannot change schema of composite type
\"%s\"",
+                           rv->relname),
+                    errhint("Use ALTER TYPE instead.")));
I would simplify this part by removing the errhint(), and use "cannot
change schema of relation .." as error string, with a dose of
errdetail_relkind_not_supported().

I aimed for parity with the error reporting in ATExecChangeOwner() here.

+                errmsg("relation \"%s\" cannot have triggers",
+                       RelationGetRelationName(rel)),
Better as "cannot create/rename/remove triggers on relation \"%s\""
for the three code paths of trigger.c?

+                errmsg("relation \"%s\" cannot have rules",
[...]
+                errmsg("relation \"%s\" cannot have rules",
For rewriteDefine.c, this could be "cannot create/rename rules on
relation".

I had it like that, but in previous reviews some people liked it better
this way. ;-) I tend to agree with that, since the error condition
isn't that you can't create a rule/etc. (like, due to incorrect
prerequisite state) but that there cannot be one ever.

On 2021-Jun-24, Peter Eisentraut wrote:

There might be room for some wordsmithing in a few places, but generally I
think this is complete.

This looks good to me. I am +0.1 on your proposal of "cannot have
triggers" vs Michael's "cannot create triggers", but really I could go
with either. Michael's idea has the disadvantage that if the user fails
to see the trailing "s" in "triggers" they could get the idea that it's
possible to create some other trigger; that seems impossible to miss
with your wording. But it's not that bad either.

It seemed odd to me at first that errdetail_relkind_not_supported()
returns int, but I realized that it's a trick to let you write "return
errdetail()" so you don't have to have "break" which would require one
extra line. Looks fine.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

On 02.07.21 18:10, Alvaro Herrera wrote:

On 2021-Jun-24, Peter Eisentraut wrote:

There might be room for some wordsmithing in a few places, but generally I
think this is complete.

This looks good to me. I am +0.1 on your proposal of "cannot have
triggers" vs Michael's "cannot create triggers", but really I could go
with either. Michael's idea has the disadvantage that if the user fails
to see the trailing "s" in "triggers" they could get the idea that it's
possible to create some other trigger; that seems impossible to miss
with your wording. But it's not that bad either.

It seemed odd to me at first that errdetail_relkind_not_supported()
returns int, but I realized that it's a trick to let you write "return
errdetail()" so you don't have to have "break" which would require one
extra line. Looks fine.

Thanks, committed.

While reviewing the logical decoding of sequences patch, I found a few
more places that could be updated in the new style introduced by this
thread. See attached patch.

On Tue, Jul 20, 2021 at 05:08:53PM +0200, Peter Eisentraut wrote:

While reviewing the logical decoding of sequences patch, I found a few more
places that could be updated in the new style introduced by this thread.
See attached patch.

Those changes look fine. I am spotting one instance in
init_sequence() that looks worth aligning with the others?

Did you consider changing RangeVarCallbackForAlterRelation() or
ExecGrant_Relation() when it came to this thread? Just noticing that,
while going through the code.
--
Michael

On 21.07.21 04:21, Michael Paquier wrote:

On Tue, Jul 20, 2021 at 05:08:53PM +0200, Peter Eisentraut wrote:

While reviewing the logical decoding of sequences patch, I found a few more
places that could be updated in the new style introduced by this thread.
See attached patch.

Those changes look fine. I am spotting one instance in
init_sequence() that looks worth aligning with the others?

I think if you write "ALTER SEQUENCE foo", then "foo is not a sequence"
would be an appropriate error message, so this doesn't need changing.

Did you consider changing RangeVarCallbackForAlterRelation() or
ExecGrant_Relation() when it came to this thread? Just noticing that,
while going through the code.

These might be worth another look, but I'd need to investigate more in
what situations they happen.