Thread
Commits
-
More improvements of error messages about mismatching relkind
- 81d5995b4b78 15.0 landed
-
Improve error messages about mismatching relkind
- 2ed532ee8c47 15.0 landed
-
wrong relkind error messages
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-13T13:54:48Z
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 -
Re: wrong relkind error messages
Robert Haas <robertmhaas@gmail.com> — 2020-04-13T15:06:57Z
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 -
Re: wrong relkind error messages
Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-13T15:13:15Z
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
-
Re: wrong relkind error messages
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-04-13T15:17:54Z
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
-
Re: wrong relkind error messages
Michael Paquier <michael@paquier.xyz> — 2020-04-14T01:32:38Z
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 -
Re: wrong relkind error messages
Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-04-14T22:36:25Z
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
-
Re: wrong relkind error messages
Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-04-14T23:02:08Z
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 -
Re: wrong relkind error messages
Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-15T00:15:53Z
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 -
Re: wrong relkind error messages
Robert Haas <robertmhaas@gmail.com> — 2020-04-15T14:38:24Z
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 -
Re: wrong relkind error messages
Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-04-15T15:07:06Z
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
-
Re: wrong relkind error messages
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-17T12:45:34Z
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
-
Re: wrong relkind error messages
Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-17T13:09:03Z
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
-
Re: wrong relkind error messages
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-06-24T08:12:49Z
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.
-
Re: wrong relkind error messages
Michael Paquier <michael@paquier.xyz> — 2021-07-02T06:25:54Z
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 -
Re: wrong relkind error messages
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-07-02T10:53:08Z
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. -
Re: wrong relkind error messages
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-07-02T16:10:29Z
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/
-
Re: wrong relkind error messages
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-07-08T07:54:58Z
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.
-
Re: wrong relkind error messages
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-07-20T15:08:53Z
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.
-
Re: wrong relkind error messages
Michael Paquier <michael@paquier.xyz> — 2021-07-21T02:21:23Z
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
-
Re: wrong relkind error messages
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-07-21T06:03:29Z
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.