2009/7/15 Jeff Davis <pgsql@j-davis.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