LIKE INCLUDING COMMENTS code is a flight of fancy

Started by Tom Laneabout 16 years ago5 messages
#1Tom Lane
tgl@sss.pgh.pa.us

I just got done fixing a different problem in that area, and then I
noticed this:

regression=# create table src (f1 text);
CREATE TABLE
regression=# create index srclower on src(lower(f1));
CREATE INDEX
regression=# comment on column srclower.pg_expression_1 is 'a comment';
COMMENT
regression=# create index srcupper on src(upper(f1));
CREATE INDEX
regression=# comment on column srcupper.pg_expression_1 is 'another comment';
COMMENT
regression=# create table dest (like src including all);
ERROR: relation "dest_key" already exists

The reason this fails is that the LIKE INCLUDING COMMENTS code thinks it
can use ChooseRelationName() in advance of actually creating the
indexes (cf. chooseIndexName in parse_utilcmd.c). This does not work.
That function is only meant to select a name for an index that will be
created immediately --- otherwise, its logic for avoiding collisions
does not work at all. As illustrated above.

I don't immediately see a fix for this that isn't a great deal more
trouble than the feature is worth. I suggest that we might want to just
rip out the support for copying comments on indexes. Or maybe even the
whole copy-comments aspect of it.

regards, tom lane

#2Takahiro Itagaki
itagaki.takahiro@oss.ntt.co.jp
In reply to: Tom Lane (#1)
Re: LIKE INCLUDING COMMENTS code is a flight of fancy

Tom Lane <tgl@sss.pgh.pa.us> wrote:

I suggest that we might want to just
rip out the support for copying comments on indexes. Or maybe even the
whole copy-comments aspect of it.

We have two related ToDo items below. They are a bit inconsintent,
but they mean we should forbid COMMENT on columns of an index,
or must have full-support of the feature.

Which direction should we go? As for me, forbidding comments on index
columns seems to be a saner way because index can have arbitrary key
names in some cases.

- Forbid COMMENT on columns of an index
Postgres currently allows comments to be placed on the columns of
an index, but pg_dump doesn't handle them and the column names
themselves are implementation-dependent.
http://archives.postgresql.org/message-id/27676.1237906577@sss.pgh.pa.us

- pg_dump / pg_restore: Add dumping of comments on index columns and
composite type columns
http://archives.postgresql.org/pgsql-hackers/2009-03/msg00931.php
(XXX: Comments on composite type columns can work now?)

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Takahiro Itagaki (#2)
Re: LIKE INCLUDING COMMENTS code is a flight of fancy

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

I suggest that we might want to just
rip out the support for copying comments on indexes.

We have two related ToDo items below. They are a bit inconsintent,
but they mean we should forbid COMMENT on columns of an index,
or must have full-support of the feature.

Which direction should we go? As for me, forbidding comments on index
columns seems to be a saner way because index can have arbitrary key
names in some cases.

- Forbid COMMENT on columns of an index
Postgres currently allows comments to be placed on the columns of
an index, but pg_dump doesn't handle them and the column names
themselves are implementation-dependent.
http://archives.postgresql.org/message-id/27676.1237906577@sss.pgh.pa.us

- pg_dump / pg_restore: Add dumping of comments on index columns and
composite type columns
http://archives.postgresql.org/pgsql-hackers/2009-03/msg00931.php
(XXX: Comments on composite type columns can work now?)

I'm for forbidding comments on index columns. The amount of work
required to support the feature fully seems far out of proportion to
its value.

In any case, if pg_dump drops such comments (which I had forgotten,
but it seems true after a quick look at the code), then we could
certainly get away with having LIKE not copy them. That would fix
the immediate problem.

regards, tom lane

#4Andrew Dunstan
andrew@dunslane.net
In reply to: Tom Lane (#3)
Re: LIKE INCLUDING COMMENTS code is a flight of fancy

Tom Lane wrote:

I'm for forbidding comments on index columns. The amount of work
required to support the feature fully seems far out of proportion to
its value.

In any case, if pg_dump drops such comments (which I had forgotten,
but it seems true after a quick look at the code), then we could
certainly get away with having LIKE not copy them. That would fix
the immediate problem.

If we're going to keep the comments we should dump them. I don't mind
dropping them altogether - it's hardly a killer feature. We should just
be consistent, IMNSHO.

cheers

andrew

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andrew Dunstan (#4)
Re: LIKE INCLUDING COMMENTS code is a flight of fancy

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

I'm for forbidding comments on index columns. The amount of work
required to support the feature fully seems far out of proportion to
its value.

In any case, if pg_dump drops such comments (which I had forgotten,
but it seems true after a quick look at the code), then we could
certainly get away with having LIKE not copy them. That would fix
the immediate problem.

If we're going to keep the comments we should dump them. I don't mind
dropping them altogether - it's hardly a killer feature. We should just
be consistent, IMNSHO.

Well, let's just forbid them then. It'll take just a few extra lines in
comment.c to throw an error if the target table has the wrong relkind.
Then we can pull out the troublesome code in parse_utilcmds.c.

It strikes me also that this changes the terms of discussion for the
other patch I was working on. I was mistakenly assuming that we could
not change the naming convention for individual index columns because
it would cause errors during dump/reload of per-column comments. But
since pg_dump never has supported that, there is no such risk. I
propose re-instating my previous idea of replacing "pg_expression_n"
with the name chosen by FigureColname. This not only makes the index
column names a bit more useful, but it fixes the disadvantage I was
on about for CREATE TABLE LIKE: it can get the FigureColname name
from the index's pg_attribute entry, so there's no problem with
producing smart index-expression column names when cloning indexes.

regards, tom lane