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

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