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 <plural-form-of-object-type>"?

+1 for this. It's clear, informative, and relevant to the error at
hand.

In some sense that's
less informative, because it doesn't tell you which object types DO
support the requested operation, but it's not clear that you care
about that. If you are trying to drop a column from a view, the fact
that it would be possible to drop a column from a table is cold
comfort. The advantage of this method is that you need only N
translatable strings (one per relkind), rather than 2^N (one per
subset of the universe of relkinds).

A slightly more granular version of this would be to make the caller
pass in a string indicating what the requested operation actually is,
so that you can say something like "<plural-form-of-object-type> do
not support <requested-operation>" or "<requested-operation> is not
supported by <plural-form-of-object-type>" (e.g. "views do not support
SET NOT NULL"). But that breaks our guideline about assembling
translatable strings from small pieces. Maybe it'd be OK if the piece
is just a fragment of SQL syntax, though - not sure.

Or we can just stick with the way we've been doing it, if I'm the only
one who thinks it's icky.

You're not the only one.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Mon, Dec 27, 2010 at 12:13, Robert Haas <robertmhaas@gmail.com> wrote:

Could we get away with something as simple as "requested operation is
not supported for <plural-form-of-object-type>"?

+1. If so, will we have a function to get object names something like
GetPluralFormOfObjectType(Relation rel or char relkind) => char * ?

But that breaks our guideline about assembling
translatable strings from small pieces.  Maybe it'd be OK if the piece
is just a fragment of SQL syntax, though - not sure.

Agreed. <requested-operation> should be a SQL syntax,
so we won't have to translate the part.

--
Itagaki Takahiro

On Sun, Dec 26, 2010 at 10:44 PM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:

On Mon, Dec 27, 2010 at 12:13, Robert Haas <robertmhaas@gmail.com> wrote:

Could we get away with something as simple as "requested operation is
not supported for <plural-form-of-object-type>"?

+1. If so, will we have a function to get object names something like
GetPluralFormOfObjectType(Relation rel or char relkind) => char *  ?

In the interest of keeping things simple for translators, I was
thinking we'd just write out a string for each object type:

"requested operation is not supported for tables"
"requested operation is not supported for views"
"requested operation is not supported for indexes"

or if we go with the some-assembly required version, perhaps:

"tables do not support %s"
"views do not support %s"
"indexes do not support %s"

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Dec 26, 2010, at 7:55 PM, Robert Haas wrote:

"tables do not support %s"
"views do not support %s"
"indexes do not support %s"

The more detail we can give, the better, of course. Nothing's more frustrating than having a command with an error like, "Object does not support requested operation." Thanks, computer program: "Swerved off road, hit tree" is about as useful.

--
-- Christophe Pettus
xof@thebuild.com

Robert Haas <robertmhaas@gmail.com> writes:

or if we go with the some-assembly required version, perhaps:

"tables do not support %s"
"views do not support %s"
"indexes do not support %s"

+1 for that way. Although personally I'd reverse the phrasing:

/* translator: %s is the name of a SQL command */
%s does not support tables

regards, tom lane

On Mon, Dec 27, 2010 at 10:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

or if we go with the some-assembly required version, perhaps:

"tables do not support %s"
"views do not support %s"
"indexes do not support %s"

+1 for that way.  Although personally I'd reverse the phrasing:

       /* translator: %s is the name of a SQL command */
       %s does not support tables

To me, it seems as though it's the object which doesn't support the
operation, rather than the other way around. Reversing it makes most
sense to me in cases where it's an implementation restriction, such as
"DROP COLUMN does not support views". But at least to me, the
phrasing "SET WITHOUT CLUSTER does not support views" suggests that
SET WITHOUT CLUSTER is somehow defective, which is not really the
message I think we want to convey there. But maybe we need some other
votes.

I took a crack at implementing this and the results were mixed - see
attached patch. It works pretty well for the most part, but there are
a couple of warts. For ALTER TABLE commands like DROP COLUMN and SET
WITHOUT CLUSTER the results look pretty good, and I find the new error
messages a definite improvement over the old ones. It's not quite so
good for setting reloptions or attoptions. You get something like:

ERROR: views do not support SET (...)
ERROR: views do not support ALTER COLUMN SET (...)

...which might actually be OK, if not fantastically wonderful. One
might think of mitigating this problem by writing "ALTER TABLE SET
(...)" rather than just "SET (...)", but that's not easily done
because there are several equivalent forms (for example, a view can be
modified with either ALTER VIEW or ALTER TABLE, for historical - or
perhaps hysterical - reasons). An even bigger problem is this:

rhaas=# alter view v add primary key (a);
ERROR: views do not support ADD INDEX

The problem is that alter table actions AT_AddIndex and
AT_AddConstraint don't tie neatly back to a particular piece of
syntax. The message as written isn't incomprehensible (especially if
you're reading it in English) but it definitely leaves something to be
desired.

Ideas?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 29.12.2010 06:54, Robert Haas wrote:

With the patch:

rhaas=# cluster v;
ERROR: views do not support CLUSTER

"do not support" sounds like a missing feature, rather than a
nonsensical command. How about something like "CLUSTER cannot be used on
views"

The patch changes a bunch of heap_openrv() calls to relation_openrv().
Perhaps it would be better make the error message something like "\"%s\"
is not a table", and keep the callers unchanged. It's not particularly
useful to repeat the command in the error message, the user should know
what command he issued. Even if it's buried deep in a PL/pgSQL function
or something, it should be clear from the context lines.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

On 29.12.2010 06:54, Robert Haas wrote:

 With the patch:

rhaas=# cluster v;
ERROR:  views do not support CLUSTER

"do not support" sounds like a missing feature, rather than a nonsensical
command. How about something like "CLUSTER cannot be used on views"

I'm fine with flipping the ordering around. I think I like it
marginally better this way, but you and Tom both seem to prefer the
opposite ordering, ergo so be it (barring a sudden influx of contrary
votes).

The patch changes a bunch of heap_openrv() calls to relation_openrv().
Perhaps it would be better make the error message something like "\"%s\" is
not a table", and keep the callers unchanged. It's not particularly useful
to repeat the command in the error message, the user should know what
command he issued. Even if it's buried deep in a PL/pgSQL function or
something, it should be clear from the context lines.

Did you read the whole thread?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 29.12.2010 13:17, Robert Haas wrote:

Did you read the whole thread?

Ah, sorry:

I've had to change some of the heap_open(rv) calls to
relation_open(rv) to avoid having the former throw the wrong error
message before the latter kicks in. I think there might be stylistic
objections to that, but I'm not sure what else to propose. I'm
actually pretty suspicious that many of the heap_open(rv) calls I
*didn't* change are either already a little iffy or likely to become
so once the SQL/MED stuff for foreign tables goes in. They make it
easy to forget that we've got a whole pile of relkinds and you
actually need to really think about which ones you can handle.

Hmm, I believe the idea of heap_open is to check that the relation is
backed by a heap that you can read with heap_beginscan+heap_next. At the
moment that includes normal tables, sequences and toast tables. Foreign
tables would not fall into that category.

Yeah, you're right that most of the callers of heap_open actually want
to a tighter check than that.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Hmm, I believe the idea of heap_open is to check that the relation is
backed by a heap that you can read with heap_beginscan+heap_next. At the
moment that includes normal tables, sequences and toast tables. Foreign
tables would not fall into that category.

I don't believe that that definition is documented anyplace; if we
decide that's what we want it to mean, some code comments would be in
order.

Yeah, you're right that most of the callers of heap_open actually want
to a tighter check than that.

I think probably most of the physical calls of heap_open are actually
associated with system catalog accesses, and the fact that the code says
heap_open not relation_open has got more to do with copy&paste than any
real thought about what we're specifying.

regards, tom lane

On Dec 29, 2010, at 12:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Hmm, I believe the idea of heap_open is to check that the relation 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

Robert Haas <robertmhaas@gmail.com> writes:

The existing comments mention that callers must check that the return
value is not a view, if they care. So if there is currently a single
coherent definition for what heap_open is supposed to do, it's clearly
NOT the one Heikki proposes. My guess is that reality is closer to
your theory of "what got cut-and-pasted".

Well, reality is that in the beginning there was heap_open and
index_open, and nothing else. And there weren't views, so basically
those two functions covered all the interesting types of relations.
We got to the current state of affairs by a series of whatever were the
least invasive code changes at the time; nobody's ever taken a step
back and tried to define what "heap_open" ought to allow from the
standpoint of first principles.

In practice I think it would make sense if heap_open accepts all
relation types on which you can potentially do either a heapscan or
indexscan (offhand those should be the same set of relkinds, I think;
so this is the same in effect as Heikki's proposal, but phrased
differently). So it would have to start rejecting views, and we'd need
to go looking for the consequences of that.

regards, tom lane

Excerpts from Tom Lane's message of mié dic 29 16:29:45 -0300 2010:

In practice I think it would make sense if heap_open accepts all
relation types on which you can potentially do either a heapscan or
indexscan (offhand those should be the same set of relkinds, I think;
so this is the same in effect as Heikki's proposal, but phrased
differently). So it would have to start rejecting views, and we'd need
to go looking for the consequences of that.

This seems a very good idea, but I think we shouldn't let it sink the
current patch.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Tom Lane's message of mié dic 29 16:29:45 -0300 2010:

In practice I think it would make sense if heap_open accepts all
relation types on which you can potentially do either a heapscan or
indexscan (offhand those should be the same set of relkinds, I think;
so this is the same in effect as Heikki's proposal, but phrased
differently). So it would have to start rejecting views, and we'd need
to go looking for the consequences of that.

This seems a very good idea, but I think we shouldn't let it sink the
current patch.

No, but possibly regularizing what heap_open is defined to do would make
Robert's patch simpler.

regards, tom lane

On Wed, Dec 29, 2010 at 3:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Tom Lane's message of mié dic 29 16:29:45 -0300 2010:

In practice I think it would make sense if heap_open accepts all
relation types on which you can potentially do either a heapscan or
indexscan (offhand those should be the same set of relkinds, I think;
so this is the same in effect as Heikki's proposal, but phrased
differently).  So it would have to start rejecting views, and we'd need
to go looking for the consequences of that.

This seems a very good idea, but I think we shouldn't let it sink the
current patch.

No, but possibly regularizing what heap_open is defined to do would make
Robert's patch simpler.

I think that any meaning we assign to heap_open is going to be 95%
arbitrary and of little practical help. There are at least six
different sets of object classes which to which a particular commands
or alter table subcommands can be legally applied: (1) tables only,
(2) views only, (3) tables and views, (4) tables and indexes, (5)
tables and composite types, (6) tables, views, and composite types.
Adding foreign tables promises to add several more combinations
immediately and likely more down the line; if we add materialized
views, that'll add a bunch more. There's not really any single
definition that's going to be a silver bullet. I think the best thing
to do might be to get RID of heap_open(rv) and always use
relation_openrv plus an appropriate relkind test.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

On 29.12.2010 06:54, Robert Haas wrote:

 With the patch:

rhaas=# cluster v;
ERROR:  views do not support CLUSTER

"do not support" sounds like a missing feature, rather than a nonsensical
command. How about something like "CLUSTER cannot be used on views"

In the latest version of this patch, I created four translatable
strings per object type:

<blats> do not support %s (where %s is an SQL command)
<blats> do not support constraints
<blats> do not support rules
<blats> do not support triggers

It's reasonable enough to write "CLUSTER cannot be used on views", but
does "constraints cannot be used on views" seems more awkward to me.
Or do we think that's OK?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Dec 29, 2010 at 04:53:47PM -0500, Robert Haas wrote:

On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

On 29.12.2010 06:54, Robert Haas wrote:

�With the patch:

rhaas=# cluster v;
ERROR: �views do not support CLUSTER

"do not support" sounds like a missing feature, rather than a nonsensical
command. How about something like "CLUSTER cannot be used on views"

In the latest version of this patch, I created four translatable
strings per object type:

<blats> do not support %s (where %s is an SQL command)
<blats> do not support constraints
<blats> do not support rules
<blats> do not support triggers

It's reasonable enough to write "CLUSTER cannot be used on views", but
does "constraints cannot be used on views" seems more awkward to me.
Or do we think that's OK?

That particular one looks good insofar as it describes reality. With
predicate locks, etc., it may become untrue later, though :)

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Dec 29, 2010 at 5:14 PM, David Fetter <david@fetter.org> wrote:

On Wed, Dec 29, 2010 at 04:53:47PM -0500, Robert Haas wrote:

On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

On 29.12.2010 06:54, Robert Haas wrote:

 With the patch:

rhaas=# cluster v;
ERROR:  views do not support CLUSTER

"do not support" sounds like a missing feature, rather than a nonsensical
command. How about something like "CLUSTER cannot be used on views"

In the latest version of this patch, I created four translatable
strings per object type:

<blats> do not support %s (where %s is an SQL command)
<blats> do not support constraints
<blats> do not support rules
<blats> do not support triggers

It's reasonable enough to write "CLUSTER cannot be used on views", but
does "constraints cannot be used on views" seems more awkward to me.
Or do we think that's OK?

That particular one looks good insofar as it describes reality.  With
predicate locks, etc., it may become untrue later, though :)

After further thought, I think it makes sense to change this around a
bit and create a family of functions that can be invoked like this:

void check_relation_for_FEATURE_support(Relation rel);

...where FEATURE is constraint, trigger, rule, index, etc. The
function will be defined to throw an error if the relation isn't of a
type that can support the named feature. The error message will be of
the form:

constraints can only be used on tables
triggers can be used only on tables and views
etc.

This avoids the need to define a separate error message for each
unsupported relkind, and I think it's just as informative as, e.g.,
"constraints cannot be used on <whatever object type you tried to
invoke it on>". We can adopt the same language for commands, e.g.:
"CLUSTER can only be used on tables".

Comments?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company