Re: WIP: generalized index constraints

2009/7/15 Jeff Davis <pgsql(at)j-davis(dot)com>:
> Updated patch attached.
>
> Changes:
>  * Added syntax support:
>     CREATE INDEX foo_idx ON foo ... (a CONSTRAINT =, b CONSTRAINT &&);
>  * More aggressively clear the shared memory entries to avoid
>   unnecessary checks
>  * Code cleanup
>
> TODO:
>  * When adding constraint to table with data already in it, verify that
>   existing data satisfies constraint.
>  * Clean up error messages a little
>  * Docs

Hi Jeff,

I've been assigned to do an initial review of your patch. Because
this patch is still WIP, there's not a lot for me to do. I see from
the thread that there are still some design questions that remain
open, but I won't be addressing those. The internals of indexes and
constraints is not an area of the code I have any particular insight
about.

The patch applies cleanly, builds successfully and passes regression
tests. I read through the code changes, and didn't notice any code
convention violations.

I had a play around with the feature in psql. I think the syntax is
okay, but using "ALTER TABLE ... ADD" as you mentioned upthread could
be a better option.

I noticed that there's no change to the output of \d in psql to show
the constraint, so when I do a \d on my test table, I can see that
there's a gist index there, but I can't tell that there is also a
constraint on it. This seems like a pretty significant shortcoming.
Essentially once you've created one of these index constraints, it
vanishes into the catalogs and becomes invisible to the user. This
might call for a modification of pg_get_indexdef()?

You've already noted this as a TODO, but clearly the error messages
need some serious help. If I deliberately violate an index constraint
I get:

ERROR: conflict detected 3

At minimum the message should use the term "constraint" and give the
name of the index that has detected the conflict. I think something
like this would be okay:

ERROR: new record violates constraint on index "circle_idx"

I also noticed that the patch does not include any changes or
additions to the regression test suite. Perhaps it ought to?

That's all the feedback I have for the moment. I hope you find my
comments constructive.

Cheers,
BJ