Re: review patch: Distinguish between unique indexes and unique constraints

On Thu, Jul 29, 2010 at 7:56 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> The patch is in context diff format and applies cleanly.  No doc
> changes were included.  Arguably there should be a mention in the
> documentation for psql's \d+ commend, but since the number of child
> tables and the display of reloptions aren't mentioned, perhaps we're
> not trying to list *all* the differences the + makes.  If those
> don't merit mention, this doesn't.
>
> The patch implements what it's supposed to, there was consensus on
> the list that we want it, we don't already have it, the SQL spec
> doesn't apply to psql's backslash commands, and it was added just
> for the verbose listing as requested (with no objection).  There
> are no pg_dump issues.  The only danger is that someone is depending
> on the current \d+ format and will be surprised at the new
> distinction between unique indexes and unique constraints.  All
> bases seem to me to be covered.
>
> The feature works as advertised, I saw no problem corner cases, no
> assertion failures or crashes.
>
> The patch causes no noticeable performance change, doesn't claim to
> affect performance, and will not slow down anything else.
>
> The patch does not follow coding guidelines, as it places the
> opening brace for an "if" block on the same line as the "if".
> There are no portability issues; it should work everywhere that
> current backslash commands work.  The purpose of the code is obvious
> enough to not require any comment lines.  It does what it says,
> correctly.  It doesn't produce compiler warnings and does not crash.
>
> It fits together coherently with all else, and introduces no new
> interdependencies.
>
> I am attaching a new version of the patch which fixes the formatting
> issue and rearranges the code slightly in a way which I find more
> readable.  (I leave it to the committer to determine which
> arrangement better suits.)
>
> I am marking this "Ready for Committer".

I have committed this patch with a few changes. First, I felt that
there was little point in showing this detail only in verbose mode;
indeed, it seems like that could be confusing in some circumstances.
(I thought I checked this was an index not a constraint? Oh, crap, I
forgot to say \d+). So I removed that check. Second, I felt that
UNIQUE, CONSTRAINT, btree (a) was one too many commas, so I made it
just say UNIQUE CONSTRAINT, btree (a) instead. I didn't see any
discussion of either of these points in the previous discussion of
this patch, so hopefully neither change is objectionable (but if it
is, we can revisit it, if someone thinks I'm all wet!).

Thanks to Josh for the patch and Kevin for the review; I think this is
a good change.

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