Обсуждение: Patch to add a primary key using an existing index

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.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.

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

<div dir="ltr"><div class="gmail_quote">On Sat, Oct 9, 2010 at 3:30 PM, Dimitri Fontaine <span dir="ltr"><<a
href="mailto:dimitri@2ndquadrant.fr">dimitri@2ndquadrant.fr</a>></span>wrote:<br /><blockquote class="gmail_quote"
style="margin:0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> Andrew Dunstan <<a
href="mailto:andrew@dunslane.net">andrew@dunslane.net</a>>writes:<br /> > I wish we could get the archive
processorto provide access to the<br /> > attachments even if they have a MIME type of text/whatever. That's a<br />
>horrid inefficiency. Maybe we could restrict it to text attachments<br /> > that have a Content-Type with a name
attributethat contains the<br /> > string 'patch', or a similar Content-Disposition filename attribute.<br /><br />
Iwish our super admins would have some time to resume the work on the<br /> new archives infrastructure, that was about
readyfor integration if not<br /> prime time:<br /><br />  <a
href="http://archives.beccati.org/pgsql-hackers/message/276290"
target="_blank">http://archives.beccati.org/pgsql-hackers/message/276290</a><br/><br /> As you see it doesn't suffer
fromthis problem, the threading is not<br /> split arbitrarily, and less obvious but it runs from a PostgreSQL<br />
database.Yes, that means the threading code is exercising our recursive<br /> querying facility, as far as I understand
it.<br/><br /></blockquote></div><br clear="all" />Something looks wrong with that thread. The message text in my mails
ismissing. Perhaps that is contained in the .bin files but I can't tell as the link leads to 404 Not Found.<br /><br
/>Regards,<br/>-- <br />gurjeet.singh<br />@ EnterpriseDB - The Enterprise Postgres Company<br /><a
href="http://www.EnterpriseDB.com">http://www.EnterpriseDB.com</a><br/><br />singh.gurjeet@{ gmail | yahoo }.com<br
/>Twitter/Skype:singh_gurjeet<br /><br />Mail sent from my BlackLaptop device<br /></div> 

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
downtimeis 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
ofthe 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
> <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

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.

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-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.

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

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@commandprompt.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@xzilla.net> wrote:
> What exactly was the objection to the following -->
> ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name;
> Is the objection that you might have been trying to specify a constraint
> named "using" ? I'm willing to make that option more difficult. :-)

I think it's that someone might expect the word after USING to be the
name of an index AM.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/3/10 12:27 PM, Robert Haas wrote:
> On Fri, Dec 3, 2010 at 2:56 PM, r t <pgsql@xzilla.net> wrote:
>> What exactly was the objection to the following -->
>> ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name;
>> Is the objection that you might have been trying to specify a constraint
>> named "using" ? I'm willing to make that option more difficult. :-)
> 
> I think it's that someone might expect the word after USING to be the
> name of an index AM.

Seems unlikely to cause confusion to me.

However, I don't see why we need (column_list). Surely the index has a
column list already?

ALTER TABLE table_name ADD CONSTRAINT pk_name PRIMARY KEY USING index_name

... seems like the syntax most consistent with the existing commands.
Anything else would be confusingly inconsistent with the way you add a
brand-new PK.

--                                  -- Josh Berkus                                    PostgreSQL Experts Inc.
                        http://www.pgexperts.com
 

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@agliodbs.com> wrote:
> 
> > However, I don't see why we need (column_list). Surely the index has a
> > column list already?
> >
> > ALTER TABLE table_name ADD CONSTRAINT pk_name PRIMARY KEY USING index_name
> >
> > ... seems like the syntax most consistent with the existing commands.
> > Anything else would be confusingly inconsistent with the way you add a
> > brand-new PK.
> >
> >
> Uh, the syntax I posted was based on this currently valid syntax:
> 
> ALTER TABLE table_name ADD PRIMARY KEY (column_list);
> 
> The constraint bit is optional, which is why I left it out, but I presume it
> would be optional with the new syntax as well... Also, I'm not wedded to the
> idea of keeping the column list, but if you are arguing to make it super
> consistent, then I think you need to include it.

If you consider that an index basically is, in some sense, a pre-canned
column list, then:

ALTER TABLE table_name ADD PRIMARY KEY (column_list);
ALTER TABLE table_name ADD PRIMARY KEY USING index_name;

are parallel constructions. And it avoids the error case of the user
providing a column list that doesn't match the index.

Ross
-- 
Ross Reedstrom, Ph.D.                                 reedstrm@rice.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@rice.edu> writes:
> If you consider that an index basically is, in some sense, a pre-canned
> column list, then:

> ALTER TABLE table_name ADD PRIMARY KEY (column_list);
> ALTER TABLE table_name ADD PRIMARY KEY USING index_name;

> are parallel constructions. And it avoids the error case of the user
> providing a column list that doesn't match the index.

+1 for that approach.  One other thought is that ordinarily, the
add-constraint syntax ensures that the constraint is named the same as
its underlying index; in fact we go so far as to keep them in sync if
you rename the index later.  But after

ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING index_name;

they'd be named differently, unless we (a) throw an error or (b)
forcibly rename the index.  Neither of those ideas seems to satisfy the
principle of least surprise, but leaving it alone seems like it will
also lead to confusion later.

I wonder whether, in the same spirit as not letting the user write a
column name list that might not match, we should pick a syntax that
doesn't allow specifying a constraint name different from the index
name.  In the case where you say CONSTRAINT it'd be a bit plausible
to write something like

ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING EXISTING INDEX;

(implying that the index to use is named con_name) but I don't know
what to do if you want to leave off the "CONSTRAINT name" clause.

Thoughts?
        regards, tom lane

On Dec 4, 2010, at 1:30 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Ross J. Reedstrom" <reedstrm@rice.edu> writes:
>> If you consider that an index basically is, in some sense, a pre-canned
>> column list, then:
>
>> ALTER TABLE table_name ADD PRIMARY KEY (column_list);
>> ALTER TABLE table_name ADD PRIMARY KEY USING index_name;
>
>> are parallel constructions. And it avoids the error case of the user
>> providing a column list that doesn't match the index.
>
> +1 for that approach.  One other thought is that ordinarily, the
> add-constraint syntax ensures that the constraint is named the same as
> its underlying index; in fact we go so far as to keep them in sync if
> you rename the index later.  But after
>
> ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING index_name;
>
> they'd be named differently, unless we (a) throw an error or (b)
> forcibly rename the index.  Neither of those ideas seems to satisfy the
> principle of least surprise, but leaving it alone seems like it will
> also lead to confusion later.

I think that might be the best way though.

> I wonder whether, in the same spirit as not letting the user write a
> column name list that might not match, we should pick a syntax that
> doesn't allow specifying a constraint name different from the index
> name.  In the case where you say CONSTRAINT it'd be a bit plausible
> to write something like
>
> ALTER TABLE table_name ADD CONSTRAINT con_name PRIMARY KEY USING EXISTING INDEX;
>
> (implying that the index to use is named con_name) but I don't know
> what to do if you want to leave off the "CONSTRAINT name" clause.

Because this seems plain weird.

...Robert
>

Robert Treat <rob@xzilla.net> writes:
> Actually I think I'd even be comfortable with A, either you must name the
> constraint after the index, or you can leave the constraint name out, and
> we'll use the index name.

Or we could omit the "CONSTRAINT name" clause from the syntax
altogether.

I think that allowing the names to be different is a bad idea.  That
hasn't been possible in the past and there's no apparent reason why
this feature should suddenly make it possible.  We will have problems
with it, for instance failures on name collisions because generated
names are only checked against one catalog or the other.
        regards, tom lane

On Dec 4, 2010, at 11:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Treat <rob@xzilla.net> writes:
>> Actually I think I'd even be comfortable with A, either you must name the
>> constraint after the index, or you can leave the constraint name out, and
>> we'll use the index name.
>
> Or we could omit the "CONSTRAINT name" clause from the syntax
> altogether.
>
> I think that allowing the names to be different is a bad idea.  That
> hasn't been possible in the past and there's no apparent reason why
> this feature should suddenly make it possible.  We will have problems
> with it, for instance failures on name collisions because generated
> names are only checked against one catalog or the other.

So maybe we should start by deciding what the semantics should be, and then decide what syntax would convey those
semantics.

What would make sense to me is: create a pk constraint with the sane name as the existing unique index.  If that
constraintname 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
constraintname 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@xzilla.net> wrote:
> > What exactly was the objection to the following -->
> > ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name;
> > Is the objection that you might have been trying to specify a constraint
> > named "using" ? I'm willing to make that option more difficult. :-)
> 
> I think it's that someone might expect the word after USING to be the
> name of an index AM.

That could be avoided by writing

USING INDEX <name>

<div dir="ltr"><div class="gmail_quote">On Sun, Dec 5, 2010 at 2:09 PM, Peter Eisentraut <span dir="ltr"><<a
href="mailto:peter_e@gmx.net">peter_e@gmx.net</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:
0pt0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="im">On fre, 2010-12-03 at
15:27-0500, Robert Haas wrote:<br /> > On Fri, Dec 3, 2010 at 2:56 PM, r t <<a
href="mailto:pgsql@xzilla.net">pgsql@xzilla.net</a>>wrote:<br /> > > What exactly was the objection to the
following--><br /> > > ALTER TABLE table_name ADD PRIMARY KEY (column_list) USING index_name;<br /> > >
Isthe objection that you might have been trying to specify a constraint<br /> > > named "using" ? I'm willing to
makethat option more difficult. :-)<br /> ><br /> > I think it's that someone might expect the word after USING
tobe the<br /> > name of an index AM.<br /><br /></div>That could be avoided by writing<br /><br /> USING INDEX
<name><br/><br /></blockquote></div><br />Allowing USING INDEX along with USING INDEX TABLESPACE is causing
shift/reduceconflicts.<br /><br />I liked the proposal upthread of providing alternate syntax where user does not have
tospecify column-list and system picks up that list from the index.<br /><br clear="all" />ALTER TABLE table_name ADD
[CONSTRAINTcons_name] PRIMARY KEY (column_list) [WITH (...)] [USING INDEX TABLESPACE tblspcname];<br />ALTER TABLE
table_nameADD [CONSTRAINT cons_name] PRIMARY KEY [WITH (...)] [USING INDEX index_name];<br /><br />This would also help
avoidthe bug that Itagaki found, where the user wants to use an existing index, and also specifies USING INDEX
TABLESPACE.<br/><br />But I still hold a bias towards renaming the index to match constraint name (with a NOTICE),
ratherthan require that the constraint name match the index name, because the constraint name is optional and when it
isnot provided system has to generate a name and we have to rename the index anyway to maintain consistency.<br /><br
/>Followingare the gram.y changes that I am going to start with:<br /><br /> %type <boolean>
constraints_set_mode<br/>-%type <str>        OptTableSpace OptConsTableSpace OptTableSpaceOwner<br />+%type
<str>       OptTableSpace OptConsTableSpace OptConsIndex OptTableSpaceOwner<br />  %type <list>  
opt_check_option<br/><br />[...]<br />            | UNIQUE '(' columnList ')' opt_definition OptConsTableSpace<br
/>               ConstraintAttributeSpec<br />                {<br />                    Constraint *n =
makeNode(Constraint);<br/>                     n->contype = CONSTR_UNIQUE;<br />                    n->location =
@1;<br/>                    n->keys = $3;<br />                    n->options = $5;<br />                   
n->indexspace= $6;<br />                     n->deferrable = ($7 & 1) != 0;<br />                   
n->initdeferred= ($7 & 2) != 0;<br />                    $$ = (Node *)n;<br />                }<br />+          
|UNIQUE opt_definition OptConsIndex ConstraintAttributeSpec<br /> +               {<br />+                   Constraint
*n= makeNode(Constraint);<br />+                   n->contype = CONSTR_UNIQUE;<br />+                  
n->location= @1;<br />+                   n->options = $2;<br /> +                   n->indexname = $3;<br
/>+                  n->deferrable = ($4 & 1) != 0;<br />+                   n->initdeferred = ($4 & 2)
!=0;<br />+                   $$ = (Node *)n;<br />+               }<br />             | PRIMARY KEY '(' columnList ')'
opt_definitionOptConsTableSpace<br />                ConstraintAttributeSpec<br />                {<br
/>                   Constraint *n = makeNode(Constraint);<br />                     n->contype = CONSTR_PRIMARY;<br
/>                   n->location = @1;<br />                    n->keys = $4;<br />                   
n->options= $6;<br />                    n->indexspace = $7;<br />                     n->deferrable = ($8
&1) != 0;<br />                    n->initdeferred = ($8 & 2) != 0;<br />                    $$ = (Node
*)n;<br/>                }<br />+           | PRIMARY KEY opt_definition OptConsIndex ConstraintAttributeSpec<br />
+              {<br />+                   Constraint *n = makeNode(Constraint);<br />+                   n->contype
=CONSTR_PRIMARY;<br />+                   n->location = @1;<br />+                   n->options = $3;<br />
+                  n->indexname = $4;<br />+                   n->deferrable = ($5 & 1) != 0;<br
/>+                  n->initdeferred = ($5 & 2) != 0;<br />+                   $$ = (Node *)n;<br
/>+              }<br />             | EXCLUDE access_method_clause '(' ExclusionConstraintList ')'<br /><br />[...]<br
/> OptConsTableSpace:  USING INDEX TABLESPACE name   { $$ = $4; }<br />            |
/*EMPTY*/                            { $$ = NULL; }<br />         ;<br /> <br />+OptConsIndex:   USING INDEX name   {
$$= $3; }<br />+           | /*EMPTY*/             { $$ = NULL; }<br />+       ;<br />+<br /><br />Regards,<br />-- <br
/>gurjeet.singh<br/>@ EnterpriseDB - The Enterprise Postgres Company<br /><a
href="http://www.EnterpriseDB.com">http://www.EnterpriseDB.com</a><br/><br />singh.gurjeet@{ gmail | yahoo }.com<br
/>Twitter/Skype:singh_gurjeet<br /><br />Mail sent from my BlackLaptop device<br /></div> 

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> But I still hold a bias towards renaming the index to match constraint name
> (with a NOTICE), rather than require that the constraint name match the
> index name, because the constraint name is optional and when it is not
> provided system has to generate a name and we have to rename the index
> anyway to maintain consistency.

No.  If the constraint name is not specified, we should certainly use
the existing index name, not randomly rename it.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> If the constraint name is not specified, we should certainly use
> the existing index name, not randomly rename it.
+1
-Kevin

On Thu, Dec 9, 2010 at 2:51 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If the constraint name is not specified, we should certainly use
>> the existing index name, not randomly rename it.
>
> +1

+1

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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@sympatico.ca> wrote:
> I'm marking this as returned with feedback pending your answer on the
> possible memory leak above but I think the patch is very close to being
> ready.

Please use "Waiting on Author" if the patch is to be considered
further for this CommitFest, and "Returned with Feedback" only if it
will not be further considered for this CommitFest.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Steve Singer <ssinger_pg@sympatico.ca> writes:
> src/backend/parser/parse_utilcmd.c: 1452
> Your calling strdup on the attribute name.  I don't have a good enough 
> grasp on the code to be able to trace this through to where the memory 
> gets free'd.  Does it get freed? Should/could this be a call to pstrdup

strdup() is pretty much automatically wrong in the parser, not to
mention most of the rest of the backend.  pstrdup is likely what was
meant.  If that's the only issue then I don't see any need to wait on
the author, so will take this one.
        regards, tom lane

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@sss.pgh.pa.us> wrote:
> One other issue that might be worthy of discussion is that as things
> stand, execution of the ADD CONSTRAINT USING INDEX syntax will cause
> the constraint to absorb the index as an INTERNAL dependency.  That
> means dropping the constraint would make the index go away silently ---
> it no longer has any separate life. If the intent is just to provide a
> way to get the effect of ALTER ADD PRIMARY KEY CONCURRENTLY, then this
> behavior is probably fine.  But someone who believes DROP CONSTRAINT
> exactly reverses the effects of ADD CONSTRAINT might be surprised.
> Comments?

Well, I think the behavior as described is what we want.  If the
syntax associated with that behavior is going to lead to confusion,
I'd view that as a deficiency of the syntax, rather than a deficiency
of the behavior.  (I make this comment with some reluctance
considering the amount of bikeshedding we've already done on this
topic, but... that's what I think.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> On Tue, Jan 25, 2011 at 9:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> One other issue that might be worthy of discussion is that as things
>> stand, execution of the ADD CONSTRAINT USING INDEX syntax will cause
>> the constraint to absorb the index as an INTERNAL dependency.  That
>> means dropping the constraint would make the index go away silently ---
>> it no longer has any separate life.

> Since we rename the index automatically to match the constraint name,
> implying that the index now belongs to the system, I think the user should
> expect the index to go away with the constraint; else we have to remember
> index's original name and restore that name on DROP CONSTRAINT, which IMHO
> will be even more unintuitive.

Yeah, that's a good point.  Also, the documented example usage of this
feature is
  To recreate a primary key constraint, without blocking updates while the  index is rebuilt:
<programlisting>
CREATE UNIQUE INDEX CONCURRENTLY dist_id_temp_idx on distributors (dist_id);
ALTER TABLE distributors DROP CONSTRAINT distributors_pkey,   ADD CONSTRAINT distributors_pkey PRIMARY KEY USING INDEX
dist_id_temp_idx;
</programlisting>

with the implication that after you do that, the installed index is
exactly like you would have gotten from straight ADD PRIMARY KEY.
If there's something funny about it, then it's not just a replacement.

In the end I think this is mainly an issue of setting appropriate
expectations in the documentation.  I've added the following text to
the ALTER TABLE manual page:
    <para>     After this command is executed, the index is <quote>owned</> by the     constraint, in the same way as
ifthe 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

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> Attached is the updated patch with doc changes and test cases.

Applied with assorted corrections.  Aside from the refactoring I wanted,
there were various oversights.

> I have consciously disallowed the ability to specify storage_parameters
> using the WITH clause, if somebody thinks it is wise to allow that and is
> needed, I can do that.

AFAICS, WITH would be supplied at the time of index creation; it's not
appropriate to include it here, any more than INDEX TABLESPACE.

A point that may or may not have gotten discussed back when is that it's
important that the result of this process be dumpable by pg_dump, ie
there not be any hidden discrepancies between the state after ADD
CONSTRAINT USING INDEX and the state you'd get from straight ADD
CONSTRAINT, because the latter is the syntax pg_dump is going to emit.
ADD CONSTRAINT can handle WITH and INDEX TABLESPACE, so carrying those
over from the original index specification is no problem, but
non-default index opclasses or sort ordering options would be a big
problem.  That would in particular completely break pg_upgrade, because
the on-disk index wouldn't match the catalog entries created by running
pg_dump.  I added some code to check and disallow non-default opclass
and options.
        regards, tom lane