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

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.

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.

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?

regards, tom lane

On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>
> 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.
>
> 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'd like us to stick to the standard practice of not changing features/API
in beta releases.

Best regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> writes:
> On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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'd like us to stick to the standard practice of not changing features/API
> in beta releases.

This is a bug fix, not a feature addition, and sometimes you can't fix
bugs without touching APIs that might be used by third party code.
So the question here is whether this bug fix is sufficiently important,
and on the other side how likely it is that anyone has already built
extensions for 9.2 that depend on IndexStmt or DefineIndex. I don't
think trying to treat it as a "policy" matter is helpful -- it's a
tradeoff.

If you happen to know of EDB-private code that would be broken by
this change, telling us so (and why a mid-beta change would be
problematic) would be helpful.

regards, tom lane

On Sun, Jul 15, 2012 at 11:49 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> writes:
> > On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> 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'd like us to stick to the standard practice of not changing
> features/API
> > in beta releases.
>
> This is a bug fix, not a feature addition, and sometimes you can't fix
> bugs without touching APIs that might be used by third party code.
> So the question here is whether this bug fix is sufficiently important,
> and on the other side how likely it is that anyone has already built
> extensions for 9.2 that depend on IndexStmt or DefineIndex. I don't
> think trying to treat it as a "policy" matter is helpful -- it's a
> tradeoff.
>

I was hoping that we could fix the bug in released code without having to
change the structure or the API, but if that's not feasible, I will
withdraw my objection.

> If you happen to know of EDB-private code that would be broken by
> this change, telling us so (and why a mid-beta change would be
> problematic) would be helpful.
>

I checked, and I don't see any EDB code that would be affected by this
change.

Best regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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.

Hmm, well, if that's the argument for doing this then we really need to
throw away the entire implementation of CREATE TABLE LIKE, because it's
doing that all over the place; I'm only proposing to remove one specific
instance.

regards, tom lane

On Mon, Jul 16, 2012 at 12:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 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.
>
> Hmm, well, if that's the argument for doing this then we really need to
> throw away the entire implementation of CREATE TABLE LIKE, because it's
> doing that all over the place; I'm only proposing to remove one specific
> instance.

The problem isn't confined to CREATE TABLE LIKE; it's a widespread
design flaw that will likely take years of work to clean up
completely. I don't think that's a reason not to commit your change
though; it fixes a bug and is an incremental improvement, even if a
small one. That having been said, if you're feeling an urge to tackle
the problem more broadly, don't let me stand in your way...

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> The problem isn't confined to CREATE TABLE LIKE; it's a widespread
> design flaw that will likely take years of work to clean up
> completely. I don't think that's a reason not to commit your change
> though; it fixes a bug and is an incremental improvement, even if a
> small one. That having been said, if you're feeling an urge to tackle
> the problem more broadly, don't let me stand in your way...

Not me; I'm just trying to close out a bug report.

FWIW, I think your argument only barely supports this change at all,
because CREATE TABLE LIKE is still generating an IndexStmt which after
all is the representation of a CREATE INDEX command. We've overloaded
it to do a few other things, and now it will be able to do one more
thing, but this isn't moving things at all towards separating high-
and low-level operations.

regards, tom lane

On Mon, Jul 16, 2012 at 1:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> The problem isn't confined to CREATE TABLE LIKE; it's a widespread
>> design flaw that will likely take years of work to clean up
>> completely. I don't think that's a reason not to commit your change
>> though; it fixes a bug and is an incremental improvement, even if a
>> small one. That having been said, if you're feeling an urge to tackle
>> the problem more broadly, don't let me stand in your way...
>
> Not me; I'm just trying to close out a bug report.
>
> FWIW, I think your argument only barely supports this change at all,
> because CREATE TABLE LIKE is still generating an IndexStmt which after
> all is the representation of a CREATE INDEX command. We've overloaded
> it to do a few other things, and now it will be able to do one more
> thing, but this isn't moving things at all towards separating high-
> and low-level operations.

:-(

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