On Sat, Nov 20, 2010 at 9:00 AM, Steve Singer <ssinger...@sympatico.ca>wrote:
> > Submission Review: > ======================== > > Tests > -------- > The expected output for the regression tests you added don't match > what I'm getting when I run the tests with your patch applied. > I think you just need to regenerate the expected results they seem > to be from a previous version of the patch (different error messages > etc..). > > Fixed. Also modified one test to cover the case where constraint name is provided. > > Documentation > --------------- > > I was able to generate the docs. > > The ALTER TABLE page under the synopsis has > > ADD table_constraint > > where table_constraint is defined on the CREATE TABLE page. > On the CREATE TABLE page table_constraint isn't defined as having the WITH > , the WITH is part of index_parameters. > > I propose the alter table page instead have > > ADD table_constraint [index_parameters] > > where index_parameters also references the CREATE TABLE page like > table_constraint. > IMHO index_parameters is an optional component of table_constraint, and hence can't be mentioned here, at least not the way shown above. I have made slight improvements to the doc which might help the user understand that this WITH(INDEX=) option is exclusive to ALTER TABLE and not provided by CREATE TABLE. > Usability Review > ==================== > > Behaviour > ------------- > I feel that if the ALTER TABLE ... renames the the index > a NOTICE should be generated. We generate notices about creating an index > for a new pkey. We should give them a notice that we are renaming an index > on them. > Done. > > Coding Review: > ====================== > > Error Messages > ----------------- > in tablecmds your errdetail messages often don't start with a capital > letter. I belive the preference is to have the errdetail strings start with > a capital letter and end with a period. > Fixed. > > > tablecmds.c - get_constraint_index_oid > > contains the check > > /* Currently only B-tree indexes are suupported for primary keys */ > if (index_rel->rd_rel->relam != BTREE_AM_OID) > elog(ERROR, "\"%s\" is not a B-Tree index", > index_name); > > but above we already validate that the index is a unique index with another > check. Today only B-tree indexes support unique constraints. If this > changed at some point and we could have a unique index of some other type, > would something in this patch need to be changed to support them? If we are > only depending on the uniqueness property then I think this check is covered > by the uniquness one higher in the function. > > Also note the typo in your comment above (suupported) > I agree; code removed. > Comments > ----------------- > > index.c: Line 671 and 694. Your indentation changes make the comments > run over 80 characters. If you end up submitting a new version > of the patch I'd reformat those two comments. > Fixed. > > Other than those issues the patch looks good to me. > Thanks for your time Steve. Regards, PS: I will be mostly unavailable between 11/25 and 12/6, so wouldn't mind if somebody took ownership of this patch for that duration. -- gurjeet.singh @ EnterpriseDB - The Enterprise Postgres Company http://www.EnterpriseDB.com singh.gurj...@{ gmail | yahoo }.com Twitter/Skype: singh_gurjeet Mail sent from my BlackLaptop device
replace_pkey_index.revised.patch.gz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
