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

2012-07-16 Thread Robert Haas
On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane t...@sss.pgh.pa.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2012-07-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane t...@sss.pgh.pa.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2012-07-16 Thread Robert Haas
On Mon, Jul 16, 2012 at 12:43 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane t...@sss.pgh.pa.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2012-07-16 Thread Tom Lane
Robert Haas robertmh...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2012-07-16 Thread Robert Haas
On Mon, Jul 16, 2012 at 1:10 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2012-07-15 Thread Gurjeet Singh
On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane t...@sss.pgh.pa.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


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

2012-07-15 Thread Tom Lane
Gurjeet Singh singh.gurj...@gmail.com writes:
 On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane t...@sss.pgh.pa.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2012-07-15 Thread Gurjeet Singh
On Sun, Jul 15, 2012 at 11:49 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 Gurjeet Singh singh.gurj...@gmail.com writes:
  On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane t...@sss.pgh.pa.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