Re: 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(dot)gurjeet(at){ 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(dot)gurjeet(at)gmail(dot)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.
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com

singh(dot)gurjeet(at){ 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(dot)gurjeet(at)gmail(dot)com
> <mailto:singh(dot)gurjeet(at)gmail(dot)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

Andrew Dunstan <andrew(at)dunslane(dot)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(at)2ndquadrant(dot)fr>wrote:

> Andrew Dunstan <andrew(at)dunslane(dot)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(dot)gurjeet(at){ 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(at)2ndquadrant(dot)fr
> <mailto:dimitri(at)2ndquadrant(dot)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 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(dot)gurjeet(at){ gmail | yahoo }.com
> Twitter/Skype: singh_gurjeet
>
> Mail 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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Jim C. Nasby, Database Architect jim(at)nasby(dot)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(at)nasby(dot)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(dot)gurjeet(at){ gmail | yahoo }.com
> > Twitter/Skype: singh_gurjeet
> >
> > Mail 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(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
>
> --
> Jim C. Nasby, Database Architect jim(at)nasby(dot)net
> 512.569.9461 (cell) http://jim.nasby.net
>
>
>

--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com

singh(dot)gurjeet(at){ 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(dot)gurjeet(at)gmail(dot)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_index
>
> Regards,
>
>
> On Mon, Nov 1, 2010 at 10:29 PM, Jim Nasby <jim(at)nasby(dot)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(dot)gurjeet(at){ gmail | yahoo }.com
>> > Twitter/Skype: singh_gurjeet
>> >
>> > Mail 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(at)postgresql(dot)org)
>> > To make changes to your subscription:
>> > http://www.postgresql.org/mailpref/pgsql-hackers
>>
>> --
>> Jim C. Nasby, Database Architect jim(at)nasby(dot)net
>> 512.569.9461 (cell) http://jim.nasby.net
>>
>>
>>
>
>
> --
> gurjeet.singh
> @ EnterpriseDB - The Enterprise Postgres Company
> http://www.EnterpriseDB.com
>
> singh(dot)gurjeet(at){ 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(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

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(at)sympatico(dot)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(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

On 10-11-22 09:37 AM, Gurjeet Singh wrote:
> On Sat, Nov 20, 2010 at 9:00 AM, Steve Singer <ssinger_pg(at)sympatico(dot)ca
> <mailto:ssinger_pg(at)sympatico(dot)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(dot)gurjeet(at){ gmail | yahoo }.com
> Twitter/Skype: singh_gurjeet
>
> Mail 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(at)sympatico(dot)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.

On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger(at)ca(dot)afilias(dot)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(dot)takahiro(at)gmail(dot)com> wrote:
> On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger(at)ca(dot)afilias(dot)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(dot)takahiro(at)gmail(dot)com> wrote:
> > On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger(at)ca(dot)afilias(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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(dot)takahiro(at)gmail(dot)com> wrote:
> > On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger(at)ca(dot)afilias(dot)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(at)gmx(dot)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(dot)takahiro(at)gmail(dot)com> wrote:
>> > On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger(at)ca(dot)afilias(dot)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(at)gmx(dot)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(dot)takahiro(at)gmail(dot)com> wrote:
>>>> On Fri, Nov 26, 2010 at 05:58, Steve Singer<ssinger(at)ca(dot)afilias(dot)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(at)gmail(dot)com> wrote:

> On Fri, Dec 3, 2010 at 2:23 PM, Peter Eisentraut <peter_e(at)gmx(dot)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(dot)takahiro(at)gmail(dot)com> wrote:
> >> > On Fri, Nov 26, 2010 at 05:58, Steve Singer <ssinger(at)ca(dot)afilias(dot)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

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 <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

On Fri, Dec 3, 2010 at 2:56 PM, r t <pgsql(at)xzilla(dot)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

On 12/3/10 12:27 PM, Robert Haas wrote:
> On Fri, Dec 3, 2010 at 2:56 PM, r t <pgsql(at)xzilla(dot)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

On Fri, Dec 3, 2010 at 4:41 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:

> On 12/3/10 12:27 PM, Robert Haas wrote:
> > On Fri, Dec 3, 2010 at 2:56 PM, r t <pgsql(at)xzilla(dot)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

On Fri, Dec 03, 2010 at 05:16:04PM -0500, Robert Treat wrote:
> On Fri, Dec 3, 2010 at 4:41 PM, Josh Berkus <josh(at)agliodbs(dot)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. reedstrm(at)rice(dot)edu
Systems Engineer & Admin, Research Scientist phone: 713-348-6166
Connexions http://cnx.org fax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE

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

"Ross J. Reedstrom" <reedstrm(at)rice(dot)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

On Dec 4, 2010, at 1:30 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Ross J. Reedstrom" <reedstrm(at)rice(dot)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
>

On Sat, Dec 4, 2010 at 6:48 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Dec 4, 2010, at 1:30 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > "Ross J. Reedstrom" <reedstrm(at)rice(dot)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

Robert Treat <rob(at)xzilla(dot)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

On Dec 4, 2010, at 11:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Treat <rob(at)xzilla(dot)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

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

On fre, 2010-12-03 at 15:27 -0500, Robert Haas wrote:
> On Fri, Dec 3, 2010 at 2:56 PM, r t <pgsql(at)xzilla(dot)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>

On Sun, Dec 5, 2010 at 2:09 PM, Peter Eisentraut <peter_e(at)gmx(dot)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 <pgsql(at)xzilla(dot)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 <str> OptTableSpace OptConsTableSpace OptTableSpaceOwner
+%type <str> OptTableSpace 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(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> If the constraint name is not specified, we should certainly use
> the existing index name, not randomly rename it.

+1

-Kevin

On Thu, Dec 9, 2010 at 2:51 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Thu, Dec 9, 2010 at 2:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

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

On Sun, Jan 16, 2011 at 5:34 PM, Steve Singer <ssinger_pg(at)sympatico(dot)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

Steve Singer <ssinger_pg(at)sympatico(dot)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

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

On Mon, Jan 24, 2011 at 7:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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

Sorry for not being on top of this.

On Tue, Jan 25, 2011 at 9:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> writes:
> On Tue, Jan 25, 2011 at 9:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 <quote>owned</> by the
constraint, in the same way as if the index had been built by
a regular <literal>ADD PRIMARY KEY</> or <literal>ADD UNIQUE</>
command. In particular, dropping the constraint will make the index
disappear too.
</para>

regards, tom lane

On Wed, Jan 26, 2011 at 5:31 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 <quote>owned</> by the
> constraint, in the same way as if the index had been built by
> a regular <literal>ADD PRIMARY KEY</> or <literal>ADD 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(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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

On Wed, Jan 26, 2011 at 6:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device