Patch to add a primary key using an existing index
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.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
On Sat, Oct 9, 2010 at 2:07 PM, Gurjeet Singh <singh.gurjeet@gmail.com>wrote:
This is a continuation from this thread:
http://archives.postgresql.org/pgsql-hackers/2010-09/msg02153.phpThe 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.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
On 10/09/2010 02:19 PM, Gurjeet Singh wrote:
On Sat, Oct 9, 2010 at 2:07 PM, Gurjeet Singh <singh.gurjeet@gmail.com
<mailto:singh.gurjeet@gmail.com>> wrote:This is a continuation from this thread:
http://archives.postgresql.org/pgsql-hackers/2010-09/msg02153.phpThe 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
Andrew Dunstan <andrew@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
On Sat, Oct 9, 2010 at 3:30 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr>wrote:
Andrew Dunstan <andrew@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.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
Hi Gurjeet,
On 09/10/2010 22:54, Gurjeet Singh wrote:
On Sat, Oct 9, 2010 at 3:30 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr
<mailto:dimitri@2ndquadrant.fr>> wrote:
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.
Thanks for the private email to point this thread out. I've been overly
busy lately and missed it.
I'll try to debug what happens with your message formatting as soon as I
can find some time.
Cheers
--
Matteo Beccati
Development & Consulting - http://www.beccati.com/
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 clauseBack 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.comsingh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeetMail sent from my BlackLaptop device
<add_pkey_with_index.patch><add_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 jim@nasby.net
512.569.9461 (cell) http://jim.nasby.net
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 <jim@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? CanceledCONCURRENT?)
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 thecommand
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 tellDefineIndex() 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.comsingh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeetMail sent from my BlackLaptop device
<add_pkey_with_index.patch><add_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 jim@nasby.net
512.569.9461 (cell) http://jim.nasby.net
--
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
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.gurjeet@gmail.com>wrote:
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_indexRegards,
On Mon, Nov 1, 2010 at 10:29 PM, Jim Nasby <jim@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? CanceledCONCURRENT?)
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 thecommand
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 tellDefineIndex() 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.comsingh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeetMail sent from my BlackLaptop device
<add_pkey_with_index.patch><add_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 jim@nasby.net
512.569.9461 (cell) http://jim.nasby.net--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.comsingh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeetMail sent from my BlackLaptop device
--
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
Attachments:
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
On Sat, Nov 20, 2010 at 9:00 AM, Steve Singer <ssinger_pg@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.
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.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
Attachments:
On 10-11-22 09:37 AM, Gurjeet Singh wrote:
On Sat, Nov 20, 2010 at 9:00 AM, Steve Singer <ssinger_pg@sympatico.ca
<mailto:ssinger_pg@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
Show quoted text
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.comsingh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeetMail sent from my BlackLaptop device
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_pg@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.
Attachments:
On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger@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
On Sun, Nov 28, 2010 at 8:06 PM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger@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
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.takahiro@gmail.com> wrote:On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger@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 <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@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
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.takahiro@gmail.com> wrote:On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger@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.
On Fri, Dec 3, 2010 at 2:23 PM, Peter Eisentraut <peter_e@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.takahiro@gmail.com> wrote:On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger@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
On 03.12.2010 21:43, Robert Haas wrote:
On Fri, Dec 3, 2010 at 2:23 PM, Peter Eisentraut<peter_e@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.takahiro@gmail.com> wrote:On Fri, Nov 26, 2010 at 05:58, Steve Singer<ssinger@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
On Fri, Dec 3, 2010 at 2:43 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Dec 3, 2010 at 2:23 PM, Peter Eisentraut <peter_e@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.takahiro@gmail.com> wrote:On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger@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 KEYOther 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
