Re: WIP: Deferrable unique constraints - Mailing list pgsql-hackers

Review feedback:

1. Compiler warning:

index.c: In function ‘index_create’:
index.c:784: warning: implicit declaration of function ‘SystemFuncName’

2. I know that the GIN, GiST, and Hash AMs don't use the
uniqueness_check argument at all -- in fact it is #ifdef'ed out.
However, for the sake of documentation it would be good to change those
unused arguments (in, e.g., gininsert()) to be IndexUniqueCheck enums
rather than bools.

3. The unique constraint no longer waits for concurrent transactions to
finish if the unique constraint is deferrable anyway (and it's not time
to actually check the constraint yet). That makes sense, because the
whole point is to defer the constraint. However, that means there are a
few degenerate situations that were OK before, but can now get us into
trouble.

For instance, if you have a big delete and a concurrent big insert, the
current code will just block at the first conflicting tuple and wait for
the delete to finish. With deferrable constraints, it would save all of
those tuples up as potential conflicts, using a lot of memory, when it
is not strictly necessary because the tuples will be gone anyway. I'm
not particularly worried about this situation -- because I think it's a
reasonable thing to expect when using deferred constraints -- but I'd
like to bring it up.

4. Requiring DEFERRABLE after UNIQUE in order to get commands like
"UPDATE ... SET i = i + 1" to work isn't following the spec. I'm not
sure what we should do here, because the 8.4 behavior is not following
the spec at all, but people may still want it.

5. In the docs, 50.2: "This is the only situation ...": it's a little
unclear what "this" is.

6. Missing support for CREATE INDEX CONCURRENTLY is unfortunate. What
would be involved in adding support?

7. It looks like deferrable unique indexes can only be created by adding
a constraint, not as part of the CREATE INDEX syntax. One consequence is
that CIC can't be supported (right?). If you don't plan to support CIC,
then maybe that's OK.

8. Code like the following:   is_vacuum ? UNIQUE_CHECK_NO :   deferUniqueCheck ? UNIQUE_CHECK_PARTIAL :
relationDescs[i]->rd_index->indisunique?   UNIQUE_CHECK_YES : UNIQUE_CHECK_NO);
 

Is a little difficult to read, at least for me. It's a style thing, so
you don't have to agree with me about this.

9. Passing problemIndexList to AfterTriggerSaveEvent seems a little
awkward. I don't see an obvious way to make it cleaner, but I thought
it's worth mentioning.

10. You're overloading tgconstrrelid to hold the constraint's index's
oid, when normally it holds the referenced table. You should probably
document this a little better, because I don't think that field is used
to hold an index oid anywhere else.

The rest of the patch seems fairly complete. Tests, documentation, psql,
and pg_dump support look good. And the patch works, of course. Code and
comments look good to me as well.

I like the patch. It solves a problem, brings us closer to the SQL
standard, and the approach seems reasonable to me.

Regards,Jeff Davis