WIP: Deferrable unique constraints
This is a feature I would find useful, and I wanted to learn more
about the database internals, so I thought I would give it a go as an
exercise.
I added 2 new index insert modes to support deferrable. During the
initial index insert, if the constraint is deferrable, it does a
"partial" check of uniqueness. This is a non-blocking check which
allows duplicates in, but can distinguish between definitely unique
and potentially non-unique. For potential uniqueness violations a
deferred trigger is queued to do a full check at the end of the
statement or transaction, or when SET CONSTRAINTS is called. The
trigger then replays the insert in a "fake insert" mode, which doesn't
actually insert anything - it just checks that what is already there
is unique, waiting for other transactions if necessary.
This approach works well if the number of potential conflicts is
small. For example, if uniqueness is not actually violated, there is
no significant penalty for the deferred check, since no triggers are
queued:
pgdevel=# CREATE TABLE foo(a int UNIQUE DEFERRABLE INITIALLY DEFERRED);
NOTICE: CREATE TABLE / UNIQUE will create implicit index "foo_a_key"
for table "foo"
CREATE TABLE
Time: 79.131 ms
pgdevel=# INSERT INTO foo (SELECT * FROM generate_series(0, 1999999,
2)); -- 1M rows
INSERT 0 1000000
Time: 11403.906 ms
pgdevel=# BEGIN;
BEGIN
Time: 0.145 ms
pgdevel=# UPDATE foo SET a=a+1; -- Uniqueness never violated
UPDATE 1000000
Time: 21258.245 ms
pgdevel=# COMMIT;
COMMIT
Time: 83.267 ms
compared with:
pgdevel=# CREATE TABLE foo(a int UNIQUE);
NOTICE: CREATE TABLE / UNIQUE will create implicit index "foo_a_key"
for table "foo"
CREATE TABLE
Time: 110.277 ms
pgdevel=# INSERT INTO foo (SELECT * FROM generate_series(0, 1999999,
2)); -- 1M rows
INSERT 0 1000000
Time: 11196.600 ms
pgdevel=# BEGIN;
BEGIN
Time: 0.146 ms
pgdevel=# UPDATE foo SET a=a+1; -- Uniqueness never violated
UPDATE 1000000
Time: 21054.430 ms
pgdevel=# COMMIT;
COMMIT
Time: 186.174 ms
However, if there are lots of temporarily non-unique values, the
performance impact is much greater, since the unique check is repeated
for each row at commit time:
pgdevel=# CREATE TABLE foo(a int UNIQUE DEFERRABLE INITIALLY DEFERRED);
NOTICE: CREATE TABLE / UNIQUE will create implicit index "foo_a_key"
for table "foo"
CREATE TABLE
Time: 64.244 ms
pgdevel=# INSERT INTO foo (SELECT * FROM generate_series(0, 1999999,
2)); -- 1M rows
INSERT 0 1000000
Time: 11621.438 ms
pgdevel=# BEGIN;
BEGIN
Time: 0.146 ms
pgdevel=# UPDATE foo SET a=a+2; -- Uniqueness temporarily violated
UPDATE 1000000
Time: 21807.128 ms
pgdevel=# COMMIT;
COMMIT
Time: 14974.916 ms
And similarly for an "immediate" check:
pgdevel=# CREATE TABLE foo(a int UNIQUE DEFERRABLE INITIALLY IMMEDIATE);
NOTICE: CREATE TABLE / UNIQUE will create implicit index "foo_a_key"
for table "foo"
CREATE TABLE
Time: 44.477 ms
pgdevel=# INSERT INTO foo (SELECT * FROM generate_series(0, 1999999,
2)); -- 1M rows
INSERT 0 1000000
Time: 12083.089 ms
pgdevel=# UPDATE foo SET a=a+2;
UPDATE 1000000
Time: 37173.210 ms
Also, as for FKs and other queued triggers, this doesn't scale because
the queue is held in memory.
Curing the scalability problem by spooling the queue to disk shouldn't
be too hard to do, but that doesn't address the problem that if a
significant proportion of rows from the table need to be checked, it
is far quicker to scan the whole index once than check row by row.
In fact, with this data on my machine, a single scan takes around
0.5sec, so on that basis a full scan would be quicker if more than
around 3% of the data is flagged as potentially non-unique, although
the planner might be a better judge of that.
I can't see a way of coding something that can switch over to a full
scan, without first recording all the potential violations. Then I'm
not entirely sure how best to do the switch-over logic.
- Dean
Attachments:
deferrable_unique.patchapplication/octet-stream; name=deferrable_unique.patchDownload+842-371
First, I'm happy that you're working on this; I think it's important. I
am working on another index constraints feature that may have some
interaction:
http://archives.postgresql.org/pgsql-hackers/2009-07/msg00302.php
Let me know if you see any potential conflicts between our work.
On Tue, 2009-07-07 at 19:38 +0100, Dean Rasheed wrote:
For potential uniqueness violations a
deferred trigger is queued to do a full check at the end of the
statement or transaction, or when SET CONSTRAINTS is called. The
trigger then replays the insert in a "fake insert" mode, which doesn't
actually insert anything - it just checks that what is already there
is unique, waiting for other transactions if necessary.
What does the deferred trigger do? Do you need a "fake insert" mode or
can you use an index search instead?
I'm thinking that you could just store the TID of the tuple that causes
the potential violation in your list. Then, when you recheck the list,
for each potential violation, find the tuple from the TID, do a search
using the appropriate attributes, and if you get multiple results
there's a conflict.
Regards,
Jeff Davis
On Tue, 2009-07-07 at 19:38 +0100, Dean Rasheed wrote:
This approach works well if the number of potential conflicts is
small.
[...]
Curing the scalability problem by spooling the queue to disk shouldn't
be too hard to do, but that doesn't address the problem that if a
significant proportion of rows from the table need to be checked, it
is far quicker to scan the whole index once than check row by row.
Another approach that might be worth considering is to build a temporary
index and try to merge them at constraint-checking time. That might work
well for unique.
However, there are some potential issues. I didn't think this through
yet, but here is a quick list just to get some thoughts down:
1. It would be tricky to merge while checking constraints if we are
supporting more general constraints like in my proposal
( http://archives.postgresql.org/pgsql-hackers/2009-07/msg00302.php ).
2. Which indexes can be merged efficiently, and how much effort would it
take to make this work?
3. A related issue: making indexes mergeable would be useful for bulk
inserts as well.
4. At the end of the command, the index needs to work, meaning that
queries would have to search two indexes. That may be difficult (but
check the GIN fast insert code, which does something similar).
5. The temporary index still can't be enforcing constraints if they are
deferred, so it won't solve all the issues here.
Regards,
Jeff Davis
Jeff Davis wrote:
First, I'm happy that you're working on this; I think it's important. I
am working on another index constraints feature that may have some
interaction:http://archives.postgresql.org/pgsql-hackers/2009-07/msg00302.php
Let me know if you see any potential conflicts between our work.
Hi Jeff,
Yes, I've been following that other thread with interest. I don't think
that there is any conflict between the 2 patches, although there may be
some interaction.
My stuff is specific to UNIQUE, and my triggers make the assumption that
this is enforced by an index. The triggers don't actually care what kind
of index, although of course currently it can only be a btree.
I guess that your generalised constraints could be made deferrable in a
similar way, and would use different triggers to do the final check, so
no problem there.
On Tue, 2009-07-07 at 19:38 +0100, Dean Rasheed wrote:
For potential uniqueness violations a
deferred trigger is queued to do a full check at the end of the
statement or transaction, or when SET CONSTRAINTS is called. The
trigger then replays the insert in a "fake insert" mode, which doesn't
actually insert anything - it just checks that what is already there
is unique, waiting for other transactions if necessary.What does the deferred trigger do? Do you need a "fake insert" mode or
can you use an index search instead?
Well the point about the "fake insert" mode is that it has to be able to
block on another transaction, to see if it commits or rolls back a
potentially conflicting value.
ISTM that trying to do this with an index search would open up the
potential for all sorts of race conditions.
- Dean
Show quoted text
I'm thinking that you could just store the TID of the tuple that causes
the potential violation in your list. Then, when you recheck the list,
for each potential violation, find the tuple from the TID, do a search
using the appropriate attributes, and if you get multiple results
there's a conflict.Regards,
Jeff Davis
Jeff Davis wrote:
On Tue, 2009-07-07 at 19:38 +0100, Dean Rasheed wrote:
This approach works well if the number of potential conflicts is
small.[...]
Curing the scalability problem by spooling the queue to disk shouldn't
be too hard to do, but that doesn't address the problem that if a
significant proportion of rows from the table need to be checked, it
is far quicker to scan the whole index once than check row by row.Another approach that might be worth considering is to build a temporary
index and try to merge them at constraint-checking time. That might work
well for unique.
I'm not really sure what you mean by a "temporary index". Do you mean
one that you would just throw away at the end of the statement? That
seems a bit heavy-weight.
Also it seems too specific to unique constraints. I think it would be
better to cure the scalability issues for all constraints and triggers
in one place, in the after triggers queue code.
I had hoped that after doing deferrable unique constraints, I might
apply a similar approach to other constraints, eg. a deferrable check
constraint. In that case, an index doesn't help, and there is no choice
but to check the rows one at a time.
Unique (and also FK) are special, in that they have potentially more
optimal ways of checking them in bulk. ISTM that this is an orthogonal
concept to the issue of making the trigger queue scalable, except that
there ought to be an efficient way of discarding all the queued entries
for a particular constraint, if we decide to check it en masse (perhaps
a separate queue per constraint, or per trigger).
- Dean
Show quoted text
However, there are some potential issues. I didn't think this through
yet, but here is a quick list just to get some thoughts down:1. It would be tricky to merge while checking constraints if we are
supporting more general constraints like in my proposal
( http://archives.postgresql.org/pgsql-hackers/2009-07/msg00302.php ).2. Which indexes can be merged efficiently, and how much effort would it
take to make this work?3. A related issue: making indexes mergeable would be useful for bulk
inserts as well.4. At the end of the command, the index needs to work, meaning that
queries would have to search two indexes. That may be difficult (but
check the GIN fast insert code, which does something similar).5. The temporary index still can't be enforcing constraints if they are
deferred, so it won't solve all the issues here.Regards,
Jeff Davis
Here is an updated version of this patch which should apply to HEAD,
with updated docs, regression tests, pg_dump and psql \d.
It works well for small numbers of temporary uniqueness violations,
and at least as well (in fact about twice as fast) as deferred FK
checks for large numbers of deferred checks.
I propose trying to improve performance and scalability for large
numbers of deferred checks in a separate patch.
- Dean
Attachments:
deferrable_unique.patchapplication/octet-stream; name=deferrable_unique.patchDownload+1207-495
On Sun, 2009-07-12 at 14:14 +0100, Dean Rasheed wrote:
Here is an updated version of this patch which should apply to HEAD,
with updated docs, regression tests, pg_dump and psql \d.It works well for small numbers of temporary uniqueness violations,
and at least as well (in fact about twice as fast) as deferred FK
checks for large numbers of deferred checks.
I took a brief look at this. You're extending the index AM, and that
might not be necessary. It might be fine, but usually there is a lot of
discussion around the changing of important APIs, so it might be worth
looking at alternatives.
With the patch I'm working on for generalized index constraints, there
would be no need to extend the index AM. However, I don't expect my
mechanism to replace the existing unique btree constraints, because I
would expect the existing unique constraints to be faster (I haven't
tested yet, though).
Perhaps we could instead use the TRY/CATCH mechanism. It's generally
difficult to figure out from the code exactly what happened, but in this
case we have the error code ERRCODE_UNIQUE_VIOLATION. So, just check for
that error code rather than passing back a boolean. You might want to
change the signature of _bt_check_unique() so that it doesn't have to
raise the error inside, and you can raise the error from _bt_doinsert().
The only problem there is telling the btree AM whether or not to do the
insert or not (i.e. fake versus real insert). Perhaps you can just do
that with careful use of a global variable?
Sure, all of this is a little ugly, but we've already acknowledged that
there is some ugliness around the existing unique constraint and the
btree code that supports it (for one, the btree AM accesses the heap).
I propose trying to improve performance and scalability for large
numbers of deferred checks in a separate patch.
Would it be possible to just check how long the list of potential
conflicts is growing, and if it gets to big, just replace them all with
a "bulk check" event?
Regards,
Jeff Davis
On Tue, Jul 14, 2009 at 09:56:48AM -0700, Jeff Davis wrote:
On Sun, 2009-07-12 at 14:14 +0100, Dean Rasheed wrote:
Here is an updated version of this patch which should apply to HEAD,
with updated docs, regression tests, pg_dump and psql \d.It works well for small numbers of temporary uniqueness violations,
and at least as well (in fact about twice as fast) as deferred FK
checks for large numbers of deferred checks.I took a brief look at this. You're extending the index AM, and that
might not be necessary. It might be fine, but usually there is a lot of
discussion around the changing of important APIs, so it might be worth
looking at alternatives.With the patch I'm working on for generalized index constraints, there
would be no need to extend the index AM. However, I don't expect my
mechanism to replace the existing unique btree constraints, because I
would expect the existing unique constraints to be faster (I haven't
tested yet, though).Perhaps we could instead use the TRY/CATCH mechanism. It's generally
difficult to figure out from the code exactly what happened, but in this
case we have the error code ERRCODE_UNIQUE_VIOLATION. So, just check for
that error code rather than passing back a boolean. You might want to
change the signature of _bt_check_unique() so that it doesn't have to
raise the error inside, and you can raise the error from _bt_doinsert().The only problem there is telling the btree AM whether or not to do the
insert or not (i.e. fake versus real insert). Perhaps you can just do
that with careful use of a global variable?Sure, all of this is a little ugly, but we've already acknowledged that
there is some ugliness around the existing unique constraint and the
btree code that supports it (for one, the btree AM accesses the heap).
I am looking at adding unique support to hash indexes for 8.5 and
they will definitely need to visit the heap.
Regards,
Ken
Show quoted text
I propose trying to improve performance and scalability for large
numbers of deferred checks in a separate patch.Would it be possible to just check how long the list of potential
conflicts is growing, and if it gets to big, just replace them all with
a "bulk check" event?Regards,
Jeff Davis
Jeff Davis wrote:
The only problem there is telling the btree AM whether or not to do the
insert or not (i.e. fake versus real insert). Perhaps you can just do
that with careful use of a global variable?Sure, all of this is a little ugly, but we've already acknowledged that
there is some ugliness around the existing unique constraint and the
btree code that supports it (for one, the btree AM accesses the heap).
My 2c on this issue: if this is ugly (and it is) and needs revisiting to
extend it, please by all means let's make it not ugly instead of moving
the ugliness around. I didn't read the original proposal in detail so
IMBFOS, but it doesn't seem like using our existing deferred constraints
to handle uniqueness checks unuglifies this code enough ... For example
I think we'd like to support stuff like "UPDATE ... SET a = -a" where
the table is large.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
On Tue, 2009-07-14 at 13:29 -0500, Kenneth Marshall wrote:
I am looking at adding unique support to hash indexes for 8.5 and
they will definitely need to visit the heap.
Have you seen this patch?
http://archives.postgresql.org/message-id/1246840119.19547.126.camel@jdavis
This patch will support unique constraints for hash indexes as well.
There may still be a use-case for specialized hash index unique
constraints, similar to btree, but please follow the work to make sure
that no work is wasted.
Also, I don't see a problem with using the same hacks in the hash index
code as is used in the btree index code. If you see a better way, or if
you think index AM changes would be useful to you as well, you should
probably open that discussion.
I was trying to provide an alternative to an index AM API change,
because I thought there might be some resistance to that. However, if
there are multiple index AMs that can make use of it, there is a
stronger case for an API change.
Regards,
Jeff Davis
On Tue, 2009-07-14 at 15:00 -0400, Alvaro Herrera wrote:
My 2c on this issue: if this is ugly (and it is) and needs revisiting to
extend it, please by all means let's make it not ugly instead of moving
the ugliness around. I didn't read the original proposal in detail so
IMBFOS, but it doesn't seem like using our existing deferred constraints
to handle uniqueness checks unuglifies this code enough ... For example
I think we'd like to support stuff like "UPDATE ... SET a = -a" where
the table is large.
I don't entirely understand what you're suggesting.
1. Are you saying that an AM API change is the best route? If so, we
should probably start a discussion along those lines. Heikki is already
changing the API for index-only scans, and Dean's API change proposal
may be useful for Kenneth's unique hash indexes. You might as well all
attack the current API in unison ;)
2. Even if we allow some kind of bulk constraint check to optimize the
"set a = a + 1" case, we should still allow some much cheaper mechanism
to defer retail constraint violations. For that, why not make use of the
existing constraint trigger mechanism?
Regards,
Jeff Davis
2009/7/14 Alvaro Herrera <alvherre@commandprompt.com>:
Jeff Davis wrote:
The only problem there is telling the btree AM whether or not to do the
insert or not (i.e. fake versus real insert). Perhaps you can just do
that with careful use of a global variable?Sure, all of this is a little ugly, but we've already acknowledged that
there is some ugliness around the existing unique constraint and the
btree code that supports it (for one, the btree AM accesses the heap).
Well the ugliness referred to here (btree accessing the heap) seems
like a necessary evil. I don't think I want to add to it by
introducing global variables.
My 2c on this issue: if this is ugly (and it is) and needs revisiting to
extend it, please by all means let's make it not ugly instead of moving
the ugliness around. I didn't read the original proposal in detail so
IMBFOS, but it doesn't seem like using our existing deferred constraints
to handle uniqueness checks unuglifies this code enough ... For example
I think we'd like to support stuff like "UPDATE ... SET a = -a" where
the table is large.
This patch works OK for around 1M rows. 10M is a real stretch (for me
it took around 1.7GB of backend memory). Any larger than that is not
going to be feasible. There is a separate TODO item to tackle this
scalability limit for deferred triggers, and I'd like to tackle that
in a separate patch. I think more discussion needs to be had on ways
to fix this (and hopefully unuglify that code in the process).
ITSM that it is not simply a matter of spooling the current queues to
disk. There is code in there which scans whole queues shuffling things
around. So perhaps a queue per trigger would help optimise this,
allowing us to move a whole queue cheaply, or drop it in favour of a
bulk check. I've not thought it through much more than that so far.
- Dean
Jeff Davis wrote:
1. Are you saying that an AM API change is the best route? If so, we
should probably start a discussion along those lines. Heikki is already
changing the API for index-only scans, and Dean's API change proposal
may be useful for Kenneth's unique hash indexes. You might as well all
attack the current API in unison ;)
Yeah, I don't think there's any point in keeping the API stable just for
the sake of keeping it stable. I mean surely we don't want to break it
for no reason, but if we can clean up the uniqueness check situation
somehow by breaking the API, for all means let's explore that ... (I
don't think anybody likes the way btree currently abuses the heap API;
it's just that it's so damn fast to do it that way. Certainly we don't
want to make it slower!)
2. Even if we allow some kind of bulk constraint check to optimize the
"set a = a + 1" case, we should still allow some much cheaper mechanism
to defer retail constraint violations. For that, why not make use of the
existing constraint trigger mechanism?
Sure, perhaps you're right, which is why I added the disclaimer that I
hadn't actually read the patch in any detail ...
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Tue, 2009-07-14 at 20:32 +0100, Dean Rasheed wrote:
Well the ugliness referred to here (btree accessing the heap) seems
like a necessary evil. I don't think I want to add to it by
introducing global variables.
Ok, try to coordinate with Kenneth to make sure that the API change
satisfies deferrable uniques for both btree and hash indexes. I don't
have a strong opinion one way or another about the API change.
Regards,
Jeff Davis
On Tue, Jul 14, 2009 at 12:13:33PM -0700, Jeff Davis wrote:
On Tue, 2009-07-14 at 13:29 -0500, Kenneth Marshall wrote:
I am looking at adding unique support to hash indexes for 8.5 and
they will definitely need to visit the heap.Have you seen this patch?
http://archives.postgresql.org/message-id/1246840119.19547.126.camel@jdavis
This patch will support unique constraints for hash indexes as well.
There may still be a use-case for specialized hash index unique
constraints, similar to btree, but please follow the work to make sure
that no work is wasted.Also, I don't see a problem with using the same hacks in the hash index
code as is used in the btree index code. If you see a better way, or if
you think index AM changes would be useful to you as well, you should
probably open that discussion.I was trying to provide an alternative to an index AM API change,
because I thought there might be some resistance to that. However, if
there are multiple index AMs that can make use of it, there is a
stronger case for an API change.Regards,
Jeff Davis
I will take a look at that patch. My thought was to use the same
process as the btree support for unique indexes since it has been
well tested and optimized.
Thanks,
Ken
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
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@j-davis.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
Show quoted text
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
Attachments:
deferrable_unique.patchapplication/octet-stream; name=deferrable_unique.patchDownload+1246-540
Dean Rasheed wrote:
Thanks for the thorough review. I attach an new version of the patch,
updated to HEAD, and with the index AM change discussed.
Wow, this is a large patch.
I didn't do a thorough review, but some quickies I noticed:
* Please move the code that says that it should be in a new file to a
new file.
* Please don't mimic the silliness of RI_FKey_check of an unused return
value. Just make it return void, and have the caller use the proper
PG_RETURN_FOO macro. (Unless there's a specific reason to not do
things that way)
* I'm not sure about the changes in trigger.h and related elsewhere.
Seems related to the new list in AfterTriggerSaveEvent, which is
used in ways that seem to conflict with its header comment ... I
wonder if we should be doing that in the executor itself instead.
In any case it's inconsistent that the list is added to
ExecARInsertTriggers but not to ExecARUpdateTrigger et al ...
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
2009/7/20 Alvaro Herrera <alvherre@commandprompt.com>:
Dean Rasheed wrote:
Thanks for the thorough review. I attach an new version of the patch,
updated to HEAD, and with the index AM change discussed.Wow, this is a large patch.
I didn't do a thorough review, but some quickies I noticed:
* Please move the code that says that it should be in a new file to a
new file.
Ah yes, I forgot about that. Will do.
utils/adt/constraints.c ?
I'm thinking other deferred constraints (eg. CHECK) might get added to
it one day.
* Please don't mimic the silliness of RI_FKey_check of an unused return
value. Just make it return void
OK.
and have the caller use the proper
PG_RETURN_FOO macro. (Unless there's a specific reason to not do
things that way)
It looks like I can't use PG_RETURN_NULL() because that will break
the trigger calling code, but I could use PG_RETURN_VOID(). But is that
the recommended style? Section 35.4 of the docs suggests using
PointerGetDatum(NULL).
* I'm not sure about the changes in trigger.h and related elsewhere.
Seems related to the new list in AfterTriggerSaveEvent, which is
used in ways that seem to conflict with its header comment ... I
wonder if we should be doing that in the executor itself instead.
Yes I suppose the comment is a bit misleading. I put the check there
because that's where the similar RI checks are, which already conflict
with the header comment. The simplest solution is to just update the
comment.
This seemed like the least intrusive way to do this. It is also called from
copy.c which duplicates some of the executor code.
In any case it's inconsistent that the list is added to
ExecARInsertTriggers but not to ExecARUpdateTrigger et al ...
Yes it is added to ExecARUpdateTriggers().
- Dean
Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
* Please move the code that says that it should be in a new file to a
�new file.
Ah yes, I forgot about that. Will do.
utils/adt/constraints.c ?
Please, no. Constraints are not a datatype. Do not copy the brain-dead
decision that put ri_triggers there.
regards, tom lane
