Thread: wrong relkind error messages
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.
This was discussed many years ago in [0], with the same arguments, and
there appeared to have been general agreement to change this, but then
the thread stalled somehow on some technical details.
Attached is another attempt to improve this. I have rewritten the
primary error messages using the principle of "cannot do this with that"
and then added a detail message to show what relkind the object has.
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.
and
-ERROR: "test_foreign_table" is not a table, materialized view, or
TOAST table
+ERROR: relation "test_foreign_table" does not have a visibility map
+DETAIL: "test_foreign_table" is a foreign table.
You can see more instances of this in the test diffs in the attached patch.
In passing, I also changed a few places to use the RELKIND_HAS_STORAGE()
macro. This is related because it allows writing more helpful error
messages, such as in pgstatindex.c.
One question on a detail arose:
check_relation_relkind() in pg_visibility.c accepts RELKIND_RELATION,
RELKIND_MATVIEW, and RELKIND_TOASTVALUE, but pgstatapprox.c only accepts
RELKIND_RELATION and RELKIND_MATVIEW, even though they both look for a
visibility map. Is that an intentional omission? If so, it should be
commented better.
[0]:
https://www.postgresql.org/message-id/flat/AANLkTimR_sZ_wKd1cgqVG1PEvTvdr9j7zD%2B3_NPvfaa_%40mail.gmail.com
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
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
tableinstead.") :
+ (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
Attachment
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] 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] https://www.postgresql.org/message-id/1293803569.19789.6.camel%40fsopti579.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.
Attachment
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
Attachment
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.
Attachment
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
Attachment
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.