Re: Getting rid of pre-assignment of index names in CREATE TABLE LIKE

On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> In bug #6734 we have a complaint about a longstanding misfeature of
> CREATE TABLE LIKE. Ordinarily, this command doesn't select names for
> copied indexes, but leaves that to be done at runtime by DefineIndex.
> But if it's copying comments, and an index of the source table has a
> comment, it's forced to preassign a name to the new index so that it can
> build a CommentStmt that can apply the comment after the index is made.
> Apart from being something of a modularity violation, this isn't very safe
> because of the danger of name collision with earlier indexes for the new
> table. And that's exactly what's happening in bug #6734.
>
> I suggested that we could dodge the problem by allowing IndexStmt to
> carry a comment to be attached to the new index, and thereby avoid
> needing an explicit COMMENT command. Attached is a patch that fixes it
> that way.

I agree with this approach. I think it's pretty much always a bad
idea for DDL command A to fake up a parse node of the type used by DDL
command B. It tends to make the code ugly and unmaintainable and
propagates nasty abstraction violations all over the place. We should
really aim to break every DDL command into a high-level part that does
permissions checks, sanity checks, locking, etc. and a low-level part
that actually performs the requested operation. In the case of
comments, we happen to have it broken up pretty much correctly, with
CreateComments and CreateSharedComments as the workhorse routines and
CommentObject as the high-level routine; there are other places where
things are not so happy.

> While I was at it, it seemed like DefineIndex's parameter list had grown
> well past any sane bound, so I refactored it to pass the IndexStmt
> struct as-is rather than passing all the fields individually.

I agree with this as well.

> With or without that choice, though, this approach means a change in
> DefineIndex's API, as well as the contents of struct IndexStmt. That
> means it's probably unsafe to back-patch, since it seems plausible that
> there might be third-party code out there that creates indexes and would
> use these interfaces.
>
> I would like to sneak this fix into 9.2, though. Does anyone think
> it's already too late to be touching these APIs for 9.2?

I do not.

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