Re: WIP: Deferrable unique constraints

Thanks for the thorough review. I attach an new version of the patch,
updated to HEAD, and with the index AM change discussed.

2009/7/18 Jeff Davis <pgsql(at)j-davis(dot)com>:
>
> 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.
>

Fixed.

> 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.
>

So basically NOT DEFERRABLE should behave the same as
DEFERRABLE-always-IMMEDIATE? I guess that you'd want something that
blocks at the first conflict from another transaction but otherwise
queues up potential conflicts to be checked at the end. It seems
plausible to build that on top of what I've done so far, but I'm not
brave enough to try in this patch (which is after all about implementing
deferrable).

> 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?
>

Not too hard in the btree code, just a minor reshuffle. It seems to
have no noticeable performance hit, so I've done it there, but...

> 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.
>

that's right, the current constraint syntax doesn't allow the constraint
index to be built concurrently. Perhaps in the future...

> 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.
>

Yeah, I agree with you on that, so I've tidied it up a bit.

> 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.
>

Done. Thanks for the feedback.

- Dean

> 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
>
>
>