Thread: and it's not a bunny rabbit, either
In reviewing the work Shigeru Hanada has done on SQL/MED, it's come to my attention that we have a lot of error messages that use the error code ERRCODE_WRONG_OBJECT_TYPE and have text like this: "%s" is not a table "%s" is not an index or even better: "%s" is not a table, view, composite type, or index which, once we have foreign tables, needs to be changed to read: "%s" is not a table, view, composite type, index, or foreign table If we someday add materialized views, it'll need to mention that, too. In the particular case I'm looking at right now (renameatt_internal), a more informative error message might be something like "system-generated attribute names may not be altered", and maybe that's actually a good way to go in that particular case. But it seems somewhat painful to make this work for ATSimplePermissions() and ATSimplePermissionsRelationOrIndex(); in many cases, there's not much to say beyond the fact that the particular alter table subcommand isn't supported by the object type to which the user has attempted to apply it. Still, I'm a bit wondering if there's some more generic way we can phrase the problem. Could we get away with something as simple as "requested operation is not supported for <plural-form-of-object-type>"? In some sense that's less informative, because it doesn't tell you which object types DO support the requested operation, but it's not clear that you care about that. If you are trying to drop a column from a view, the fact that it would be possible to drop a column from a table is cold comfort. The advantage of this method is that you need only N translatable strings (one per relkind), rather than 2^N (one per subset of the universe of relkinds). A slightly more granular version of this would be to make the caller pass in a string indicating what the requested operation actually is, so that you can say something like "<plural-form-of-object-type> do not support <requested-operation>" or "<requested-operation> is not supported by <plural-form-of-object-type>" (e.g. "views do not support SET NOT NULL"). But that breaks our guideline about assembling translatable strings from small pieces. Maybe it'd be OK if the piece is just a fragment of SQL syntax, though - not sure. Or we can just stick with the way we've been doing it, if I'm the only one who thinks it's icky. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, Dec 26, 2010 at 10:13:35PM -0500, Robert Haas wrote: > In reviewing the work Shigeru Hanada has done on SQL/MED, it's come to > my attention that we have a lot of error messages that use the error > code ERRCODE_WRONG_OBJECT_TYPE and have text like this: > > "%s" is not a table > "%s" is not an index > > or even better: > > "%s" is not a table, view, composite type, or index > > which, once we have foreign tables, needs to be changed to read: > > "%s" is not a table, view, composite type, index, or foreign table > > If we someday add materialized views, it'll need to mention that, too. > In the particular case I'm looking at right now (renameatt_internal), > a more informative error message might be something like > "system-generated attribute names may not be altered", and maybe > that's actually a good way to go in that particular case. But it > seems somewhat painful to make this work for ATSimplePermissions() and > ATSimplePermissionsRelationOrIndex(); in many cases, there's not much > to say beyond the fact that the particular alter table subcommand > isn't supported by the object type to which the user has attempted to > apply it. Still, I'm a bit wondering if there's some more generic way > we can phrase the problem. > > Could we get away with something as simple as "requested operation is > not supported for <plural-form-of-object-type>"? +1 for this. It's clear, informative, and relevant to the error at hand. > In some sense that's > less informative, because it doesn't tell you which object types DO > support the requested operation, but it's not clear that you care > about that. If you are trying to drop a column from a view, the fact > that it would be possible to drop a column from a table is cold > comfort. The advantage of this method is that you need only N > translatable strings (one per relkind), rather than 2^N (one per > subset of the universe of relkinds). > > A slightly more granular version of this would be to make the caller > pass in a string indicating what the requested operation actually is, > so that you can say something like "<plural-form-of-object-type> do > not support <requested-operation>" or "<requested-operation> is not > supported by <plural-form-of-object-type>" (e.g. "views do not support > SET NOT NULL"). But that breaks our guideline about assembling > translatable strings from small pieces. Maybe it'd be OK if the piece > is just a fragment of SQL syntax, though - not sure. > > Or we can just stick with the way we've been doing it, if I'm the only > one who thinks it's icky. You're not the only one. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Mon, Dec 27, 2010 at 12:13, Robert Haas <robertmhaas@gmail.com> wrote: > Could we get away with something as simple as "requested operation is > not supported for <plural-form-of-object-type>"? +1. If so, will we have a function to get object names something like GetPluralFormOfObjectType(Relation rel or char relkind) => char * ? > But that breaks our guideline about assembling > translatable strings from small pieces. Maybe it'd be OK if the piece > is just a fragment of SQL syntax, though - not sure. Agreed. <requested-operation> should be a SQL syntax, so we won't have to translate the part. -- Itagaki Takahiro
On Sun, Dec 26, 2010 at 10:44 PM, Itagaki Takahiro <itagaki.takahiro@gmail.com> wrote: > On Mon, Dec 27, 2010 at 12:13, Robert Haas <robertmhaas@gmail.com> wrote: >> Could we get away with something as simple as "requested operation is >> not supported for <plural-form-of-object-type>"? > > +1. If so, will we have a function to get object names something like > GetPluralFormOfObjectType(Relation rel or char relkind) => char * ? In the interest of keeping things simple for translators, I was thinking we'd just write out a string for each object type: "requested operation is not supported for tables" "requested operation is not supported for views" "requested operation is not supported for indexes" or if we go with the some-assembly required version, perhaps: "tables do not support %s" "views do not support %s" "indexes do not support %s" -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Dec 26, 2010, at 7:55 PM, Robert Haas wrote: > "tables do not support %s" > "views do not support %s" > "indexes do not support %s" The more detail we can give, the better, of course. Nothing's more frustrating than having a command with an error like,"Object does not support requested operation." Thanks, computer program: "Swerved off road, hit tree" is about as useful. -- -- Christophe Pettus xof@thebuild.com
Robert Haas <robertmhaas@gmail.com> writes:
> or if we go with the some-assembly required version, perhaps:
> "tables do not support %s"
> "views do not support %s"
> "indexes do not support %s"
+1 for that way. Although personally I'd reverse the phrasing:
/* translator: %s is the name of a SQL command */%s does not support tables
regards, tom lane
On Mon, Dec 27, 2010 at 10:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> or if we go with the some-assembly required version, perhaps: > >> "tables do not support %s" >> "views do not support %s" >> "indexes do not support %s" > > +1 for that way. Although personally I'd reverse the phrasing: > > /* translator: %s is the name of a SQL command */ > %s does not support tables To me, it seems as though it's the object which doesn't support the operation, rather than the other way around. Reversing it makes most sense to me in cases where it's an implementation restriction, such as "DROP COLUMN does not support views". But at least to me, the phrasing "SET WITHOUT CLUSTER does not support views" suggests that SET WITHOUT CLUSTER is somehow defective, which is not really the message I think we want to convey there. But maybe we need some other votes. I took a crack at implementing this and the results were mixed - see attached patch. It works pretty well for the most part, but there are a couple of warts. For ALTER TABLE commands like DROP COLUMN and SET WITHOUT CLUSTER the results look pretty good, and I find the new error messages a definite improvement over the old ones. It's not quite so good for setting reloptions or attoptions. You get something like: ERROR: views do not support SET (...) ERROR: views do not support ALTER COLUMN SET (...) ...which might actually be OK, if not fantastically wonderful. One might think of mitigating this problem by writing "ALTER TABLE SET (...)" rather than just "SET (...)", but that's not easily done because there are several equivalent forms (for example, a view can be modified with either ALTER VIEW or ALTER TABLE, for historical - or perhaps hysterical - reasons). An even bigger problem is this: rhaas=# alter view v add primary key (a); ERROR: views do not support ADD INDEX The problem is that alter table actions AT_AddIndex and AT_AddConstraint don't tie neatly back to a particular piece of syntax. The message as written isn't incomprehensible (especially if you're reading it in English) but it definitely leaves something to be desired. Ideas? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Mon, Dec 27, 2010 at 2:06 PM, Robert Haas <robertmhaas@gmail.com> wrote: > The problem is that alter table actions AT_AddIndex and > AT_AddConstraint don't tie neatly back to a particular piece of > syntax. The message as written isn't incomprehensible (especially if > you're reading it in English) but it definitely leaves something to be > desired. > > Ideas? Here's a somewhat more complete patch implementing this concept, plus adding additional messages for non-support of constraints, rules, and triggers. More could be done in this vein, but this picks a decent fraction of the low-hanging fruit. I've had to change some of the heap_open(rv) calls to relation_open(rv) to avoid having the former throw the wrong error message before the latter kicks in. I think there might be stylistic objections to that, but I'm not sure what else to propose. I'm actually pretty suspicious that many of the heap_open(rv) calls I *didn't* change are either already a little iffy or likely to become so once the SQL/MED stuff for foreign tables goes in. They make it easy to forget that we've got a whole pile of relkinds and you actually need to really think about which ones you can handle. For example, on unpatched head: rhaas=# create view v as select 1 as a; CREATE VIEW rhaas=# cluster v; ERROR: there is no previously clustered index for table "v" The error message is demonstrably correct in the sense that, first, there isn't any table v, only a view v, so surely table v has no clustered index - or anything else; and second, even if we construe table "v" to mean view "v", it is certainly right to say it has no clustered index because it does not - and can not - have any indexes at all. But as undeniably true as that error message is, it's a bad error message. With the patch: rhaas=# cluster v; ERROR: views do not support CLUSTER That's more like it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On 29.12.2010 06:54, Robert Haas wrote: > With the patch: > > rhaas=# cluster v; > ERROR: views do not support CLUSTER "do not support" sounds like a missing feature, rather than a nonsensical command. How about something like "CLUSTER cannot be used on views" The patch changes a bunch of heap_openrv() calls to relation_openrv(). Perhaps it would be better make the error message something like "\"%s\" is not a table", and keep the callers unchanged. It's not particularly useful to repeat the command in the error message, the user should know what command he issued. Even if it's buried deep in a PL/pgSQL function or something, it should be clear from the context lines. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 29.12.2010 06:54, Robert Haas wrote: >> >> With the patch: >> >> rhaas=# cluster v; >> ERROR: views do not support CLUSTER > > "do not support" sounds like a missing feature, rather than a nonsensical > command. How about something like "CLUSTER cannot be used on views" I'm fine with flipping the ordering around. I think I like it marginally better this way, but you and Tom both seem to prefer the opposite ordering, ergo so be it (barring a sudden influx of contrary votes). > The patch changes a bunch of heap_openrv() calls to relation_openrv(). > Perhaps it would be better make the error message something like "\"%s\" is > not a table", and keep the callers unchanged. It's not particularly useful > to repeat the command in the error message, the user should know what > command he issued. Even if it's buried deep in a PL/pgSQL function or > something, it should be clear from the context lines. Did you read the whole thread? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 29.12.2010 13:17, Robert Haas wrote: > Did you read the whole thread? Ah, sorry: > I've had to change some of the heap_open(rv) calls to > relation_open(rv) to avoid having the former throw the wrong error > message before the latter kicks in. I think there might be stylistic > objections to that, but I'm not sure what else to propose. I'm > actually pretty suspicious that many of the heap_open(rv) calls I > *didn't* change are either already a little iffy or likely to become > so once the SQL/MED stuff for foreign tables goes in. They make it > easy to forget that we've got a whole pile of relkinds and you > actually need to really think about which ones you can handle. Hmm, I believe the idea of heap_open is to check that the relation is backed by a heap that you can read with heap_beginscan+heap_next. At the moment that includes normal tables, sequences and toast tables. Foreign tables would not fall into that category. Yeah, you're right that most of the callers of heap_open actually want to a tighter check than that. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Hmm, I believe the idea of heap_open is to check that the relation is
> backed by a heap that you can read with heap_beginscan+heap_next. At the
> moment that includes normal tables, sequences and toast tables. Foreign
> tables would not fall into that category.
I don't believe that that definition is documented anyplace; if we
decide that's what we want it to mean, some code comments would be in
order.
> Yeah, you're right that most of the callers of heap_open actually want
> to a tighter check than that.
I think probably most of the physical calls of heap_open are actually
associated with system catalog accesses, and the fact that the code says
heap_open not relation_open has got more to do with copy&paste than any
real thought about what we're specifying.
regards, tom lane
On Dec 29, 2010, at 12:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:Hmm, I believe the idea of heap_open is to check that the relation isbacked by a heap that you can read with heap_beginscan+heap_next. At themoment that includes normal tables, sequences and toast tables. Foreigntables would not fall into that category.
I don't believe that that definition is documented anyplace; if we
decide that's what we want it to mean, some code comments would be in
order.
The existing comments mention that callers must check that the return value is not a view, if they care. So if there is currently a single coherent definition for what heap_open is supposed to do, it's clearly NOT the one Heikki proposes. My guess is that reality is closer to your theory of "what got cut-and-pasted".
...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> The existing comments mention that callers must check that the return
> value is not a view, if they care. So if there is currently a single
> coherent definition for what heap_open is supposed to do, it's clearly
> NOT the one Heikki proposes. My guess is that reality is closer to
> your theory of "what got cut-and-pasted".
Well, reality is that in the beginning there was heap_open and
index_open, and nothing else. And there weren't views, so basically
those two functions covered all the interesting types of relations.
We got to the current state of affairs by a series of whatever were the
least invasive code changes at the time; nobody's ever taken a step
back and tried to define what "heap_open" ought to allow from the
standpoint of first principles.
In practice I think it would make sense if heap_open accepts all
relation types on which you can potentially do either a heapscan or
indexscan (offhand those should be the same set of relkinds, I think;
so this is the same in effect as Heikki's proposal, but phrased
differently). So it would have to start rejecting views, and we'd need
to go looking for the consequences of that.
regards, tom lane
Excerpts from Tom Lane's message of mié dic 29 16:29:45 -0300 2010: > In practice I think it would make sense if heap_open accepts all > relation types on which you can potentially do either a heapscan or > indexscan (offhand those should be the same set of relkinds, I think; > so this is the same in effect as Heikki's proposal, but phrased > differently). So it would have to start rejecting views, and we'd need > to go looking for the consequences of that. This seems a very good idea, but I think we shouldn't let it sink the current patch. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Tom Lane's message of mié dic 29 16:29:45 -0300 2010:
>> In practice I think it would make sense if heap_open accepts all
>> relation types on which you can potentially do either a heapscan or
>> indexscan (offhand those should be the same set of relkinds, I think;
>> so this is the same in effect as Heikki's proposal, but phrased
>> differently). So it would have to start rejecting views, and we'd need
>> to go looking for the consequences of that.
> This seems a very good idea, but I think we shouldn't let it sink the
> current patch.
No, but possibly regularizing what heap_open is defined to do would make
Robert's patch simpler.
regards, tom lane
On Wed, Dec 29, 2010 at 3:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: >> Excerpts from Tom Lane's message of mié dic 29 16:29:45 -0300 2010: >>> In practice I think it would make sense if heap_open accepts all >>> relation types on which you can potentially do either a heapscan or >>> indexscan (offhand those should be the same set of relkinds, I think; >>> so this is the same in effect as Heikki's proposal, but phrased >>> differently). So it would have to start rejecting views, and we'd need >>> to go looking for the consequences of that. > >> This seems a very good idea, but I think we shouldn't let it sink the >> current patch. > > No, but possibly regularizing what heap_open is defined to do would make > Robert's patch simpler. I think that any meaning we assign to heap_open is going to be 95% arbitrary and of little practical help. There are at least six different sets of object classes which to which a particular commands or alter table subcommands can be legally applied: (1) tables only, (2) views only, (3) tables and views, (4) tables and indexes, (5) tables and composite types, (6) tables, views, and composite types. Adding foreign tables promises to add several more combinations immediately and likely more down the line; if we add materialized views, that'll add a bunch more. There's not really any single definition that's going to be a silver bullet. I think the best thing to do might be to get RID of heap_open(rv) and always use relation_openrv plus an appropriate relkind test. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 29.12.2010 06:54, Robert Haas wrote: >> >> With the patch: >> >> rhaas=# cluster v; >> ERROR: views do not support CLUSTER > > "do not support" sounds like a missing feature, rather than a nonsensical > command. How about something like "CLUSTER cannot be used on views" In the latest version of this patch, I created four translatable strings per object type: <blats> do not support %s (where %s is an SQL command) <blats> do not support constraints <blats> do not support rules <blats> do not support triggers It's reasonable enough to write "CLUSTER cannot be used on views", but does "constraints cannot be used on views" seems more awkward to me. Or do we think that's OK? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Dec 29, 2010 at 04:53:47PM -0500, Robert Haas wrote: > On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.com> wrote: > > On 29.12.2010 06:54, Robert Haas wrote: > >> > >> With the patch: > >> > >> rhaas=# cluster v; > >> ERROR: views do not support CLUSTER > > > > "do not support" sounds like a missing feature, rather than a nonsensical > > command. How about something like "CLUSTER cannot be used on views" > > In the latest version of this patch, I created four translatable > strings per object type: > > <blats> do not support %s (where %s is an SQL command) > <blats> do not support constraints > <blats> do not support rules > <blats> do not support triggers > > It's reasonable enough to write "CLUSTER cannot be used on views", but > does "constraints cannot be used on views" seems more awkward to me. > Or do we think that's OK? That particular one looks good insofar as it describes reality. With predicate locks, etc., it may become untrue later, though :) Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Wed, Dec 29, 2010 at 5:14 PM, David Fetter <david@fetter.org> wrote: > On Wed, Dec 29, 2010 at 04:53:47PM -0500, Robert Haas wrote: >> On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas >> <heikki.linnakangas@enterprisedb.com> wrote: >> > On 29.12.2010 06:54, Robert Haas wrote: >> >> >> >> With the patch: >> >> >> >> rhaas=# cluster v; >> >> ERROR: views do not support CLUSTER >> > >> > "do not support" sounds like a missing feature, rather than a nonsensical >> > command. How about something like "CLUSTER cannot be used on views" >> >> In the latest version of this patch, I created four translatable >> strings per object type: >> >> <blats> do not support %s (where %s is an SQL command) >> <blats> do not support constraints >> <blats> do not support rules >> <blats> do not support triggers >> >> It's reasonable enough to write "CLUSTER cannot be used on views", but >> does "constraints cannot be used on views" seems more awkward to me. >> Or do we think that's OK? > > That particular one looks good insofar as it describes reality. With > predicate locks, etc., it may become untrue later, though :) After further thought, I think it makes sense to change this around a bit and create a family of functions that can be invoked like this: void check_relation_for_FEATURE_support(Relation rel); ...where FEATURE is constraint, trigger, rule, index, etc. The function will be defined to throw an error if the relation isn't of a type that can support the named feature. The error message will be of the form: constraints can only be used on tables triggers can be used only on tables and views etc. This avoids the need to define a separate error message for each unsupported relkind, and I think it's just as informative as, e.g., "constraints cannot be used on <whatever object type you tried to invoke it on>". We can adopt the same language for commands, e.g.: "CLUSTER can only be used on tables". Comments? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of jue dic 30 12:47:42 -0300 2010: > After further thought, I think it makes sense to change this around a > bit and create a family of functions that can be invoked like this: > > void check_relation_for_FEATURE_support(Relation rel); > > ...where FEATURE is constraint, trigger, rule, index, etc. The > function will be defined to throw an error if the relation isn't of a > type that can support the named feature. The error message will be of > the form: > > constraints can only be used on tables > triggers can be used only on tables and views > etc. So this will create a combinatorial explosion of strings to translate? I liked the other idea because the number of translatable strings was kept within reasonable bounds. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Dec 30, 2010 at 11:00 AM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Excerpts from Robert Haas's message of jue dic 30 12:47:42 -0300 2010: > >> After further thought, I think it makes sense to change this around a >> bit and create a family of functions that can be invoked like this: >> >> void check_relation_for_FEATURE_support(Relation rel); >> >> ...where FEATURE is constraint, trigger, rule, index, etc. The >> function will be defined to throw an error if the relation isn't of a >> type that can support the named feature. The error message will be of >> the form: >> >> constraints can only be used on tables >> triggers can be used only on tables and views >> etc. > > So this will create a combinatorial explosion of strings to translate? > I liked the other idea because the number of translatable strings was > kept within reasonable bounds. No, quite the opposite. With the other approach, you needed: constraints cannot be used on views constraints cannot be used on composite types constraints cannot be used on TOAST tables constraints cannot be used on indexes constraints cannot be used on foreign tables With this, you just need: constraints can only be used on tables -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> After further thought, I think it makes sense to change this around a
> bit and create a family of functions that can be invoked like this:
> void check_relation_for_FEATURE_support(Relation rel);
That seems like a reasonable idea, but ...
> ... The error message will be of the form:
> constraints can only be used on tables
> triggers can be used only on tables and views
> etc.
> This avoids the need to define a separate error message for each
> unsupported relkind, and I think it's just as informative as, e.g.,
> "constraints cannot be used on <whatever object type you tried to
> invoke it on>". We can adopt the same language for commands, e.g.:
> "CLUSTER can only be used on tables".
ISTM there are four things we might potentially want to state in the
error message: the feature/operation you tried to apply, the name of the
object you tried to apply it to, the type of that object, and the set of
object types that the feature/operation will actually work for. Our
current wording ("foo is not a table or view") covers the second and
fourth of these, though the fourth is stated rather awkwardly. Your
proposal above covers the first and fourth. I'm not happy about leaving
out the object name, because there are going to be cases where people
get this type of error out of a long sequence or nest of operations and
it's not clear what it's talking about. It'd probably be okay to leave
out the actual object type as long as you include its name, though.
One possibility is to break it down like this:
ERROR: foo is a sequenceDETAIL: Triggers can only be used on tables and views.
This could still be emitted by a function such as you suggest, and
indeed that would be the most practical way from both a consistency
and code-size standpoint.
regards, tom lane
Excerpts from Tom Lane's message of jue dic 30 13:49:20 -0300 2010: > One possibility is to break it down like this: > > ERROR: foo is a sequence > DETAIL: Triggers can only be used on tables and views. > > This could still be emitted by a function such as you suggest, and > indeed that would be the most practical way from both a consistency > and code-size standpoint. This seems good to me. There will only be as many messages as relkinds we have, plus as many as "features" there are. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Dec 30, 2010 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > One possibility is to break it down like this: > > ERROR: foo is a sequence > DETAIL: Triggers can only be used on tables and views. > > This could still be emitted by a function such as you suggest, and > indeed that would be the most practical way from both a consistency > and code-size standpoint. Great idea. I should have thought of that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Dec 30, 2010 at 12:32 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Dec 30, 2010 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> One possibility is to break it down like this: >> >> ERROR: foo is a sequence >> DETAIL: Triggers can only be used on tables and views. >> >> This could still be emitted by a function such as you suggest, and >> indeed that would be the most practical way from both a consistency >> and code-size standpoint. > > Great idea. I should have thought of that. On further reflection, this can still turn into a laundry list in certain cases. DETAIL: You can only comment on columns of tables, views, and composite types. seems less helpful than: DETAIL: Comments on relations with system-generated column names are not supported. I think that for rules, triggers, constraints, and anything that only works on a single relkind, we can't do much better than to list the specific object types. But where there's some sort of guiding principle involved I think we'd do well to articulate it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On further reflection, this can still turn into a laundry list in certain cases.
> DETAIL: You can only comment on columns of tables, views, and composite types.
> seems less helpful than:
> DETAIL: Comments on relations with system-generated column names are
> not supported.
> I think that for rules, triggers, constraints, and anything that only
> works on a single relkind, we can't do much better than to list the
> specific object types. But where there's some sort of guiding
> principle involved I think we'd do well to articulate it.
I'm unconvinced, because the "guiding principle" is likely to be an
implementation detail that won't actually mean much to users. Your
example above is a case in point --- I do *not* think the average
user will see that as an improvement.
regards, tom lane
On Thu, Dec 30, 2010 at 8:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On further reflection, this can still turn into a laundry list in certain cases. > >> DETAIL: You can only comment on columns of tables, views, and composite types. > >> seems less helpful than: > >> DETAIL: Comments on relations with system-generated column names are >> not supported. > >> I think that for rules, triggers, constraints, and anything that only >> works on a single relkind, we can't do much better than to list the >> specific object types. But where there's some sort of guiding >> principle involved I think we'd do well to articulate it. > > I'm unconvinced, because the "guiding principle" is likely to be an > implementation detail that won't actually mean much to users. Your > example above is a case in point --- I do *not* think the average > user will see that as an improvement. I think this thread has worked itself around to where it's entirely pointless. My original complaint was about error messages like this: "%s" is not a table, view, composite type, or index which, once we have foreign tables, needs to be changed to read: "%s" is not a table, view, composite type, index, or foreign table I think that message is the epitome of worthless, and several other people agreed. After various proposals of greater and lesser merit, we've somehow worked around to the suggestion that this should be reworded to: ERROR: "%s" is a sequence DETAIL: Only attributes of tables, views, composite types, indexes, or foreign tables can be renamed. While that may be a marginal improvement in clarity, it does absolutely nothing to address my original complaint, which is that adding a relkind forces trivial revisions of messages all over the system, some of which are already excessively long-winded. This message also does nothing to help the user understand WHY we don't allow renaming the attributes of his sequence or TOAST table, whereas the proposed revision does. The absolute worst offenders are messages of the form: <blah> is not supported on X, Y, Z, or T. which now have to be revised to read: <blah> is not supported on X,Y, Z, T, or W. This problem could be avoided by writing: <blah> is supported on A and B Or: <blah> is supported only for relation types which quack -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> I think this thread has worked itself around to where it's entirely
> pointless.
I understand your frustration, but it's not clear to me that there *is*
any simple solution to this problem. Fundamentally, adding new relkinds
to the system is always going to require running around and looking at a
lot of code to see what's affected; and that goes for the error messages
too. I put no stock at all in the idea that writing a "guiding
principle" in the error messages will avoid anything, because as often
as not, adding a fundamentally new relkind is going to involve some
tweaking of what those principles are.
> ... This message also does nothing to help the user understand WHY we don't
> allow renaming the attributes of his sequence or TOAST table, whereas
> the proposed revision does.
I remain unconvinced that the average user cares, or will be able to
extrapolate the message to understand what's supported or not, even
if he does care about the reason for the restriction.
regards, tom lane
On Thu, Dec 30, 2010 at 9:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I think this thread has worked itself around to where it's entirely >> pointless. > > I understand your frustration, but it's not clear to me that there *is* > any simple solution to this problem. Fundamentally, adding new relkinds > to the system is always going to require running around and looking at a > lot of code to see what's affected; and that goes for the error messages > too. I put no stock at all in the idea that writing a "guiding > principle" in the error messages will avoid anything, because as often > as not, adding a fundamentally new relkind is going to involve some > tweaking of what those principles are. I think that's true in some cases but not all. The system-generated attribute names thing actually applies in several cases, and I think it's pretty cut-and-dried. When you get into something like which kinds of relations support triggers, that's a lot more arbitrary. >> ... This message also does nothing to help the user understand WHY we don't >> allow renaming the attributes of his sequence or TOAST table, whereas >> the proposed revision does. > > I remain unconvinced that the average user cares, or will be able to > extrapolate the message to understand what's supported or not, even > if he does care about the reason for the restriction. I'm convinced, but that only makes one of us. I think for now what I had better do is try to get this SQL/MED patch finished up by soldiering through this mess rather than trying to fix it. I think it's going to be kind of ugly, but we haven't got another plan then we're just going to have to live with the ugliness. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of vie dic 31 02:07:18 -0300 2010: > I think that's true in some cases but not all. The system-generated > attribute names thing actually applies in several cases, and I think > it's pretty cut-and-dried. When you get into something like which > kinds of relations support triggers, that's a lot more arbitrary. I think part of the problem with the phrase "system-generated attribute names" is: how are users supposed to figure out what that means, and what relation types it applies to? It seems entirely non-obvious. > I think for now what I > had better do is try to get this SQL/MED patch finished up by > soldiering through this mess rather than trying to fix it. I think > it's going to be kind of ugly, but we haven't got another plan then > we're just going to have to live with the ugliness. Perhaps it would make sense to fix the cases for which there is a consensus, and leave the rest alone for now. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Fri, Dec 31, 2010 at 8:10 AM, Alvaro Herrera <alvherre@commandprompt.com> wrote: >> I think for now what I >> had better do is try to get this SQL/MED patch finished up by >> soldiering through this mess rather than trying to fix it. I think >> it's going to be kind of ugly, but we haven't got another plan then >> we're just going to have to live with the ugliness. > > Perhaps it would make sense to fix the cases for which there is a > consensus, and leave the rest alone for now. Sure. Which cases do we have consensus on? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote: > No, quite the opposite. With the other approach, you needed: > > constraints cannot be used on views > constraints cannot be used on composite types > constraints cannot be used on TOAST tables > constraints cannot be used on indexes > constraints cannot be used on foreign tables > > With this, you just need: > > constraints can only be used on tables At the beginning of this thread you said that the error messages should focus on what you tried to do, not what you could do instead. Also, in this particular case, the user could very well assume that a TOAST table or a foreign table is a table.
On tor, 2010-12-30 at 11:49 -0500, Tom Lane wrote: > ISTM there are four things we might potentially want to state in the > error message: the feature/operation you tried to apply, the name of > the object you tried to apply it to, the type of that object, and the > set of object types that the feature/operation will actually work for. I think the latter should be completely omitted unless it's exceptionally important. You can construct pretty silly things down this line: ERROR: permission denied for relation "x" ERROR: relation "x" does not exist vs. ERROR: you only have permission on relation a, b, c ERROR: only the following relations exist: a, b, c
On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote: >> No, quite the opposite. With the other approach, you needed: >> >> constraints cannot be used on views >> constraints cannot be used on composite types >> constraints cannot be used on TOAST tables >> constraints cannot be used on indexes >> constraints cannot be used on foreign tables >> >> With this, you just need: >> >> constraints can only be used on tables > > At the beginning of this thread you said that the error messages should > focus on what you tried to do, not what you could do instead. Yeah, and I still believe that. I'm having difficulty coming up with a workable approach, though. It would be simple enough if we could write: /* translator: first %s is a feature, second %s is a relation type */ %s cannot be used on %s ...but I think this is likely to cause some translation headaches. > Also, in this particular case, the user could very well assume that a > TOAST table or a foreign table is a table. There's a limited amount we can do about confused users, but it is true that the negative phrasing is better for that case. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Le 01/01/2011 06:05, Robert Haas a écrit : > On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >> On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote: >>> No, quite the opposite. With the other approach, you needed: >>> >>> constraints cannot be used on views >>> constraints cannot be used on composite types >>> constraints cannot be used on TOAST tables >>> constraints cannot be used on indexes >>> constraints cannot be used on foreign tables >>> >>> With this, you just need: >>> >>> constraints can only be used on tables >> >> At the beginning of this thread you said that the error messages should >> focus on what you tried to do, not what you could do instead. > > Yeah, and I still believe that. I'm having difficulty coming up with > a workable approach, though. It would be simple enough if we could > write: > > /* translator: first %s is a feature, second %s is a relation type */ > %s cannot be used on %s > > ...but I think this is likely to cause some translation headaches. > Actually, this is simply not translatable in some languages. We had the same issue on pgAdmin, and we resolved this by having quite a big number of new strings to translate. Harder one time for the translator, but results in a much better experience for the user. >> Also, in this particular case, the user could very well assume that a >> TOAST table or a foreign table is a table. > > There's a limited amount we can do about confused users, but it is > true that the negative phrasing is better for that case. > It's at least better for the translator. -- Guillaumehttp://www.postgresql.frhttp://dalibo.com
On Sat, Jan 1, 2011 at 9:53 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Le 01/01/2011 06:05, Robert Haas a écrit : >> On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>> On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote: >>>> No, quite the opposite. With the other approach, you needed: >>>> >>>> constraints cannot be used on views >>>> constraints cannot be used on composite types >>>> constraints cannot be used on TOAST tables >>>> constraints cannot be used on indexes >>>> constraints cannot be used on foreign tables >>>> >>>> With this, you just need: >>>> >>>> constraints can only be used on tables >>> >>> At the beginning of this thread you said that the error messages should >>> focus on what you tried to do, not what you could do instead. >> >> Yeah, and I still believe that. I'm having difficulty coming up with >> a workable approach, though. It would be simple enough if we could >> write: >> >> /* translator: first %s is a feature, second %s is a relation type */ >> %s cannot be used on %s >> >> ...but I think this is likely to cause some translation headaches. > > Actually, this is simply not translatable in some languages. We had the > same issue on pgAdmin, and we resolved this by having quite a big number > of new strings to translate. Harder one time for the translator, but > results in a much better experience for the user. Is it in any better if we write one string per feature, like this: constraints cannot be used on %s triggers cannot be used on %s ...where %s is a plural object type (views, foreign tables, etc.). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Le 01/01/2011 16:00, Robert Haas a écrit : > On Sat, Jan 1, 2011 at 9:53 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> Le 01/01/2011 06:05, Robert Haas a écrit : >>> On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>>> On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote: >>>>> No, quite the opposite. With the other approach, you needed: >>>>> >>>>> constraints cannot be used on views >>>>> constraints cannot be used on composite types >>>>> constraints cannot be used on TOAST tables >>>>> constraints cannot be used on indexes >>>>> constraints cannot be used on foreign tables >>>>> >>>>> With this, you just need: >>>>> >>>>> constraints can only be used on tables >>>> >>>> At the beginning of this thread you said that the error messages should >>>> focus on what you tried to do, not what you could do instead. >>> >>> Yeah, and I still believe that. I'm having difficulty coming up with >>> a workable approach, though. It would be simple enough if we could >>> write: >>> >>> /* translator: first %s is a feature, second %s is a relation type */ >>> %s cannot be used on %s >>> >>> ...but I think this is likely to cause some translation headaches. >> >> Actually, this is simply not translatable in some languages. We had the >> same issue on pgAdmin, and we resolved this by having quite a big number >> of new strings to translate. Harder one time for the translator, but >> results in a much better experience for the user. > > Is it in any better if we write one string per feature, like this: > > constraints cannot be used on %s > triggers cannot be used on %s > > ...where %s is a plural object type (views, foreign tables, etc.). > If %s was a singular object, it would be an issue for french. But for plural form, it won't be an issue. Not sure it would be the same in other languages. IIRC from my student years, german could have an issue here. -- Guillaumehttp://www.postgresql.frhttp://dalibo.com
On lör, 2011-01-01 at 00:05 -0500, Robert Haas wrote: > Yeah, and I still believe that. I'm having difficulty coming up with > a workable approach, though. I don't see anything wrong with having 20 or 30 messages of variants of "foo cannot be used on bar" without placeholders.
On lör, 2011-01-01 at 10:00 -0500, Robert Haas wrote: > Is it in any better if we write one string per feature, like this: > > constraints cannot be used on %s > triggers cannot be used on %s > > ...where %s is a plural object type (views, foreign tables, etc.). No, this won't work.
On Sat, Jan 1, 2011 at 4:28 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On lör, 2011-01-01 at 00:05 -0500, Robert Haas wrote: >> Yeah, and I still believe that. I'm having difficulty coming up with >> a workable approach, though. > > I don't see anything wrong with having 20 or 30 messages of variants of > > "foo cannot be used on bar" > > without placeholders. Well, that's OK with me. It seems a little grotty, but manageably so.Questions: 1. Should we try to include the name of the object? If so, how? 2. Can we have a variant with an SQL-command-fragment parameter? %s cannot be used on views where %s might be CLUSTER, DROP COLUMN, etc. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On lör, 2011-01-01 at 17:21 -0500, Robert Haas wrote: > > I don't see anything wrong with having 20 or 30 messages of variants of > > > > "foo cannot be used on bar" > > > > without placeholders. > > Well, that's OK with me. It seems a little grotty, but manageably so. > Questions: > > 1. Should we try to include the name of the object? If so, how? Hmm. There is a bit of a difference in my mind between, say, constraints cannot be used on sequences constraint "foo" cannot be used on sequence "bar" the latter leaving open the question whether some other combination might work. > 2. Can we have a variant with an SQL-command-fragment parameter? > > %s cannot be used on views > where %s might be CLUSTER, DROP COLUMN, etc. That's OK; we do that in several other places.
On Sun, Jan 2, 2011 at 4:45 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On lör, 2011-01-01 at 17:21 -0500, Robert Haas wrote: >> > I don't see anything wrong with having 20 or 30 messages of variants of >> > >> > "foo cannot be used on bar" >> > >> > without placeholders. >> >> Well, that's OK with me. It seems a little grotty, but manageably so. >> Questions: >> >> 1. Should we try to include the name of the object? If so, how? > > Hmm. There is a bit of a difference in my mind between, say, > > constraints cannot be used on sequences > > constraint "foo" cannot be used on sequence "bar" > > the latter leaving open the question whether some other combination > might work. Yeah, that's no good. Maybe there's a good way to clear things up with an errdetail(), though I'm having a hard time thinking how to phrase it. ERROR: sequence "%s" does not support the requested operation DETAIL: Constraints are not supported on sequences. ERROR: constraints are not supported on sequences DETAIL: "%s" is a sequence. ERROR: "%s" is a sequence DETAIL: Constraints and sequences are like water and oil, dude. >> 2. Can we have a variant with an SQL-command-fragment parameter? >> >> %s cannot be used on views >> where %s might be CLUSTER, DROP COLUMN, etc. > > That's OK; we do that in several other places. Cool. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of lun ene 03 12:21:44 -0300 2011: > Yeah, that's no good. Maybe there's a good way to clear things up > with an errdetail(), though I'm having a hard time thinking how to > phrase it. > > ERROR: sequence "%s" does not support the requested operation > DETAIL: Constraints are not supported on sequences. This seems backwards to me: the "detail" is more general than the main message. > ERROR: constraints are not supported on sequences > DETAIL: "%s" is a sequence. This one seems good -- the detail message gives the detail. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support