Re: [HACKERS] Patch to add a primary key using an existing index
On Wed, Jan 26, 2011 at 6:14 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Gurjeet Singh singh.gurj...@gmail.com writes:
Attached is the updated patch with doc changes and test cases.
Applied with assorted corrections. Aside from the refactoring I wanted,
there were various oversights.
Looking at the commit, the committed patch resembles the submitted patch by
only about 20% :) .
I agree there were quite serious oversights. Thanks for taking care of
those.
Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
Re: [HACKERS] Patch to add a primary key using an existing index
On Mon, Jan 24, 2011 at 07:01:13PM -0500, Tom Lane wrote: One other issue that might be worthy of discussion is that as things stand, execution of the ADD CONSTRAINT USING INDEX syntax will cause the constraint to absorb the index as an INTERNAL dependency. That means dropping the constraint would make the index go away silently --- it no longer has any separate life. If the intent is just to provide a way to get the effect of ALTER ADD PRIMARY KEY CONCURRENTLY, then this behavior is probably fine. But someone who believes DROP CONSTRAINT exactly reverses the effects of ADD CONSTRAINT might be surprised. Comments? So you'd manually create an index, attach it to a constraint, drop the constraint, and find that the index had disappeared? ISTM since you created the index explicitly, you should have to drop it explicitly as well. -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com signature.asc Description: Digital signature
Re: [HACKERS] Patch to add a primary key using an existing index
Sorry for not being on top of this.
On Tue, Jan 25, 2011 at 9:01 AM, Tom Lane t...@sss.pgh.pa.us wrote:
I wrote:
... If that's the only issue then I don't see any need to wait on
the author, so will take this one.
I find myself quite dissatisfied with the way that this patch adds yet
another bool flag to index_create (which has too many of those already),
with the effect of causing it to exactly *not* do an index creation.
That's a clear violation of the principle of least astonishment IMNSHO.
I think what's needed here is to refactor things a bit so that the
constraint-creation code is pulled out of index_create and called
separately where needed. Hacking on that now.
Thanks.
One other issue that might be worthy of discussion is that as things
stand, execution of the ADD CONSTRAINT USING INDEX syntax will cause
the constraint to absorb the index as an INTERNAL dependency. That
means dropping the constraint would make the index go away silently ---
it no longer has any separate life. If the intent is just to provide a
way to get the effect of ALTER ADD PRIMARY KEY CONCURRENTLY, then this
behavior is probably fine. But someone who believes DROP CONSTRAINT
exactly reverses the effects of ADD CONSTRAINT might be surprised.
Comments?
Since we rename the index automatically to match the constraint name,
implying that the index now belongs to the system, I think the user should
expect the index to go away with the constraint; else we have to remember
index's original name and restore that name on DROP CONSTRAINT, which IMHO
will be even more unintuitive.
Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
Re: [HACKERS] Patch to add a primary key using an existing index
Gurjeet Singh singh.gurj...@gmail.com writes: On Tue, Jan 25, 2011 at 9:01 AM, Tom Lane t...@sss.pgh.pa.us wrote: One other issue that might be worthy of discussion is that as things stand, execution of the ADD CONSTRAINT USING INDEX syntax will cause the constraint to absorb the index as an INTERNAL dependency. That means dropping the constraint would make the index go away silently --- it no longer has any separate life. Since we rename the index automatically to match the constraint name, implying that the index now belongs to the system, I think the user should expect the index to go away with the constraint; else we have to remember index's original name and restore that name on DROP CONSTRAINT, which IMHO will be even more unintuitive. Yeah, that's a good point. Also, the documented example usage of this feature is To recreate a primary key constraint, without blocking updates while the index is rebuilt: programlisting CREATE UNIQUE INDEX CONCURRENTLY dist_id_temp_idx on distributors (dist_id); ALTER TABLE distributors DROP CONSTRAINT distributors_pkey, ADD CONSTRAINT distributors_pkey PRIMARY KEY USING INDEX dist_id_temp_idx; /programlisting with the implication that after you do that, the installed index is exactly like you would have gotten from straight ADD PRIMARY KEY. If there's something funny about it, then it's not just a replacement. In the end I think this is mainly an issue of setting appropriate expectations in the documentation. I've added the following text to the ALTER TABLE manual page: para After this command is executed, the index is quoteowned/ by the constraint, in the same way as if the index had been built by a regular literalADD PRIMARY KEY/ or literalADD UNIQUE/ command. In particular, dropping the constraint will make the index disappear too. /para 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] Patch to add a primary key using an existing index
On Wed, Jan 26, 2011 at 5:31 AM, Tom Lane t...@sss.pgh.pa.us wrote:
In the end I think this is mainly an issue of setting appropriate
expectations in the documentation. I've added the following text to
the ALTER TABLE manual page:
para
After this command is executed, the index is quoteowned/ by the
constraint, in the same way as if the index had been built by
a regular literalADD PRIMARY KEY/ or literalADD UNIQUE/
command. In particular, dropping the constraint will make the index
disappear too.
/para
I'd change that last sentence to:
... dropping the constraint will drop the index too.
'disappear' doesn't seem accurate in the context.
Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
Re: [HACKERS] Patch to add a primary key using an existing index
Gurjeet Singh singh.gurj...@gmail.com writes: Attached is the updated patch with doc changes and test cases. Applied with assorted corrections. Aside from the refactoring I wanted, there were various oversights. I have consciously disallowed the ability to specify storage_parameters using the WITH clause, if somebody thinks it is wise to allow that and is needed, I can do that. AFAICS, WITH would be supplied at the time of index creation; it's not appropriate to include it here, any more than INDEX TABLESPACE. A point that may or may not have gotten discussed back when is that it's important that the result of this process be dumpable by pg_dump, ie there not be any hidden discrepancies between the state after ADD CONSTRAINT USING INDEX and the state you'd get from straight ADD CONSTRAINT, because the latter is the syntax pg_dump is going to emit. ADD CONSTRAINT can handle WITH and INDEX TABLESPACE, so carrying those over from the original index specification is no problem, but non-default index opclasses or sort ordering options would be a big problem. That would in particular completely break pg_upgrade, because the on-disk index wouldn't match the catalog entries created by running pg_dump. I added some code to check and disallow non-default opclass and options. 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] Patch to add a primary key using an existing index
Steve Singer ssinger...@sympatico.ca writes: src/backend/parser/parse_utilcmd.c: 1452 Your calling strdup on the attribute name. I don't have a good enough grasp on the code to be able to trace this through to where the memory gets free'd. Does it get freed? Should/could this be a call to pstrdup strdup() is pretty much automatically wrong in the parser, not to mention most of the rest of the backend. pstrdup is likely what was meant. If that's the only issue then I don't see any need to wait on the author, so will take this one. 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] Patch to add a primary key using an existing index
I wrote: ... If that's the only issue then I don't see any need to wait on the author, so will take this one. I find myself quite dissatisfied with the way that this patch adds yet another bool flag to index_create (which has too many of those already), with the effect of causing it to exactly *not* do an index creation. That's a clear violation of the principle of least astonishment IMNSHO. I think what's needed here is to refactor things a bit so that the constraint-creation code is pulled out of index_create and called separately where needed. Hacking on that now. One other issue that might be worthy of discussion is that as things stand, execution of the ADD CONSTRAINT USING INDEX syntax will cause the constraint to absorb the index as an INTERNAL dependency. That means dropping the constraint would make the index go away silently --- it no longer has any separate life. If the intent is just to provide a way to get the effect of ALTER ADD PRIMARY KEY CONCURRENTLY, then this behavior is probably fine. But someone who believes DROP CONSTRAINT exactly reverses the effects of ADD CONSTRAINT might be surprised. Comments? 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] Patch to add a primary key using an existing index
On Mon, Jan 24, 2011 at 7:01 PM, Tom Lane t...@sss.pgh.pa.us wrote: One other issue that might be worthy of discussion is that as things stand, execution of the ADD CONSTRAINT USING INDEX syntax will cause the constraint to absorb the index as an INTERNAL dependency. That means dropping the constraint would make the index go away silently --- it no longer has any separate life. If the intent is just to provide a way to get the effect of ALTER ADD PRIMARY KEY CONCURRENTLY, then this behavior is probably fine. But someone who believes DROP CONSTRAINT exactly reverses the effects of ADD CONSTRAINT might be surprised. Comments? Well, I think the behavior as described is what we want. If the syntax associated with that behavior is going to lead to confusion, I'd view that as a deficiency of the syntax, rather than a deficiency of the behavior. (I make this comment with some reluctance considering the amount of bikeshedding we've already done on this topic, but... that's what I think.) -- 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] Patch to add a primary key using an existing index
I've taken a look at this version of the patch. Submission Review This version of the patch applies cleanly to master. It matches your git repo and includes test + docs. Usability Review --- The command syntax now matches what was discussed during the last cf. The text of the notice: test=# alter table a add constraint acons unique using index aind2; NOTICE: ALTER TABLE / ADD UNIQUE USING INDEX will rename index aind2 to acons Documentation -- I've attached a patch (to be applied on top of your latest patch) with some editorial changes I'd recommend to your documentation. I feel it reads a bit clearer (but others should chime in if they disagree or have better wordings) A git tree with changes rebased to master + this change is available at https://github.com/ssinger/postgres/tree/ssinger/constraint_with_index Code Review --- src/backend/parser/parse_utilcmd.c: 1452 Your calling strdup on the attribute name. I don't have a good enough grasp on the code to be able to trace this through to where the memory gets free'd. Does it get freed? Should/could this be a call to pstrdup Feature Test - I wasn't able to find any issues in my testing I'm marking this as returned with feedback pending your answer on the possible memory leak above but I think the patch is very close to being ready. Steve Singer diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 83d2fbb..0b486ab 100644 *** a/doc/src/sgml/ref/alter_table.sgml --- b/doc/src/sgml/ref/alter_table.sgml *** ALTER TABLE replaceable class=PARAMETE *** 242,268 termliteralADD replaceable class=PARAMETERtable_constraint_using_index/replaceable/literal/term listitem para ! This form adds a new literalPRIMARY KEY/ or literalUNIQUE/ constraint to the table using an existing index. All the columns of the index will be included in the constraint. /para para ! The index should be UNIQUE, and should not be a firsttermpartial index/ ! or an firsttermexpressional index/. /para para ! This can be helpful in situations where one wishes to recreate or ! literalREINDEX/ the index of a literalPRIMARY KEY/ or a ! literalUNIQUE/ constraint, but dropping and recreating the constraint ! to acheive the effect is not desirable. See the illustrative example below. /para note para If a constraint name is provided then the index will be renamed to that ! name, else the constraint will be named to match the index name. /para /note --- 242,270 termliteralADD replaceable class=PARAMETERtable_constraint_using_index/replaceable/literal/term listitem para ! This form adds a new literalPRIMARY KEY/ or literalUNIQUE/literal constraint to the table using an existing index. All the columns of the index will be included in the constraint. /para para ! The index should be a UNIQUE index. A firsttermpartial index/firstterm ! or an firsttermexpressional index/firstterm is not allowed. /para para ! Adding a constraint using an existing index can be helpful in situations ! where you wishes to rebuild an index used for a ! literalPRIMARY KEY/literal or a literalUNIQUE/literal constraint, ! but dropping and recreating the constraint ! is not desirable. See the illustrative example below. /para note para If a constraint name is provided then the index will be renamed to that ! name of the constraint. Otherwise the constraint will be named to match ! the index name. /para /note -- 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] Patch to add a primary key using an existing index
On Sun, Jan 16, 2011 at 5:34 PM, Steve Singer ssinger...@sympatico.ca wrote: I'm marking this as returned with feedback pending your answer on the possible memory leak above but I think the patch is very close to being ready. Please use Waiting on Author if the patch is to be considered further for this CommitFest, and Returned with Feedback only if it will not be further considered for this CommitFest. -- 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] Patch to add a primary key using an existing index
On Thu, Dec 9, 2010 at 2:48 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Gurjeet Singh singh.gurj...@gmail.com writes:
But I still hold a bias towards renaming the index to match constraint
name
(with a NOTICE), rather than require that the constraint name match the
index name, because the constraint name is optional and when it is not
provided system has to generate a name and we have to rename the index
anyway to maintain consistency.
No. If the constraint name is not specified, we should certainly use
the existing index name, not randomly rename it.
Attached is the updated patch with doc changes and test cases. An overview
of the patch is in order:
The new command syntax is
ALTER TABLE table_name
ADD [CONSTRAINT constraint_name]
PRIMARY KEY USING INDEX index_name
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE
];
ALTER TABLE table_name
ADD [CONSTRAINT constraint_name]
UNIQUE USING INDEX index_name
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE
];
The index should be a unique index, and it should not be an expressional or
partial index. The included test cases exercise a few other cases.
If the constraint name is provided, then index is renamed to that with a
NOTICE, else the index name is used as the constraint name.
I have consciously disallowed the ability to specify storage_parameters
using the WITH clause, if somebody thinks it is wise to allow that and is
needed, I can do that.
Git branch: https://github.com/gurjeet/postgres/tree/constraint_with_index
Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
constraint_using_index.patch.gz
Description: GNU Zip compressed data
--
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] Patch to add a primary key using an existing index
On Sun, Dec 5, 2010 at 2:09 PM, Peter Eisentraut pete...@gmx.net wrote:
On fre, 2010-12-03 at 15:27 -0500, Robert Haas wrote:
On Fri, Dec 3, 2010 at 2:56 PM, r t pg...@xzilla.net wrote:
What exactly was the objection to the following --
ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name;
Is the objection that you might have been trying to specify a
constraint
named using ? I'm willing to make that option more difficult. :-)
I think it's that someone might expect the word after USING to be the
name of an index AM.
That could be avoided by writing
USING INDEX name
Allowing USING INDEX along with USING INDEX TABLESPACE is causing
shift/reduce conflicts.
I liked the proposal upthread of providing alternate syntax where user does
not have to specify column-list and system picks up that list from the
index.
ALTER TABLE table_name ADD [CONSTRAINT cons_name] PRIMARY KEY (column_list)
[WITH (...)] [USING INDEX TABLESPACE tblspcname];
ALTER TABLE table_name ADD [CONSTRAINT cons_name] PRIMARY KEY [WITH (...)]
[USING INDEX index_name];
This would also help avoid the bug that Itagaki found, where the user wants
to use an existing index, and also specifies USING INDEX TABLESPACE.
But I still hold a bias towards renaming the index to match constraint name
(with a NOTICE), rather than require that the constraint name match the
index name, because the constraint name is optional and when it is not
provided system has to generate a name and we have to rename the index
anyway to maintain consistency.
Following are the gram.y changes that I am going to start with:
%type boolean constraints_set_mode
-%type strOptTableSpace OptConsTableSpace OptTableSpaceOwner
+%type strOptTableSpace OptConsTableSpace OptConsIndex
OptTableSpaceOwner
%type list opt_check_option
[...]
| UNIQUE '(' columnList ')' opt_definition OptConsTableSpace
ConstraintAttributeSpec
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_UNIQUE;
n-location = @1;
n-keys = $3;
n-options = $5;
n-indexspace = $6;
n-deferrable = ($7 1) != 0;
n-initdeferred = ($7 2) != 0;
$$ = (Node *)n;
}
+ | UNIQUE opt_definition OptConsIndex ConstraintAttributeSpec
+ {
+ Constraint *n = makeNode(Constraint);
+ n-contype = CONSTR_UNIQUE;
+ n-location = @1;
+ n-options = $2;
+ n-indexname = $3;
+ n-deferrable = ($4 1) != 0;
+ n-initdeferred = ($4 2) != 0;
+ $$ = (Node *)n;
+ }
| PRIMARY KEY '(' columnList ')' opt_definition
OptConsTableSpace
ConstraintAttributeSpec
{
Constraint *n = makeNode(Constraint);
n-contype = CONSTR_PRIMARY;
n-location = @1;
n-keys = $4;
n-options = $6;
n-indexspace = $7;
n-deferrable = ($8 1) != 0;
n-initdeferred = ($8 2) != 0;
$$ = (Node *)n;
}
+ | PRIMARY KEY opt_definition OptConsIndex
ConstraintAttributeSpec
+ {
+ Constraint *n = makeNode(Constraint);
+ n-contype = CONSTR_PRIMARY;
+ n-location = @1;
+ n-options = $3;
+ n-indexname = $4;
+ n-deferrable = ($5 1) != 0;
+ n-initdeferred = ($5 2) != 0;
+ $$ = (Node *)n;
+ }
| EXCLUDE access_method_clause '(' ExclusionConstraintList ')'
[...]
OptConsTableSpace: USING INDEX TABLESPACE name { $$ = $4; }
| /*EMPTY*/ { $$ = NULL; }
;
+OptConsIndex: USING INDEX name { $$ = $3; }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
+
Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
Re: [HACKERS] Patch to add a primary key using an existing index
Gurjeet Singh singh.gurj...@gmail.com writes: But I still hold a bias towards renaming the index to match constraint name (with a NOTICE), rather than require that the constraint name match the index name, because the constraint name is optional and when it is not provided system has to generate a name and we have to rename the index anyway to maintain consistency. No. If the constraint name is not specified, we should certainly use the existing index name, not randomly rename it. 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] Patch to add a primary key using an existing index
Tom Lane t...@sss.pgh.pa.us wrote: If the constraint name is not specified, we should certainly use the existing index name, not randomly rename it. +1 -Kevin -- 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] Patch to add a primary key using an existing index
On Thu, Dec 9, 2010 at 2:51 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Tom Lane t...@sss.pgh.pa.us wrote: If the constraint name is not specified, we should certainly use the existing index name, not randomly rename it. +1 +1 -- 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] Patch to add a primary key using an existing index
On fre, 2010-12-03 at 15:27 -0500, Robert Haas wrote: On Fri, Dec 3, 2010 at 2:56 PM, r t pg...@xzilla.net wrote: What exactly was the objection to the following -- ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name; Is the objection that you might have been trying to specify a constraint named using ? I'm willing to make that option more difficult. :-) I think it's that someone might expect the word after USING to be the name of an index AM. That could be avoided by writing USING INDEX name -- 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] Patch to add a primary key using an existing index
On Dec 4, 2010, at 1:30 AM, Tom Lane t...@sss.pgh.pa.us wrote: Ross J. Reedstrom reeds...@rice.edu writes: If you consider that an index basically is, in some sense, a pre-canned column list, then: ALTER TABLE table_name ADD PRIMARY KEY (column_list); ALTER TABLE table_name ADD PRIMARY KEY USING index_name; are parallel constructions. And it avoids the error case of the user providing a column list that doesn't match the index. +1 for that approach. One other thought is that ordinarily, the add-constraint syntax ensures that the constraint is named the same as its underlying index; in fact we go so far as to keep them in sync if you rename the index later. But after ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING index_name; they'd be named differently, unless we (a) throw an error or (b) forcibly rename the index. Neither of those ideas seems to satisfy the principle of least surprise, but leaving it alone seems like it will also lead to confusion later. I think that might be the best way though. I wonder whether, in the same spirit as not letting the user write a column name list that might not match, we should pick a syntax that doesn't allow specifying a constraint name different from the index name. In the case where you say CONSTRAINT it'd be a bit plausible to write something like ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING EXISTING INDEX; (implying that the index to use is named con_name) but I don't know what to do if you want to leave off the CONSTRAINT name clause. Because this seems plain weird. ...Robert -- 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] Patch to add a primary key using an existing index
On Sat, Dec 4, 2010 at 6:48 AM, Robert Haas robertmh...@gmail.com wrote: On Dec 4, 2010, at 1:30 AM, Tom Lane t...@sss.pgh.pa.us wrote: Ross J. Reedstrom reeds...@rice.edu writes: If you consider that an index basically is, in some sense, a pre-canned column list, then: ALTER TABLE table_name ADD PRIMARY KEY (column_list); ALTER TABLE table_name ADD PRIMARY KEY USING index_name; are parallel constructions. And it avoids the error case of the user providing a column list that doesn't match the index. +1 for that approach. One other thought is that ordinarily, the add-constraint syntax ensures that the constraint is named the same as its underlying index; in fact we go so far as to keep them in sync if you rename the index later. But after ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING index_name; they'd be named differently, unless we (a) throw an error or (b) forcibly rename the index. Neither of those ideas seems to satisfy the principle of least surprise, but leaving it alone seems like it will also lead to confusion later. I think that might be the best way though. Haas, are you promoting to leave them different? I'd be comfortable with that. I'd also be comfortable with B (renaming with notice, similar to the notice when creating a constraint). Given we rename the constraint when we rename the index, I would not find the reverse behavior terribly surprising. Actually I think I'd even be comfortable with A, either you must name the constraint after the index, or you can leave the constraint name out, and we'll use the index name. I wonder whether, in the same spirit as not letting the user write a column name list that might not match, we should pick a syntax that doesn't allow specifying a constraint name different from the index name. In the case where you say CONSTRAINT it'd be a bit plausible to write something like ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING EXISTING INDEX; (implying that the index to use is named con_name) but I don't know what to do if you want to leave off the CONSTRAINT name clause. Because this seems plain weird. +1 Robert Treat play: http://www.xzilla.net work: http://www.omniti.com/is/hiring
Re: [HACKERS] Patch to add a primary key using an existing index
Robert Treat r...@xzilla.net writes: Actually I think I'd even be comfortable with A, either you must name the constraint after the index, or you can leave the constraint name out, and we'll use the index name. Or we could omit the CONSTRAINT name clause from the syntax altogether. I think that allowing the names to be different is a bad idea. That hasn't been possible in the past and there's no apparent reason why this feature should suddenly make it possible. We will have problems with it, for instance failures on name collisions because generated names are only checked against one catalog or the other. 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] Patch to add a primary key using an existing index
On Dec 4, 2010, at 11:46 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Treat r...@xzilla.net writes: Actually I think I'd even be comfortable with A, either you must name the constraint after the index, or you can leave the constraint name out, and we'll use the index name. Or we could omit the CONSTRAINT name clause from the syntax altogether. I think that allowing the names to be different is a bad idea. That hasn't been possible in the past and there's no apparent reason why this feature should suddenly make it possible. We will have problems with it, for instance failures on name collisions because generated names are only checked against one catalog or the other. So maybe we should start by deciding what the semantics should be, and then decide what syntax would convey those semantics. What would make sense to me is: create a pk constraint with the sane name as the existing unique index. If that constraint name already exists, error. ...Robert -- 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] Patch to add a primary key using an existing index
On 12/04/2010 12:37 PM, Robert Haas wrote: What would make sense to me is: create a pk constraint with the sane name as the existing unique index. If that constraint name already exists, error. +1, agreed. Based on this, the syntax should be obvious. We'll need to doc what to do in the event of a name collision error, though (rename the other constraint). Hmmm, can you rename a constraint? -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com -- 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] Patch to add a primary key using an existing index
On sön, 2010-11-28 at 20:40 -0500, Robert Haas wrote: On Sun, Nov 28, 2010 at 8:06 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: On Fri, Nov 26, 2010 at 05:58, Steve Singer ssin...@ca.afilias.info wrote: The attached version of the patch gets your regression tests to pass. I'm going to mark this as ready for a committer. I think we need more discussions about the syntax: ALTER TABLE table_name ADD PRIMARY KEY (...) WITH (INDEX='index_name') Why not: ALTER TABLE table_name ADD PRIMARY KEY (...) INDEX index_name; I would think that that determines that name of the index that the command creates. It does not convey that an existing index is to be used. -- 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] Patch to add a primary key using an existing index
On Fri, Dec 3, 2010 at 2:23 PM, Peter Eisentraut pete...@gmx.net wrote: On sön, 2010-11-28 at 20:40 -0500, Robert Haas wrote: On Sun, Nov 28, 2010 at 8:06 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: On Fri, Nov 26, 2010 at 05:58, Steve Singer ssin...@ca.afilias.info wrote: The attached version of the patch gets your regression tests to pass. I'm going to mark this as ready for a committer. I think we need more discussions about the syntax: ALTER TABLE table_name ADD PRIMARY KEY (...) WITH (INDEX='index_name') Why not: ALTER TABLE table_name ADD PRIMARY KEY (...) INDEX index_name; I would think that that determines that name of the index that the command creates. It does not convey that an existing index is to be used. Well, that'll become clear pretty quickly if you try to use it that way, but I'm certainly open to other ideas. Random thoughts: ALTER TABLE table_name SET PRIMARY KEY INDEX index_name ALTER INDEX index_name PRIMARY KEY Other suggestions? -- 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] Patch to add a primary key using an existing index
On 03.12.2010 21:43, Robert Haas wrote: On Fri, Dec 3, 2010 at 2:23 PM, Peter Eisentrautpete...@gmx.net wrote: On sön, 2010-11-28 at 20:40 -0500, Robert Haas wrote: On Sun, Nov 28, 2010 at 8:06 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: On Fri, Nov 26, 2010 at 05:58, Steve Singerssin...@ca.afilias.info wrote: The attached version of the patch gets your regression tests to pass. I'm going to mark this as ready for a committer. I think we need more discussions about the syntax: ALTER TABLE table_name ADD PRIMARY KEY (...) WITH (INDEX='index_name') Why not: ALTER TABLE table_name ADD PRIMARY KEY (...) INDEX index_name; I would think that that determines that name of the index that the command creates. It does not convey that an existing index is to be used. Well, that'll become clear pretty quickly if you try to use it that way, but I'm certainly open to other ideas. Random thoughts: ALTER TABLE table_name SET PRIMARY KEY INDEX index_name ALTER INDEX index_name PRIMARY KEY ALTER TABLE table_name SET PRIMARY KEY USING INDEX index_name. Quite verbose, but imho USING makes it much more clear that it's an existing index. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- 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] Patch to add a primary key using an existing index
Excerpts from Heikki Linnakangas's message of vie dic 03 16:45:59 -0300 2010: ALTER TABLE table_name SET PRIMARY KEY USING INDEX index_name. Quite verbose, but imho USING makes it much more clear that it's an existing index. I was going to post the same thing (well except I was still thinking in ADD PRIMARY KEY rather than SET PRIMARY KEY). I think SET is better than ADD in that it is a bit different from the syntax that makes it create a new index. On the other hand, it could also be pointlessly annoying. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Patch to add a primary key using an existing index
On Fri, Dec 3, 2010 at 2:43 PM, Robert Haas robertmh...@gmail.com wrote: On Fri, Dec 3, 2010 at 2:23 PM, Peter Eisentraut pete...@gmx.net wrote: On sön, 2010-11-28 at 20:40 -0500, Robert Haas wrote: On Sun, Nov 28, 2010 at 8:06 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: On Fri, Nov 26, 2010 at 05:58, Steve Singer ssin...@ca.afilias.info wrote: The attached version of the patch gets your regression tests to pass. I'm going to mark this as ready for a committer. I think we need more discussions about the syntax: ALTER TABLE table_name ADD PRIMARY KEY (...) WITH (INDEX='index_name') Why not: ALTER TABLE table_name ADD PRIMARY KEY (...) INDEX index_name; I would think that that determines that name of the index that the command creates. It does not convey that an existing index is to be used. +1 on this being confusing Well, that'll become clear pretty quickly if you try to use it that way, but I'm certainly open to other ideas. Random thoughts: ALTER TABLE table_name SET PRIMARY KEY INDEX index_name ALTER INDEX index_name PRIMARY KEY Other suggestions? What exactly was the objection to the following -- ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name; Is the objection that you might have been trying to specify a constraint named using ? I'm willing to make that option more difficult. :-) Robert Treat play: http://www.xzilla.net work: http://www.omniti.com/is/hiring
Re: [HACKERS] Patch to add a primary key using an existing index
On 03.12.2010 21:58, Alvaro Herrera wrote: Excerpts from Heikki Linnakangas's message of vie dic 03 16:45:59 -0300 2010: ALTER TABLE table_name SET PRIMARY KEY USING INDEX index_name. Quite verbose, but imho USING makes it much more clear that it's an existing index. I was going to post the same thing (well except I was still thinking in ADD PRIMARY KEY rather than SET PRIMARY KEY). I think SET is better than ADD in that it is a bit different from the syntax that makes it create a new index. On the other hand, it could also be pointlessly annoying. I think I'd prefer ADD too. I didn't pay attention to that when I posted. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- 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] Patch to add a primary key using an existing index
On Fri, Dec 3, 2010 at 2:56 PM, r t pg...@xzilla.net wrote: What exactly was the objection to the following -- ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name; Is the objection that you might have been trying to specify a constraint named using ? I'm willing to make that option more difficult. :-) I think it's that someone might expect the word after USING to be the name of an index AM. -- 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] Patch to add a primary key using an existing index
On 12/3/10 12:27 PM, Robert Haas wrote: On Fri, Dec 3, 2010 at 2:56 PM, r t pg...@xzilla.net wrote: What exactly was the objection to the following -- ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name; Is the objection that you might have been trying to specify a constraint named using ? I'm willing to make that option more difficult. :-) I think it's that someone might expect the word after USING to be the name of an index AM. Seems unlikely to cause confusion to me. However, I don't see why we need (column_list). Surely the index has a column list already? ALTER TABLE table_name ADD CONSTRAINT pk_name PRIMARY KEY USING index_name ... seems like the syntax most consistent with the existing commands. Anything else would be confusingly inconsistent with the way you add a brand-new PK. -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com -- 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] Patch to add a primary key using an existing index
On Fri, Dec 3, 2010 at 4:41 PM, Josh Berkus j...@agliodbs.com wrote: On 12/3/10 12:27 PM, Robert Haas wrote: On Fri, Dec 3, 2010 at 2:56 PM, r t pg...@xzilla.net wrote: What exactly was the objection to the following -- ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name; Is the objection that you might have been trying to specify a constraint named using ? I'm willing to make that option more difficult. :-) I think it's that someone might expect the word after USING to be the name of an index AM. Seems unlikely to cause confusion to me. +1. And were we ever to support that, I think that would be the case to use WITH (storage_parameter) type syntax, where you would specify access_method=hash (or whatever). Although, let's not debate that syntax right now, at this point :-) However, I don't see why we need (column_list). Surely the index has a column list already? ALTER TABLE table_name ADD CONSTRAINT pk_name PRIMARY KEY USING index_name ... seems like the syntax most consistent with the existing commands. Anything else would be confusingly inconsistent with the way you add a brand-new PK. Uh, the syntax I posted was based on this currently valid syntax: ALTER TABLE table_name ADD PRIMARY KEY (column_list); The constraint bit is optional, which is why I left it out, but I presume it would be optional with the new syntax as well... Also, I'm not wedded to the idea of keeping the column list, but if you are arguing to make it super consistent, then I think you need to include it. Robert Treat play: http://www.xzilla.net work: http://www.omniti.com/is/hiring
Re: [HACKERS] Patch to add a primary key using an existing index
On Fri, Dec 03, 2010 at 05:16:04PM -0500, Robert Treat wrote: On Fri, Dec 3, 2010 at 4:41 PM, Josh Berkus j...@agliodbs.com wrote: However, I don't see why we need (column_list). Surely the index has a column list already? ALTER TABLE table_name ADD CONSTRAINT pk_name PRIMARY KEY USING index_name ... seems like the syntax most consistent with the existing commands. Anything else would be confusingly inconsistent with the way you add a brand-new PK. Uh, the syntax I posted was based on this currently valid syntax: ALTER TABLE table_name ADD PRIMARY KEY (column_list); The constraint bit is optional, which is why I left it out, but I presume it would be optional with the new syntax as well... Also, I'm not wedded to the idea of keeping the column list, but if you are arguing to make it super consistent, then I think you need to include it. If you consider that an index basically is, in some sense, a pre-canned column list, then: ALTER TABLE table_name ADD PRIMARY KEY (column_list); ALTER TABLE table_name ADD PRIMARY KEY USING index_name; are parallel constructions. And it avoids the error case of the user providing a column list that doesn't match the index. Ross -- Ross Reedstrom, Ph.D. reeds...@rice.edu Systems Engineer Admin, Research Scientistphone: 713-348-6166 Connexions http://cnx.orgfax: 713-348-3665 Rice University MS-375, Houston, TX 77005 GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE -- 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] Patch to add a primary key using an existing index
On 12/3/10 2:16 PM, Robert Treat wrote: Uh, the syntax I posted was based on this currently valid syntax: ALTER TABLE table_name ADD PRIMARY KEY (column_list); The constraint bit is optional, which is why I left it out, but I presume it would be optional with the new syntax as well... Also, I'm not wedded to the idea of keeping the column list, but if you are arguing to make it super consistent, then I think you need to include it. No, I'm not in that case. I'm suggesting we omit the column list and skip directly to USING. Why no column list? 1. The extra typing will annoy our users 2. The column list provides opportunities for users to fail to be consistent with the index and get errors -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com -- 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] Patch to add a primary key using an existing index
Ross J. Reedstrom reeds...@rice.edu writes: If you consider that an index basically is, in some sense, a pre-canned column list, then: ALTER TABLE table_name ADD PRIMARY KEY (column_list); ALTER TABLE table_name ADD PRIMARY KEY USING index_name; are parallel constructions. And it avoids the error case of the user providing a column list that doesn't match the index. +1 for that approach. One other thought is that ordinarily, the add-constraint syntax ensures that the constraint is named the same as its underlying index; in fact we go so far as to keep them in sync if you rename the index later. But after ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING index_name; they'd be named differently, unless we (a) throw an error or (b) forcibly rename the index. Neither of those ideas seems to satisfy the principle of least surprise, but leaving it alone seems like it will also lead to confusion later. I wonder whether, in the same spirit as not letting the user write a column name list that might not match, we should pick a syntax that doesn't allow specifying a constraint name different from the index name. In the case where you say CONSTRAINT it'd be a bit plausible to write something like ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING EXISTING INDEX; (implying that the index to use is named con_name) but I don't know what to do if you want to leave off the CONSTRAINT name clause. Thoughts? 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] Patch to add a primary key using an existing index
On Fri, Nov 26, 2010 at 05:58, Steve Singer ssin...@ca.afilias.info wrote: The attached version of the patch gets your regression tests to pass. I'm going to mark this as ready for a committer. I think we need more discussions about the syntax: ALTER TABLE table_name ADD PRIMARY KEY (...) WITH (INDEX='index_name') Issues: * WITH (...) is designed for storage parameters. I think treating INDEX as a special keyword in the way might be confusable. * 'index_name' needs to be single-quoted, but object identifiers should be double-quoted literals in normal cases. * The key specifier is a duplicated option because the index has own keys. Do we need it? It might be for safety, but redundant. Note that the patch raises a reasonable error on conflict: ERROR: PRIMARY KEY/UNIQUE constraint definition does not match the index And, I found a bug: * USING INDEX TABLESPACE clause is silently ignored, even if the index uses another tablespace. After all, do we need a special syntax for the functionality? Reusing WITH (...) syntax seems to be a trouble for me. ADD PRIMARY KEY USING index_name might be a candidate, but we'd better reserve USING for non-btree PRIMARY KEY/UNIQUE indexes. Ideas and suggestions? -- Itagaki Takahiro -- 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] Patch to add a primary key using an existing index
On Sun, Nov 28, 2010 at 8:06 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: On Fri, Nov 26, 2010 at 05:58, Steve Singer ssin...@ca.afilias.info wrote: The attached version of the patch gets your regression tests to pass. I'm going to mark this as ready for a committer. I think we need more discussions about the syntax: ALTER TABLE table_name ADD PRIMARY KEY (...) WITH (INDEX='index_name') Why not: ALTER TABLE table_name ADD PRIMARY KEY (...) INDEX index_name; -- 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] Patch to add a primary key using an existing index
On Sun, Nov 28, 2010 at 08:40:08PM -0500, Robert Haas wrote: On Sun, Nov 28, 2010 at 8:06 PM, Itagaki Takahiro itagaki.takah...@gmail.com wrote: On Fri, Nov 26, 2010 at 05:58, Steve Singer ssin...@ca.afilias.info wrote: The attached version of the patch gets your regression tests to pass. I'm going to mark this as ready for a committer. I think we need more discussions about the syntax: ALTER TABLE table_name ADD PRIMARY KEY (...) WITH (INDEX='index_name') Why not: ALTER TABLE table_name ADD PRIMARY KEY (...) INDEX index_name; +1 :) Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Patch to add a primary key using an existing index
On 10-11-22 03:24 PM, Steve Singer wrote: On 10-11-22 09:37 AM, Gurjeet Singh wrote: On Sat, Nov 20, 2010 at 9:00 AM, Steve Singer ssinger...@sympatico.ca Almost fixed. I still get an unexpected difference. ! DETAIL: cannot create PRIMARY KEY/UNIQUE constraint with a non-unique index. CREATE UNIQUE INDEX rpi_idx2 ON rpi_test(a , b); -- should fail; WITH INDEX option specified more than once. ALTER TABLE rpi_test ADD PRIMARY KEY (a, b) --- 35,41 -- should fail; non-unique ALTER TABLE rpi_test ADD primary key(a, b) WITH (INDEX = 'rpi_idx1'); ERROR: rpi_idx1 is not a unique index ! DETAIL: Cannot create PRIMARY KEY/UNIQUE constraint using a non-unique index. The attached version of the patch gets your regression tests to pass. I'm going to mark this as ready for a committer. replace_pkey_index.revised2.patch.gz Description: GNU Zip compressed data -- 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] Patch to add a primary key using an existing index
On Sat, Nov 20, 2010 at 9:00 AM, Steve Singer ssinger...@sympatico.cawrote:
Submission Review:
Tests
The expected output for the regression tests you added don't match
what I'm getting when I run the tests with your patch applied.
I think you just need to regenerate the expected results they seem
to be from a previous version of the patch (different error messages
etc..).
Fixed. Also modified one test to cover the case where constraint name is
provided.
Documentation
---
I was able to generate the docs.
The ALTER TABLE page under the synopsis has
ADD table_constraint
where table_constraint is defined on the CREATE TABLE page.
On the CREATE TABLE page table_constraint isn't defined as having the WITH
, the WITH is part of index_parameters.
I propose the alter table page instead have
ADD table_constraint [index_parameters]
where index_parameters also references the CREATE TABLE page like
table_constraint.
IMHO index_parameters is an optional component of table_constraint, and
hence can't be mentioned here, at least not the way shown above.
I have made slight improvements to the doc which might help the user
understand that this WITH(INDEX=) option is exclusive to ALTER TABLE and not
provided by CREATE TABLE.
Usability Review
Behaviour
-
I feel that if the ALTER TABLE ... renames the the index
a NOTICE should be generated. We generate notices about creating an index
for a new pkey. We should give them a notice that we are renaming an index
on them.
Done.
Coding Review:
==
Error Messages
-
in tablecmds your errdetail messages often don't start with a capital
letter. I belive the preference is to have the errdetail strings start with
a capital letter and end with a period.
Fixed.
tablecmds.c - get_constraint_index_oid
contains the check
/* Currently only B-tree indexes are suupported for primary keys */
if (index_rel-rd_rel-relam != BTREE_AM_OID)
elog(ERROR, \%s\ is not a B-Tree index,
index_name);
but above we already validate that the index is a unique index with another
check. Today only B-tree indexes support unique constraints. If this
changed at some point and we could have a unique index of some other type,
would something in this patch need to be changed to support them? If we are
only depending on the uniqueness property then I think this check is covered
by the uniquness one higher in the function.
Also note the typo in your comment above (suupported)
I agree; code removed.
Comments
-
index.c: Line 671 and 694. Your indentation changes make the comments
run over 80 characters. If you end up submitting a new version
of the patch I'd reformat those two comments.
Fixed.
Other than those issues the patch looks good to me.
Thanks for your time Steve.
Regards,
PS: I will be mostly unavailable between 11/25 and 12/6, so wouldn't mind if
somebody took ownership of this patch for that duration.
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
replace_pkey_index.revised.patch.gz
Description: GNU Zip compressed data
--
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] Patch to add a primary key using an existing index
On 10-11-22 09:37 AM, Gurjeet Singh wrote:
On Sat, Nov 20, 2010 at 9:00 AM, Steve Singer ssinger...@sympatico.ca
mailto:ssinger...@sympatico.ca wrote:
Submission Review:
Tests
The expected output for the regression tests you added don't match
what I'm getting when I run the tests with your patch applied.
I think you just need to regenerate the expected results they seem
to be from a previous version of the patch (different error
messages etc..).
Fixed. Also modified one test to cover the case where constraint name
is provided.
Almost fixed.
I still get an unexpected difference.
! DETAIL: cannot create PRIMARY KEY/UNIQUE constraint with a non-unique
index.
CREATE UNIQUE INDEX rpi_idx2 ON rpi_test(a , b);
-- should fail; WITH INDEX option specified more than once.
ALTER TABLE rpi_test ADD PRIMARY KEY (a, b)
--- 35,41
-- should fail; non-unique
ALTER TABLE rpi_test ADD primary key(a, b) WITH (INDEX = 'rpi_idx1');
ERROR: rpi_idx1 is not a unique index
! DETAIL: Cannot create PRIMARY KEY/UNIQUE constraint using a
non-unique index.
Documentation
---
I was able to generate the docs.
The ALTER TABLE page under the synopsis has
ADD table_constraint
where table_constraint is defined on the CREATE TABLE page.
On the CREATE TABLE page table_constraint isn't defined as having
the WITH
, the WITH is part of index_parameters.
I propose the alter table page instead have
ADD table_constraint [index_parameters]
where index_parameters also references the CREATE TABLE page like
table_constraint.
IMHO index_parameters is an optional component of table_constraint,
and hence can't be mentioned here, at least not the way shown above.
My reading of CREATE TABLE is that index_parameters is an optional
parameter that comes after table_constraint and isn't part of
table_constraint. Any other opinions?
Everything else I mentioned seems fixed in this version
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
Re: [HACKERS] Patch to add a primary key using an existing index
On 10-11-07 01:54 PM, Gurjeet Singh wrote: Attached is the patch that extends the same feature for UNIQUE indexes. It also includes some doc changes for the ALTER TABLE command, but I could not verify the resulting changes since I don't have the doc-building infrastructure installed. Regards, Gurjeet, I've taken a stab at reviewing this. Submission Review: Tests The expected output for the regression tests you added don't match what I'm getting when I run the tests with your patch applied. I think you just need to regenerate the expected results they seem to be from a previous version of the patch (different error messages etc..). Documentation --- I was able to generate the docs. The ALTER TABLE page under the synopsis has ADD table_constraint where table_constraint is defined on the CREATE TABLE page. On the CREATE TABLE page table_constraint isn't defined as having the WITH , the WITH is part of index_parameters. I propose the alter table page instead have ADD table_constraint [index_parameters] where index_parameters also references the CREATE TABLE page like table_constraint. Usability Review Behaviour - I feel that if the ALTER TABLE ... renames the the index a NOTICE should be generated. We generate notices about creating an index for a new pkey. We should give them a notice that we are renaming an index on them. Coding Review: == Error Messages - in tablecmds your errdetail messages often don't start with a capital letter. I belive the preference is to have the errdetail strings start with a capital letter and end with a period. tablecmds.c - get_constraint_index_oid contains the check /* Currently only B-tree indexes are suupported for primary keys */ if (index_rel-rd_rel-relam != BTREE_AM_OID) elog(ERROR, \%s\ is not a B-Tree index, index_name); but above we already validate that the index is a unique index with another check. Today only B-tree indexes support unique constraints. If this changed at some point and we could have a unique index of some other type, would something in this patch need to be changed to support them? If we are only depending on the uniqueness property then I think this check is covered by the uniquness one higher in the function. Also note the typo in your comment above (suupported) Comments - index.c: Line 671 and 694. Your indentation changes make the comments run over 80 characters. If you end up submitting a new version of the patch I'd reformat those two comments. Other than those issues the patch looks good to me. Steve -- 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] Patch to add a primary key using an existing index
Depesz brought that to my attention a few days after the initial submission,
and adding support for UNIQUE was not much pain. I implemented it almost
immediately, but didn't announce it as I was hoping I could submit some doc
changes too with that.
If you are the adventurous kind, you can follow the Git branch here:
https://github.com/gurjeet/postgres/tree/replace_pkey_index
Regards,
On Mon, Nov 1, 2010 at 10:29 PM, Jim Nasby j...@nasby.net wrote:
UNIQUE constraints suffer from the same behavior; feel like fixing that
too? :)
On Oct 9, 2010, at 1:07 PM, Gurjeet Singh wrote:
This is a continuation from this thread:
http://archives.postgresql.org/pgsql-hackers/2010-09/msg02153.php
The attached patch allows creating a primary key using an existing index.
This capability would be helpful in situations where one wishes to
rebuild/reindex the primary key, but associated downtime is not desirable.
It also allows one to create a table and start using it, while creating a
unique index 'concurrently' and later adding the primary key using the
concurrently built index. Maybe pg_dump can also use it.
The command syntax is:
ALTER TABLE sometable ADD PRIMARY KEY( col1, col2 ) WITH ( INDEX =
'indexname' );
A typical use case:
CREATE INDEX CONCURRENTLY new_pkey_idx ON sometable( a, b );
ALTER TABLE sometable ADD PRIMARY KEY ( a, b ) WITH (INDEX =
'new_pkey_idx' );
- OR -
ALTER TABLE sometable DROP CONSTRAINT sometable_pkey,
ADD PRIMARY KEY ( a, b ) WITH (INDEX = 'new_pkey_idx' );
Notes for the reviewers:
Don't be scared by the size of changes to index.c :) These are mostly
indentation diffs. I have attached two versions of the patch: one is context
diff, and the other is the same except ignoring whitespace changes.
The pseudocode is as follows:
In ATExecAddIndex()
If this ALTER command specifies a PRIMARY KEY
Call get_pkey_index_oid() to perform checks.
In get_pkey_index_oid()
Look for the WITH INDEX option
Reject
if more than one WITH INDEX clause specified
if the index doesn't exist or not found in table's schema
if the index is associated with any CONSTRAINT
if index is not ready or not valid (CONCURRENT buiild? Canceled
CONCURRENT?)
if index is on some other table
if index is not unique
if index is an expression index
if index is a partial index
if index columns do not match the PRIMARY KEY clause in the
command
if index is not B-tree
If PRIMARY KEY clause doesn't have a constraint name, assign it one.
(code comments explain why)
Rename the index to match constraint name in the PRIMARY KEY clause
Back in ATExecAddIndex()
Use the index OID returned by get_pkey_index_oid() to tell
DefineIndex() to not create index.
Now mark the index as having 'indisprimary' flag.
In DefineIndex() and index_create() APIs
pass an additional flag: index_exists
Skip various actions based on this flag.
The patch contains a few tests, and doesn't yet have a docs patch.
The development branch is at
http://github.com/gurjeet/postgres/tree/replace_pkey_index
Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
add_pkey_with_index.patchadd_pkey_with_index.ignore_ws.patch
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Jim C. Nasby, Database Architect j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
Re: [HACKERS] Patch to add a primary key using an existing index
Attached is the patch that extends the same feature for UNIQUE indexes.
It also includes some doc changes for the ALTER TABLE command, but I could
not verify the resulting changes since I don't have the doc-building
infrastructure installed.
Regards,
On Mon, Nov 8, 2010 at 1:39 AM, Gurjeet Singh singh.gurj...@gmail.comwrote:
Depesz brought that to my attention a few days after the initial
submission, and adding support for UNIQUE was not much pain. I implemented
it almost immediately, but didn't announce it as I was hoping I could submit
some doc changes too with that.
If you are the adventurous kind, you can follow the Git branch here:
https://github.com/gurjeet/postgres/tree/replace_pkey_index
Regards,
On Mon, Nov 1, 2010 at 10:29 PM, Jim Nasby j...@nasby.net wrote:
UNIQUE constraints suffer from the same behavior; feel like fixing that
too? :)
On Oct 9, 2010, at 1:07 PM, Gurjeet Singh wrote:
This is a continuation from this thread:
http://archives.postgresql.org/pgsql-hackers/2010-09/msg02153.php
The attached patch allows creating a primary key using an existing
index.
This capability would be helpful in situations where one wishes to
rebuild/reindex the primary key, but associated downtime is not desirable.
It also allows one to create a table and start using it, while creating a
unique index 'concurrently' and later adding the primary key using the
concurrently built index. Maybe pg_dump can also use it.
The command syntax is:
ALTER TABLE sometable ADD PRIMARY KEY( col1, col2 ) WITH ( INDEX =
'indexname' );
A typical use case:
CREATE INDEX CONCURRENTLY new_pkey_idx ON sometable( a, b );
ALTER TABLE sometable ADD PRIMARY KEY ( a, b ) WITH (INDEX =
'new_pkey_idx' );
- OR -
ALTER TABLE sometable DROP CONSTRAINT sometable_pkey,
ADD PRIMARY KEY ( a, b ) WITH (INDEX = 'new_pkey_idx' );
Notes for the reviewers:
Don't be scared by the size of changes to index.c :) These are mostly
indentation diffs. I have attached two versions of the patch: one is context
diff, and the other is the same except ignoring whitespace changes.
The pseudocode is as follows:
In ATExecAddIndex()
If this ALTER command specifies a PRIMARY KEY
Call get_pkey_index_oid() to perform checks.
In get_pkey_index_oid()
Look for the WITH INDEX option
Reject
if more than one WITH INDEX clause specified
if the index doesn't exist or not found in table's schema
if the index is associated with any CONSTRAINT
if index is not ready or not valid (CONCURRENT buiild? Canceled
CONCURRENT?)
if index is on some other table
if index is not unique
if index is an expression index
if index is a partial index
if index columns do not match the PRIMARY KEY clause in the
command
if index is not B-tree
If PRIMARY KEY clause doesn't have a constraint name, assign it one.
(code comments explain why)
Rename the index to match constraint name in the PRIMARY KEY clause
Back in ATExecAddIndex()
Use the index OID returned by get_pkey_index_oid() to tell
DefineIndex() to not create index.
Now mark the index as having 'indisprimary' flag.
In DefineIndex() and index_create() APIs
pass an additional flag: index_exists
Skip various actions based on this flag.
The patch contains a few tests, and doesn't yet have a docs patch.
The development branch is at
http://github.com/gurjeet/postgres/tree/replace_pkey_index
Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
add_pkey_with_index.patchadd_pkey_with_index.ignore_ws.patch
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Jim C. Nasby, Database Architect j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
replace_pkey_index.uniq+doc.patch.gz
Description: GNU Zip compressed data
--
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] Patch to add a primary key using an existing index
UNIQUE constraints suffer from the same behavior; feel like fixing that too? :)
On Oct 9, 2010, at 1:07 PM, Gurjeet Singh wrote:
This is a continuation from this thread:
http://archives.postgresql.org/pgsql-hackers/2010-09/msg02153.php
The attached patch allows creating a primary key using an existing index.
This capability would be helpful in situations where one wishes to
rebuild/reindex the primary key, but associated downtime is not desirable. It
also allows one to create a table and start using it, while creating a unique
index 'concurrently' and later adding the primary key using the concurrently
built index. Maybe pg_dump can also use it.
The command syntax is:
ALTER TABLE sometable ADD PRIMARY KEY( col1, col2 ) WITH ( INDEX =
'indexname' );
A typical use case:
CREATE INDEX CONCURRENTLY new_pkey_idx ON sometable( a, b );
ALTER TABLE sometable ADD PRIMARY KEY ( a, b ) WITH (INDEX = 'new_pkey_idx' );
- OR -
ALTER TABLE sometable DROP CONSTRAINT sometable_pkey,
ADD PRIMARY KEY ( a, b ) WITH (INDEX = 'new_pkey_idx' );
Notes for the reviewers:
Don't be scared by the size of changes to index.c :) These are mostly
indentation diffs. I have attached two versions of the patch: one is context
diff, and the other is the same except ignoring whitespace changes.
The pseudocode is as follows:
In ATExecAddIndex()
If this ALTER command specifies a PRIMARY KEY
Call get_pkey_index_oid() to perform checks.
In get_pkey_index_oid()
Look for the WITH INDEX option
Reject
if more than one WITH INDEX clause specified
if the index doesn't exist or not found in table's schema
if the index is associated with any CONSTRAINT
if index is not ready or not valid (CONCURRENT buiild? Canceled
CONCURRENT?)
if index is on some other table
if index is not unique
if index is an expression index
if index is a partial index
if index columns do not match the PRIMARY KEY clause in the command
if index is not B-tree
If PRIMARY KEY clause doesn't have a constraint name, assign it one.
(code comments explain why)
Rename the index to match constraint name in the PRIMARY KEY clause
Back in ATExecAddIndex()
Use the index OID returned by get_pkey_index_oid() to tell DefineIndex()
to not create index.
Now mark the index as having 'indisprimary' flag.
In DefineIndex() and index_create() APIs
pass an additional flag: index_exists
Skip various actions based on this flag.
The patch contains a few tests, and doesn't yet have a docs patch.
The development branch is at
http://github.com/gurjeet/postgres/tree/replace_pkey_index
Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
add_pkey_with_index.patchadd_pkey_with_index.ignore_ws.patch
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Jim C. Nasby, Database Architect j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net
--
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] Patch to add a primary key using an existing index
On Sat, Oct 9, 2010 at 2:07 PM, Gurjeet Singh singh.gurj...@gmail.comwrote:
This is a continuation from this thread:
http://archives.postgresql.org/pgsql-hackers/2010-09/msg02153.php
The attached patch allows creating a primary key using an existing index.
I have attached two versions of the patch: one is context diff, and the
other is the same except ignoring whitespace changes.
Attached are gzip'd patches for archives. Archive shows the previous mail
attachments all inline... horrible.
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
add_pkey_with_index.patch.gz
Description: GNU Zip compressed data
add_pkey_with_index.ignore_ws.patch.gz
Description: GNU Zip compressed data
--
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] Patch to add a primary key using an existing index
On 10/09/2010 02:19 PM, Gurjeet Singh wrote: On Sat, Oct 9, 2010 at 2:07 PM, Gurjeet Singh singh.gurj...@gmail.com mailto:singh.gurj...@gmail.com wrote: This is a continuation from this thread: http://archives.postgresql.org/pgsql-hackers/2010-09/msg02153.php The attached patch allows creating a primary key using an existing index. I have attached two versions of the patch: one is context diff, and the other is the same except ignoring whitespace changes. Attached are gzip'd patches for archives. Archive shows the previous mail attachments all inline... horrible. I wish we could get the archive processor to provide access to the attachments even if they have a MIME type of text/whatever. That's a horrid inefficiency. Maybe we could restrict it to text attachments that have a Content-Type with a name attribute that contains the string 'patch', or a similar Content-Disposition filename attribute. cheers andrew
archives, attachments, etc (was: [HACKERS] Patch to add a primary key using an existing index)
Andrew Dunstan and...@dunslane.net writes: I wish we could get the archive processor to provide access to the attachments even if they have a MIME type of text/whatever. That's a horrid inefficiency. Maybe we could restrict it to text attachments that have a Content-Type with a name attribute that contains the string 'patch', or a similar Content-Disposition filename attribute. I wish our super admins would have some time to resume the work on the new archives infrastructure, that was about ready for integration if not prime time: http://archives.beccati.org/pgsql-hackers/message/276290 As you see it doesn't suffer from this problem, the threading is not split arbitrarily, and less obvious but it runs from a PostgreSQL database. Yes, that means the threading code is exercising our recursive querying facility, as far as I understand it. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: archives, attachments, etc (was: [HACKERS] Patch to add a primary key using an existing index)
On Sat, Oct 9, 2010 at 3:30 PM, Dimitri Fontaine dimi...@2ndquadrant.frwrote:
Andrew Dunstan and...@dunslane.net writes:
I wish we could get the archive processor to provide access to the
attachments even if they have a MIME type of text/whatever. That's a
horrid inefficiency. Maybe we could restrict it to text attachments
that have a Content-Type with a name attribute that contains the
string 'patch', or a similar Content-Disposition filename attribute.
I wish our super admins would have some time to resume the work on the
new archives infrastructure, that was about ready for integration if not
prime time:
http://archives.beccati.org/pgsql-hackers/message/276290
As you see it doesn't suffer from this problem, the threading is not
split arbitrarily, and less obvious but it runs from a PostgreSQL
database. Yes, that means the threading code is exercising our recursive
querying facility, as far as I understand it.
Something looks wrong with that thread. The message text in my mails is
missing. Perhaps that is contained in the .bin files but I can't tell as the
link leads to 404 Not Found.
Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com
singh.gurj...@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
