Horton

Thread

  1. and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-27T03:13:35Z

    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

  2. Re: and it's not a bunny rabbit, either

    David Fetter <david@fetter.org> — 2010-12-27T03:33:15Z

    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

  3. Re: and it's not a bunny rabbit, either

    Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2010-12-27T03:44:40Z

    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

  4. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-27T03:55:44Z

    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

  5. Re: and it's not a bunny rabbit, either

    Christophe Pettus <xof@thebuild.com> — 2010-12-27T04:09:41Z

    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

  6. Re: and it's not a bunny rabbit, either

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-27T15:18:21Z

    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

  7. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-27T19:06:52Z

    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

  8. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-29T04:54:46Z

    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

  9. Re: and it's not a bunny rabbit, either

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-12-29T09:09:56Z

    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

  10. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-29T11:17:57Z

    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

  11. Re: and it's not a bunny rabbit, either

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-12-29T17:29:23Z

    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

  12. Re: and it's not a bunny rabbit, either

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-29T17:49:19Z

    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

  13. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-29T18:35:07Z

    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 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

  14. Re: and it's not a bunny rabbit, either

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-29T19:29:45Z

    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

  15. Re: and it's not a bunny rabbit, either

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-12-29T19:42:08Z

    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

  16. Re: and it's not a bunny rabbit, either

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-29T20:01:10Z

    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

  17. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-29T21:04:05Z

    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

  18. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-29T21:53:47Z

    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

  19. Re: and it's not a bunny rabbit, either

    David Fetter <david@fetter.org> — 2010-12-29T22:14:35Z

    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

  20. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-30T15:47:42Z

    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

  21. Re: and it's not a bunny rabbit, either

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-12-30T16:00:02Z

    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

  22. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-30T16:03:35Z

    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

  23. Re: and it's not a bunny rabbit, either

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-30T16:49:20Z

    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 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

  24. Re: and it's not a bunny rabbit, either

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-12-30T17:06:02Z

    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

  25. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-30T17:32:32Z

    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

  26. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-30T19:34:45Z

    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

  27. Re: and it's not a bunny rabbit, either

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-31T01:58:20Z

    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

  28. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-31T02:19:26Z

    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

  29. Re: and it's not a bunny rabbit, either

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-31T02:30:32Z

    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

  30. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-31T05:07:18Z

    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

  31. Re: and it's not a bunny rabbit, either

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-12-31T13:10:07Z

    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

  32. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2010-12-31T13:35:25Z

    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

  33. Re: and it's not a bunny rabbit, either

    Peter Eisentraut <peter_e@gmx.net> — 2010-12-31T13:48:18Z

    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.

  34. Re: and it's not a bunny rabbit, either

    Peter Eisentraut <peter_e@gmx.net> — 2010-12-31T13:52:49Z

    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

  35. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2011-01-01T05:05:30Z

    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

  36. Re: and it's not a bunny rabbit, either

    Guillaume Lelarge <guillaume@lelarge.info> — 2011-01-01T14:53:57Z

    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.


-- 
Guillaume
 http://www.postgresql.fr
 http://dalibo.com

  37. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2011-01-01T15:00:46Z

    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

  38. Re: and it's not a bunny rabbit, either

    Guillaume Lelarge <guillaume@lelarge.info> — 2011-01-01T15:31:29Z

    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.


-- 
Guillaume
 http://www.postgresql.fr
 http://dalibo.com

  39. Re: and it's not a bunny rabbit, either

    Peter Eisentraut <peter_e@gmx.net> — 2011-01-01T21:28:10Z

    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.

  40. Re: and it's not a bunny rabbit, either

    Peter Eisentraut <peter_e@gmx.net> — 2011-01-01T21:28:34Z

    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.

  41. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2011-01-01T22:21:38Z

    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

  42. Re: and it's not a bunny rabbit, either

    Peter Eisentraut <peter_e@gmx.net> — 2011-01-02T21:45:29Z

    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.

  43. Re: and it's not a bunny rabbit, either

    Robert Haas <robertmhaas@gmail.com> — 2011-01-03T15:21:44Z

    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

  44. Re: and it's not a bunny rabbit, either

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-01-03T15:31:07Z

    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