Re: Patch to add a primary key using an existing index

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.