Re: [HACKERS] Getting rid of pre-assignment of index names in CREATE TABLE LIKE
On Mon, Jul 16, 2012 at 1:10 PM, Tom Lane wrote: > Robert Haas 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
Robert Haas 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
On Mon, Jul 16, 2012 at 12:43 PM, Tom Lane wrote: > Robert Haas writes: >> On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane 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
Robert Haas writes: > On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane 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
On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane 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
On Sun, Jul 15, 2012 at 11:49 AM, Tom Lane wrote: > Gurjeet Singh writes: > > On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane 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
Re: [HACKERS] Getting rid of pre-assignment of index names in CREATE TABLE LIKE
Gurjeet Singh writes: > On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane 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
On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane 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
[HACKERS] 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 diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y index 18f0add852e7832739e3877811e385abcb540fab..ec634f1660e0be883b451abbb380d6dc30e69b93 100644 *** a/src/backend/bootstrap/bootparse.y --- b/src/backend/bootstrap/bootparse.y *** Boot_InsertStmt: *** 279,296 Boot_DeclareIndexStmt: XDECLARE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN { do_start(); ! DefineIndex(makeRangeVar(NULL, $6, -1), ! $3, $4, ! InvalidOid, ! $8, ! NULL, ! $10, ! NULL, NIL, NIL, ! false, false, false, false, false, ! false, false, true, false, false); do_end(); } ; --- 279,312 Boot_DeclareIndexStmt: XDECLARE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN { + IndexStmt *stmt = makeNode(IndexStmt); + do_start(); ! stmt->idxname = $3; ! stmt->relation = makeRangeVar(NULL, $6, -1); ! stmt->accessMethod = $8; ! stmt->tableSpace = NULL; ! stmt->indexParams = $10; ! stmt->options = NIL; ! stmt->whereClause = NULL; ! stmt->excludeOpNames = NIL; ! stmt->idxcomment = NULL; ! stmt->indexOid = InvalidOid; ! stmt->oldNode = InvalidOid; ! stmt->unique = false; ! stmt->primary = false; ! stmt->isconstraint = false; ! stmt->deferrable = false; ! stmt->initdeferred = false; ! stmt->concurrent = false; ! ! DefineIndex(stmt, $4, ! false, ! false, ! true, /* skip_build */ ! false); do_end(); } ; *** Boot_DeclareIndexStmt: *** 298,315 Boot_DeclareUniqueIndexStmt: XDECLARE UNIQUE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN { do_start(); ! DefineIndex(makeRangeVar(NULL, $7, -1), ! $4, $5, ! InvalidOid, ! $9, ! NULL, ! $11, ! NULL, NIL, NIL, ! true, false, false, false, false, ! false, false, true, false, false); do_end(); } ; --- 314,347 Boot_DeclareUniqueIndexStmt: XDECLARE UNIQUE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN { + IndexStmt *stmt = makeNode(IndexStmt); + do_start(); ! stmt->idxname = $4; ! stmt->relation = makeRangeVar(NULL, $7, -1); ! stmt->accessMethod = $9; ! stmt->tableSpace = NULL; ! stmt->indexParams = $11; ! stmt->options = NIL; ! stmt->whereClause = NULL; ! stmt->excludeOpNames = NIL; ! stmt->idxcomment = NULL; ! stmt->indexOid = InvalidOid; ! stmt->oldNode = InvalidOid; ! stmt->unique = true; ! stmt->primary = false; ! stmt->isconstraint = false; ! stmt->deferrable = false; ! stmt->initdeferred = false; ! stmt->concurrent = false; ! ! DefineIndex(stmt, $5, ! false, ! false, ! true, /* skip_build */ ! false); do_end(); } ; diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index e5d1d8b699ad6d37cd3dd9e19b1d76ba97c01eaa..f315ab60154d6c6aa3c96f05babb8e79945f70b8 100644 *** a/src/backend/commands/indexcmds.c --- b/src/backend/commands/indexcmds.c *** *** 24,29 -