Re: [HACKERS] and it's not a bunny rabbit, either
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 The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Sun, Jan 2, 2011 at 4:45 PM, Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Sat, Jan 1, 2011 at 4:28 PM, Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
Le 01/01/2011 16:00, Robert Haas a écrit : > On Sat, Jan 1, 2011 at 9:53 AM, Guillaume Lelarge > wrote: >> Le 01/01/2011 06:05, Robert Haas a écrit : >>> On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut 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. -- Guillaume http://www.postgresql.fr http://dalibo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Sat, Jan 1, 2011 at 9:53 AM, Guillaume Lelarge wrote: > Le 01/01/2011 06:05, Robert Haas a écrit : >> On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
Le 01/01/2011 06:05, Robert Haas a écrit : > On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut 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. -- Guillaume http://www.postgresql.fr http://dalibo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Fri, Dec 31, 2010 at 8:10 AM, Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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 The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Thu, Dec 30, 2010 at 9:30 PM, Tom Lane wrote: > Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Thu, Dec 30, 2010 at 8:58 PM, Tom Lane wrote: > Robert Haas 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: is not supported on X, Y, Z, or T. which now have to be revised to read: is not supported on X,Y, Z, T, or W. This problem could be avoided by writing: is supported on A and B Or: is supported only for relation types which quack -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Thu, Dec 30, 2010 at 12:32 PM, Robert Haas wrote: > On Thu, Dec 30, 2010 at 11:49 AM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Thu, Dec 30, 2010 at 11:49 AM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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 The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
Robert Haas 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 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 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.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Thu, Dec 30, 2010 at 11:00 AM, Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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 The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Wed, Dec 29, 2010 at 5:14 PM, David Fetter 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 >> 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: >> >> do not support %s (where %s is an SQL command) >> do not support constraints >> do not support rules >> 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 ". 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Wed, Dec 29, 2010 at 04:53:47PM -0500, Robert Haas wrote: > On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas > 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: > > do not support %s (where %s is an SQL command) > do not support constraints > do not support rules > 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 http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas 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: do not support %s (where %s is an SQL command) do not support constraints do not support rules 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Wed, Dec 29, 2010 at 3:01 PM, Tom Lane wrote: > Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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 The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Dec 29, 2010, at 12:49 PM, Tom Lane wrote: > Heikki Linnakangas 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. 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
Re: [HACKERS] and it's not a bunny rabbit, either
Heikki Linnakangas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Mon, Dec 27, 2010 at 2:06 PM, Robert Haas 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
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 249067f..1555b61 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -113,7 +113,9 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
Relation rel;
/* Find and lock the table */
- rel = heap_openrv(stmt->relation, AccessExclusiveLock);
+ rel = relation_openrv(stmt->relation, AccessExclusiveLock);
+ if (rel->rd_rel->relkind != RELKIND_RELATION)
+ ErrorWrongRelkind(rel, WRONG_RELKIND_FOR_COMMAND, "CLUSTER");
tableOid = RelationGetRelid(rel);
diff --git a/src/backend/commands/comment.c b/src/backend/commands/comment.c
index b578818..66df9f8 100644
--- a/src/backend/commands/comment.c
+++ b/src/backend/commands/comment.c
@@ -22,6 +22,7 @@
#include "catalog/pg_shdescription.h"
#include "commands/comment.h"
#include "commands/dbcommands.h"
+#include "commands/tablecmds.h"
#include "libpq/be-fsstubs.h"
#include "miscadmin.h"
#include "parser/parse_func.h"
@@ -583,10 +584,8 @@ CheckAttributeComment(Relation relation)
if (relation->rd_rel->relkind != RELKIND_RELATION &&
relation->rd_rel->relkind != RELKIND_VIEW &&
relation->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or composite type",
- RelationGetRelationName(relation;
+ ErrorWrongRelkind(relation, WRONG_RELKIND_FOR_COMMAND,
+ "COMMENT ON COLUMN");
}
/*
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 7b8bee8..488cc80 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -27,6 +27,7 @@
#include "catalog/pg_type.h"
#include "commands/copy.h"
#include "commands/defrem.h"
+#include "commands/tablecmds.h"
#include "commands/trigger.h"
#include "executor/executor.h"
#include "libpq/libpq.h"
@@ -998,8 +999,19 @@ DoCopy(const CopyStmt *stmt, const char *queryString)
cstate->queryDesc = NULL;
/* Open and lock the relation, using the appropriate lock type. */
- cstate->rel = heap_openrv(stmt->relation,
+ cstate->rel = relation_openrv(stmt->relation,
(is_from ? RowExclusiveLock : AccessShareLock));
+ if (cstate->rel->rd_rel->relkind != RELKIND_RELATION)
+ {
+ if (!is_from && cstate->rel->rd_rel->relkind == RELKIND_VIEW)
+ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("views do not support COPY TO"),
+ errhint("Try the COPY (SELECT ...) TO variant.")));
+ else
+ErrorWrongRelkind(cstate->rel, WRONG_RELKIND_FOR_COMMAND,
+ is_from ? "COPY FROM" : "COPY TO");
+ }
tupDesc = RelationGetDescr(cstate->rel);
@@ -1225,29 +1237,6 @@ DoCopyTo(CopyState cstate)
{
bool pipe = (cstate->filename == NULL);
- if (cstate->rel)
- {
- if (cstate->rel->rd_rel->relkind != RELKIND_RELATION)
- {
- if (cstate->rel->rd_rel->relkind == RELKIND_VIEW)
-ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot copy from view \"%s\"",
-Relation
Re: [HACKERS] and it's not a bunny rabbit, either
On Mon, Dec 27, 2010 at 10:18 AM, Tom Lane wrote:
> Robert Haas 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
diff --git a/src/backend/commands/comment.c b/src/backend/commands/comment.c
index b578818..9701233 100644
--- a/src/backend/commands/comment.c
+++ b/src/backend/commands/comment.c
@@ -22,6 +22,7 @@
#include "catalog/pg_shdescription.h"
#include "commands/comment.h"
#include "commands/dbcommands.h"
+#include "commands/tablecmds.h"
#include "libpq/be-fsstubs.h"
#include "miscadmin.h"
#include "parser/parse_func.h"
@@ -583,10 +584,7 @@ CheckAttributeComment(Relation relation)
if (relation->rd_rel->relkind != RELKIND_RELATION &&
relation->rd_rel->relkind != RELKIND_VIEW &&
relation->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or composite type",
- RelationGetRelationName(relation;
+ ErrorWrongRelationType(relation, "COMMENT ON COLUMN");
}
/*
diff --git a/src/backend/commands/seclabel.c b/src/backend/commands/seclabel.c
index 762bbae..00b64bf 100644
--- a/src/backend/commands/seclabel.c
+++ b/src/backend/commands/seclabel.c
@@ -366,10 +366,7 @@ CheckAttributeSecLabel(Relation relation)
if (relation->rd_rel->relkind != RELKIND_RELATION &&
relation->rd_rel->relkind != RELKIND_VIEW &&
relation->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or composite type",
- RelationGetRelationName(relation;
+ ErrorWrongRelationType(relation, "SECURITY LABEL ON COLUMN");
}
void
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6729d83..ee410c4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -222,6 +222,44 @@ static const struct dropmsgstrings dropmsgstringarray[] = {
{'\0', 0, NULL, NULL, NULL, NULL}
};
+/*
+ * Error-reporting support for wrong-object type errors
+ */
+struct wrongtypestrings
+{
+ char kind;
+ const char *wrongtype_message;
+};
+
+static const struct wrongtypestrings wrongtypestringarray[] = {
+ {RELKIND_RELATION,
+ /* translator: %s is an SQL command */
+ gettext_noop("tables do not support %s")},
+ {RELKIND_SEQUENCE,
+ /* translator: %s is an SQL command */
+ gettext_noop("sequences do not support %s")},
+ {RELKIND_VIEW,
+ /* translator: %s is an SQL command */
+ gettext_noop("views do not support %s")},
+ {RELKIND_INDEX,
+ /* translator: %s is an SQL command */
+ gettext_noop("indexes do not support %s")},
+ {RELKIND_COMPOSITE_TYPE,
+ /* translator: %s is an SQL command */
+ gettext_noop("composite types do not support %s")},
+ {RELKIND_TOASTVALUE,
+ /* translator: %s is an SQL command */
+ gettext_noop("TOAST tables do not support %s")},
+ {'\0', NULL}
+};
+
+
Re: [HACKERS] and it's not a bunny rabbit, either
Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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 x...@thebuild.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Sun, Dec 26, 2010 at 10:44 PM, Itagaki Takahiro wrote: > On Mon, Dec 27, 2010 at 12:13, Robert Haas wrote: >> Could we get away with something as simple as "requested operation is >> not supported for "? > > +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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
On Mon, Dec 27, 2010 at 12:13, Robert Haas wrote: > Could we get away with something as simple as "requested operation is > not supported for "? +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. should be a SQL syntax, so we won't have to translate the part. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] and it's not a bunny rabbit, either
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 "? +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 " do > not support " or " is not > supported by " (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 http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 "? 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 " do not support " or " is not supported by " (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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
