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

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