Re: [HACKERS] FOR KEY LOCK foreign keys
On Wed, Jul 27, 2011 at 7:16 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: One thing I have not addressed is Noah's idea about creating a new lock mode, KEY UPDATE, that would let us solve the initial problem that this patch set to resolve in the first place. I am not clear on exactly how that is to be implemented, because currently heap_update and heap_delete do not grab any kind of lock but instead do their own ad-hoc waiting. I think that might need to be reshuffled a bit, to which I haven't gotten yet, and is a radical enough idea that I would like it to be discussed by the hackers community at large before setting sail on developing it. In the meantime, this patch does improve the current situation quite a lot. I haven't looked at the patch yet, but do you have a pointer to Noah's proposal? And/or a description of how it differs from what you implemented here? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Robert Haas's message of mié ago 03 12:14:15 -0400 2011: On Wed, Jul 27, 2011 at 7:16 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: One thing I have not addressed is Noah's idea about creating a new lock mode, KEY UPDATE, that would let us solve the initial problem that this patch set to resolve in the first place. I am not clear on exactly how that is to be implemented, because currently heap_update and heap_delete do not grab any kind of lock but instead do their own ad-hoc waiting. I think that might need to be reshuffled a bit, to which I haven't gotten yet, and is a radical enough idea that I would like it to be discussed by the hackers community at large before setting sail on developing it. In the meantime, this patch does improve the current situation quite a lot. I haven't looked at the patch yet, but do you have a pointer to Noah's proposal? And/or a description of how it differs from what you implemented here? Yes, see his review email here: http://archives.postgresql.org/message-id/20110211071322.gb26...@tornado.leadboat.com It's long, but search for the part where he talks about KEY UPDATE. The way my patch works is explained by Noah there. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Kevin Grittner's message of sáb jul 16 14:03:31 -0400 2011: Noah Misch wrote: With this patch in its final form, I have completed 180+ suite runs without a failure. The attached patch allows the tests to pass when default_transaction_isolation is stricter than 'read committed'. This is a slight change from the previously posted version of the files (because of a change in the order of statements, based on the timeouts), and in patch form this time. Thanks, applied. Sorry for the delay. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Kevin Grittner's message: Noah Misch wrote: With this patch in its final form, I have completed 180+ suite runs without a failure. The attached patch allows the tests to pass when default_transaction_isolation is stricter than 'read committed'. This is a slight change from the previously posted version of the files (because of a change in the order of statements, based on the timeouts), and in patch form this time. Thanks, applied. Sorry for the delay. My patch was intended to supplement Noah's patch here: http://archives.postgresql.org/pgsql-hackers/2011-07/msg00867.php Without his patch, there is still random failure on my work machine at all transaction isolation levels. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Kevin Grittner's message of mar jul 19 13:49:53 -0400 2011: Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Kevin Grittner's message: Noah Misch wrote: With this patch in its final form, I have completed 180+ suite runs without a failure. The attached patch allows the tests to pass when default_transaction_isolation is stricter than 'read committed'. This is a slight change from the previously posted version of the files (because of a change in the order of statements, based on the timeouts), and in patch form this time. Thanks, applied. Sorry for the delay. My patch was intended to supplement Noah's patch here: I'm aware of that, thanks. I'm getting that one in too, shortly. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Noah Misch's message of sáb jul 16 13:11:49 -0400 2011: In any event, I have attached a patch that fixes the problems I have described here. To ignore autovacuum, it only recognizes a wait when one of the backends under test holds a conflicting lock. (It occurs to me that perhaps we should expose a pg_lock_conflicts(lockmode_held text, lockmode_req text) function to simplify this query -- this is a fairly common monitoring need.) Applied it. I agree that having such an utility function is worthwhile, particularly if we're working on making pg_locks more usable as a whole. (I wasn't able to reproduce Rémi's hangups here, so I wasn't able to reproduce the other bits either.) With that change in place, my setup survived through about fifty suite runs at a time. The streak would end when session 2 would unexpectedly detect a deadlock that session 1 should have detected. The session 1 deadlock_timeout I chose, 20ms, is too aggressive. When session 2 is to issue the command that completes the deadlock, it must do so before session 1 runs the deadlock detector. Since we burn 10ms just noticing that the previous statement has blocked, that left only 10ms to issue the next statement. This patch bumps the figure from 20s to 100ms; hopefully that will be enough for even a decently-loaded virtual host. Committed this too. With this patch in its final form, I have completed 180+ suite runs without a failure. In the absence of better theories on the cause for the buildfarm failures, we should give the buildfarm a whirl with this patch. Great. If there is some other failure mechanism, we'll find out ... I apologize for the quantity of errata this change is entailing. No need to apologize. I might as well apologize myself because I didn't detect these problems on review. But we don't do that -- we just fix the problems and move on. It's great that you were able to come up with a fix quickly. And this is precisely why I committed this way ahead of the patch that it was written to help: we're now not fixing problems in both simultaneously. By the time we get that other patch in, this test harness will be fully robust. Thanks for all your effort in this. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Kevin Grittner kevin.gritt...@wicourts.gov wrote: Noah Misch wrote: With this patch in its final form, I have completed 180+ suite runs without a failure. The attached patch allows the tests to pass when default_transaction_isolation is stricter than 'read committed'. Without these two patches the tests fail about one time out of three on my machine at the office at the 'read committed' transaction isolation level, and all the time at stricter levels. On my machine at home I haven't seen the failures at 'read committed'. I don't know if this is Intel (at work) versus AMD (at home) or what. With both Noah's patch and mine I haven't yet seen a failure in either environment, with a few dozen tries.. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Fri, Jul 15, 2011 at 07:01:26PM -0400, Alvaro Herrera wrote:
Excerpts from Noah Misch's message of mié jul 13 01:34:10 -0400 2011:
coypu failed during the run of the test due to a different session being
chosen
as the deadlock victim. We can now vary deadlock_timeout to prevent this;
see
attached fklocks-tests-deadlock_timeout.patch. This also makes the tests
much
faster on a default postgresql.conf.
I applied your patch, thanks. I couldn't reproduce the failures without
it, even running only the three new tests in a loop a few dozen times.
It's probably more likely to crop up on a loaded system. I did not actually
reproduce it myself. However, if you swap the timeouts, the opposite session
finds the deadlock. From there, I'm convinced that the right timing
perturbations could yield the symptom coypu exhibited.
crake failed when it reported waiting on the first step of an existing
isolation
test (two-ids.spec). I will need to look into that further.
Actually, there are four failures in tests other than the two fixed by
your patch. These are:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crakedt=2011-07-12%2022:32:02
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=nightjardt=2011-07-14%2016:27:00
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pittadt=2011-07-15%2015:00:08
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crakedt=2011-07-15%2018:32:02
Thanks for summarizing. These all boil down to lock waits not anticipated by
the test specs. Having pondered this, I've been able to come up with just one
explanation. If autovacuum runs VACUUM during the test and finds that it can
truncate dead space from the end of a relation, it will acquire an
AccessExclusiveLock. When I decrease autovacuum_naptime to 1s, I do see
plenty of pg_type and pg_attribute truncations during a test run.
When I sought to reproduce this, what I first saw instead was an indefinite
test suite hang. That turned out to arise from an unrelated thinko -- I
assumed that backend IDs were stable for the life of the backend, but they're
only stable for the life of a pgstat snapshot. This fell down when a backend
older than one of the test backends exited during the test:
4199 2011-07-16 03:33:28.733 EDT DEBUG: forked new backend, pid=23984 socket=8
23984 2011-07-16 03:33:28.737 EDT LOG: statement: SET client_min_messages =
warning;
23984 2011-07-16 03:33:28.739 EDT LOG: statement: SELECT i FROM
pg_stat_get_backend_idset() t(i) WHERE pg_stat_get_backend_pid(i) =
pg_backend_pid()
23985 2011-07-16 03:33:28.740 EDT DEBUG: autovacuum: processing database
postgres
4199 2011-07-16 03:33:28.754 EDT DEBUG: forked new backend, pid=23986 socket=8
23986 2011-07-16 03:33:28.754 EDT LOG: statement: SET client_min_messages =
warning;
4199 2011-07-16 03:33:28.755 EDT DEBUG: server process (PID 23985) exited with
exit code 0
23986 2011-07-16 03:33:28.755 EDT LOG: statement: SELECT i FROM
pg_stat_get_backend_idset() t(i) WHERE pg_stat_get_backend_pid(i) =
pg_backend_pid()
4199 2011-07-16 03:33:28.766 EDT DEBUG: forked new backend, pid=23987 socket=8
23987 2011-07-16 03:33:28.766 EDT LOG: statement: SET client_min_messages =
warning;
23987 2011-07-16 03:33:28.767 EDT LOG: statement: SELECT i FROM
pg_stat_get_backend_idset() t(i) WHERE pg_stat_get_backend_pid(i) =
pg_backend_pid()
This led isolationtester to initialize backend_ids = {1,2,2}, making us unable
to detect lock waits correctly. That's also consistent with the symptoms Rémi
Zara just reported. With that fixed, I was able to reproduce the failure due
to autovacuum-truncate-induced transient waiting using this recipe:
- autovacuum_naptime = 1s
- src/test/isolation/Makefile changed to pass --use-existing during installcheck
- Run 'make installcheck' in a loop
- A concurrent session running this in a loop:
CREATE TABLE churn (a int, b int, c int, d int, e int, f int, g int, h int);
DROP TABLE churn;
That yields a steady stream of vacuum truncations, and an associated lock wait
generally capsized the suite within 5-10 runs. Frankly, I have some
difficulty believing that this mechanic alone produced all four failures you
cite above; I suspect I'm still missing some more-frequent cause. Any other
theories on which system background activities can cause a transient lock
wait? It would have to produce a pgstat_report_waiting(true) call, so I
believe that excludes all LWLock and lighter contention.
In any event, I have attached a patch that fixes the problems I have described
here. To ignore autovacuum, it only recognizes a wait when one of the
backends under test holds a conflicting lock. (It occurs to me that perhaps
we should expose a pg_lock_conflicts(lockmode_held text, lockmode_req text)
function to simplify this query -- this is a fairly common monitoring need.)
With that change in place, my setup survived through about fifty suite runs at
a time. The streak would end when session
Re: [HACKERS] FOR KEY LOCK foreign keys
Noah Misch wrote: With this patch in its final form, I have completed 180+ suite runs without a failure. The attached patch allows the tests to pass when default_transaction_isolation is stricter than 'read committed'. This is a slight change from the previously posted version of the files (because of a change in the order of statements, based on the timeouts), and in patch form this time. Since `make installcheck-world` works at all isolation level defaults, as do all previously included isolation tests, it seems like a good idea to keep this up. It will simplify my testing of SSI changes, anyway. -Kevin fklocks-tests-strict-isolation.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Sat, Jul 16, 2011 at 01:03:31PM -0500, Kevin Grittner wrote: Noah Misch wrote: With this patch in its final form, I have completed 180+ suite runs without a failure. The attached patch allows the tests to pass when default_transaction_isolation is stricter than 'read committed'. This is a slight change from the previously posted version of the files (because of a change in the order of statements, based on the timeouts), and in patch form this time. Since `make installcheck-world` works at all isolation level defaults, as do all previously included isolation tests, it seems like a good idea to keep this up. It will simplify my testing of SSI changes, anyway. This does seem sensible. Thanks. -- Noah Mischhttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Noah Misch's message of mié jul 13 01:34:10 -0400 2011: coypu failed during the run of the test due to a different session being chosen as the deadlock victim. We can now vary deadlock_timeout to prevent this; see attached fklocks-tests-deadlock_timeout.patch. This also makes the tests much faster on a default postgresql.conf. I applied your patch, thanks. I couldn't reproduce the failures without it, even running only the three new tests in a loop a few dozen times. crake failed when it reported waiting on the first step of an existing isolation test (two-ids.spec). I will need to look into that further. Actually, there are four failures in tests other than the two fixed by your patch. These are: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crakedt=2011-07-12%2022:32:02 http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=nightjardt=2011-07-14%2016:27:00 http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pittadt=2011-07-15%2015:00:08 http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crakedt=2011-07-15%2018:32:02 The last two are an identical failure in multiple-row-versions: *** *** 1,11 Parsed test spec with 4 sessions starting permutation: rx1 wx2 c2 wx3 ry3 wy4 rz4 c4 c3 wz1 c1 ! step rx1: SELECT * FROM t WHERE id = 100; id txt 100 - step wx2: UPDATE t SET txt = 'b' WHERE id = 100; step c2: COMMIT; step wx3: UPDATE t SET txt = 'c' WHERE id = 100; step ry3: SELECT * FROM t WHERE id = 50; --- 1,12 Parsed test spec with 4 sessions starting permutation: rx1 wx2 c2 wx3 ry3 wy4 rz4 c4 c3 wz1 c1 ! step rx1: SELECT * FROM t WHERE id = 100; waiting ... ! step wx2: UPDATE t SET txt = 'b' WHERE id = 100; ! step rx1: ... completed id txt 100 step c2: COMMIT; step wx3: UPDATE t SET txt = 'c' WHERE id = 100; step ry3: SELECT * FROM t WHERE id = 50; The other failure by crake in two-ids: *** *** 440,447 step c3: COMMIT; starting permutation: rxwy2 wx1 ry3 c2 c3 c1 ! step rxwy2: update D2 set id = (select id+1 from D1); step wx1: update D1 set id = id + 1; step ry3: select id from D2; id --- 440,448 step c3: COMMIT; starting permutation: rxwy2 wx1 ry3 c2 c3 c1 ! step rxwy2: update D2 set id = (select id+1 from D1); waiting ... step wx1: update D1 set id = id + 1; + step rxwy2: ... completed step ry3: select id from D2; id And the most problematic one, in nightjar, is a failure to send two async commands, which is not supported by the new code: --- 255,260 ERROR: could not serialize access due to read/write dependencies among transactions starting permutation: ry2 wx2 rx1 wy1 c2 c1 ! step ry2: SELECT count(*) FROM project WHERE project_manager = 1; waiting ... ! failed to send query: another command is already in progress -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Noah Misch's message of vie mar 11 12:51:14 -0300 2011: On Fri, Feb 11, 2011 at 02:13:22AM -0500, Noah Misch wrote: Automated tests would go a long way toward building confidence that this patch does the right thing. Thanks to the SSI patch, we now have an in-tree test framework for testing interleaved transactions. The only thing it needs to be suitable for this work is a way to handle blocked commands. If you like, I can try to whip something up for that. [off-list ACK followed] Here's a patch implementing that. It applies to master, with or without your KEY LOCK patch also applied, though the expected outputs reflect the improvements from your patch. I add three isolation test specs: fk-contention: blocking-only test case from your blog post fk-deadlock: the deadlocking test case I used during patch review fk-deadlock2: Joel Jacobson's deadlocking test case Thanks for this patch. I have applied it, adjusting the expected output of these tests to the HEAD code. I'll adjust it when I commit the fklocks patch, I guess, but it seemed simpler to have it out of the way; besides it might end up benefitting other people who might be messing with the locking code. I only support one waiting command at a time. As long as one commands continues to wait, I run other commands to completion synchronously. Should be fine for now, I guess. I think this will work on Windows as well as pgbench does, but I haven't verified that. We will find out shortly. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Tue, Jul 12, 2011 at 05:59:01PM -0400, Alvaro Herrera wrote: Excerpts from Noah Misch's message of vie mar 11 12:51:14 -0300 2011: On Fri, Feb 11, 2011 at 02:13:22AM -0500, Noah Misch wrote: Automated tests would go a long way toward building confidence that this patch does the right thing. Thanks to the SSI patch, we now have an in-tree test framework for testing interleaved transactions. The only thing it needs to be suitable for this work is a way to handle blocked commands. If you like, I can try to whip something up for that. [off-list ACK followed] Here's a patch implementing that. It applies to master, with or without your KEY LOCK patch also applied, though the expected outputs reflect the improvements from your patch. I add three isolation test specs: fk-contention: blocking-only test case from your blog post fk-deadlock: the deadlocking test case I used during patch review fk-deadlock2: Joel Jacobson's deadlocking test case Thanks for this patch. I have applied it, adjusting the expected output of these tests to the HEAD code. I'll adjust it when I commit the fklocks patch, I guess, but it seemed simpler to have it out of the way; besides it might end up benefitting other people who might be messing with the locking code. Great. There have been a few recent patches where I would have used this functionality to provide tests, so I'm glad to have it in. I think this will work on Windows as well as pgbench does, but I haven't verified that. We will find out shortly. I see you've added a fix for the MSVC animals; thanks. coypu failed during the run of the test due to a different session being chosen as the deadlock victim. We can now vary deadlock_timeout to prevent this; see attached fklocks-tests-deadlock_timeout.patch. This also makes the tests much faster on a default postgresql.conf. crake failed when it reported waiting on the first step of an existing isolation test (two-ids.spec). I will need to look into that further. Thanks, nm diff --git a/src/test/isolation/expected/fk-deadlock.out b/src/test/isolation/expected/fk-deadlock.out index 6b6ee16..0d86cda 100644 *** a/src/test/isolation/expected/fk-deadlock.out --- b/src/test/isolation/expected/fk-deadlock.out *** *** 32,39 step s1i: INSERT INTO child VALUES (1, 1); step s2i: INSERT INTO child VALUES (2, 1); step s2u: UPDATE parent SET aux = 'baz'; waiting ... step s1u: UPDATE parent SET aux = 'bar'; - step s2u: ... completed ERROR: deadlock detected step s2c: COMMIT; step s1c: COMMIT; --- 32,39 step s2i: INSERT INTO child VALUES (2, 1); step s2u: UPDATE parent SET aux = 'baz'; waiting ... step s1u: UPDATE parent SET aux = 'bar'; ERROR: deadlock detected + step s2u: ... completed step s2c: COMMIT; step s1c: COMMIT; *** *** 52,59 step s2i: INSERT INTO child VALUES (2, 1); step s1i: INSERT INTO child VALUES (1, 1); step s2u: UPDATE parent SET aux = 'baz'; waiting ... step s1u: UPDATE parent SET aux = 'bar'; - step s2u: ... completed ERROR: deadlock detected step s2c: COMMIT; step s1c: COMMIT; --- 52,59 step s1i: INSERT INTO child VALUES (1, 1); step s2u: UPDATE parent SET aux = 'baz'; waiting ... step s1u: UPDATE parent SET aux = 'bar'; ERROR: deadlock detected + step s2u: ... completed step s2c: COMMIT; step s1c: COMMIT; diff --git a/src/test/isolation/expected/fk-deadloindex af3ce8e..6e7f12d 100644 *** a/src/test/isolation/expected/fk-deadlock2.out --- b/src/test/isolation/expected/fk-deadlock2.out *** *** 42,49 step s1u1: UPDATE A SET Col1 = 1 WHERE AID = 1; step s2u1: UPDATE B SET Col2 = 1 WHERE BID = 2; step s2u2: UPDATE B SET Col2 = 1 WHERE BID = 2; waiting ... step s1u2: UPDATE B SET Col2 = 1 WHERE BID = 2; - step s2u2: ... completed ERROR: deadlock detected step s1c: COMMIT; step s2c: COMMIT; --- 42,49 step s2u1: UPDATE B SET Col2 = 1 WHERE BID = 2; step s2u2: UPDATE B SET Col2 = 1 WHERE BID = 2; waiting ... step s1u2: UPDATE B SET Col2 = 1 WHERE BID = 2; ERROR: deadlock detected + step s2u2: ... completed step s1c: COMMIT; step s2c: COMMIT; *** *** 52,59 step s1u1: UPDATE A SET Col1 = 1 WHERE AID = 1; step s2u1: UPDATE B SET Col2 = 1 WHERE BID = 2; step s2u2: UPDATE B SET Col2 = 1 WHERE BID = 2; waiting ... step s1u2: UPDATE B SET Col2 = 1 WHERE BID = 2; - step s2u2: ... completed ERROR: deadlock detected step s2c: COMMIT; step s1c: COMMIT; --- 52,59 step s2u1: UPDATE B SET Col2 = 1 WHERE BID = 2; step s2u2: UPDATE B SET Col2 = 1 WHERE BID = 2; waiting ... step s1u2: UPDATE B SET Col2 = 1 WHERE BID = 2; ERROR: deadlock detected + step s2u2: ... completed step s2c: COMMIT; step s1c: COMMIT; *** *** 82,89 step s2u1: UPDATE B SET
Re: [HACKERS] FOR KEY LOCK foreign keys
On Sun, Jun 19, 2011 at 06:30:41PM +0200, Jesper Krogh wrote: I hope this hasn't been forgotten. But I cant see it has been committed or moved into the commitfest process? If you're asking about that main patch for $SUBJECT rather than those isolationtester changes specifically, I can't speak to the plans for it. I wasn't planning to move the test suite work forward independent of the core patch it serves, but we could do that if there's another application. Thanks, nm On 2011-03-11 16:51, Noah Misch wrote: On Fri, Feb 11, 2011 at 02:13:22AM -0500, Noah Misch wrote: Automated tests would go a long way toward building confidence that this patch does the right thing. Thanks to the SSI patch, we now have an in-tree test framework for testing interleaved transactions. The only thing it needs to be suitable for this work is a way to handle blocked commands. If you like, I can try to whip something up for that. [off-list ACK followed] Here's a patch implementing that. It applies to master, with or without your KEY LOCK patch also applied, though the expected outputs reflect the improvements from your patch. I add three isolation test specs: fk-contention: blocking-only test case from your blog post fk-deadlock: the deadlocking test case I used during patch review fk-deadlock2: Joel Jacobson's deadlocking test case -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On 2011-06-20 22:11, Noah Misch wrote: On Sun, Jun 19, 2011 at 06:30:41PM +0200, Jesper Krogh wrote: I hope this hasn't been forgotten. But I cant see it has been committed or moved into the commitfest process? If you're asking about that main patch for $SUBJECT rather than those isolationtester changes specifically, I can't speak to the plans for it. I wasn't planning to move the test suite work forward independent of the core patch it serves, but we could do that if there's another application. Yes, I was actually asking about the main patch for foreign key locks. Jesper -- Jesper -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
I hope this hasn't been forgotten. But I cant see it has been committed or moved into the commitfest process? Jesper On 2011-03-11 16:51, Noah Misch wrote: On Fri, Feb 11, 2011 at 02:13:22AM -0500, Noah Misch wrote: Automated tests would go a long way toward building confidence that this patch does the right thing. Thanks to the SSI patch, we now have an in-tree test framework for testing interleaved transactions. The only thing it needs to be suitable for this work is a way to handle blocked commands. If you like, I can try to whip something up for that. [off-list ACK followed] Here's a patch implementing that. It applies to master, with or without your KEY LOCK patch also applied, though the expected outputs reflect the improvements from your patch. I add three isolation test specs: fk-contention: blocking-only test case from your blog post fk-deadlock: the deadlocking test case I used during patch review fk-deadlock2: Joel Jacobson's deadlocking test case When a spec permutation would have us run a command in a currently-blocked session, we cannot implement that permutation. Such permutations represent impossible real-world scenarios, anyway. For now, I just explicitly name the valid permutations in each spec file. If the test harness detects this problem, we abort the current test spec. It might be nicer to instead cancel all outstanding queries, issue rollbacks in all sessions, and continue with other permutations. I hesitated to do that, because we currently leave all transaction control in the hands of the test spec. I only support one waiting command at a time. As long as one commands continues to wait, I run other commands to completion synchronously. This decision has no impact on the current test specs, which all have two sessions. It avoided a touchy policy decision concerning deadlock detection. If two commands have blocked, it may be that a third command needs to run before they will unblock, or it may be that the two commands have formed a deadlock. We won't know for sure until deadlock_timeout elapses. If it's possible to run the next step in the permutation (i.e., it uses a different session from any blocked command), we can either do so immediately or wait out the deadlock_timeout first. The latter slows the test suite, but it makes the output more natural -- more like what one would typically after running the commands by hand. If anyone can think of a sound general policy, that would be helpful. For now, I've punted. With a default postgresql.conf, deadlock_timeout constitutes most of the run time. Reduce it to 20ms to accelerate things when running the tests repeatedly. Since timing dictates which query participating in a deadlock will be chosen for cancellation, the expected outputs bearing deadlock errors are unstable. I'm not sure how much it will come up in practice, so I have not included expected output variations to address this. I think this will work on Windows as well as pgbench does, but I haven't verified that. Sorry for the delay on this. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Fri, Feb 11, 2011 at 02:13:22AM -0500, Noah Misch wrote: Automated tests would go a long way toward building confidence that this patch does the right thing. Thanks to the SSI patch, we now have an in-tree test framework for testing interleaved transactions. The only thing it needs to be suitable for this work is a way to handle blocked commands. If you like, I can try to whip something up for that. [off-list ACK followed] Here's a patch implementing that. It applies to master, with or without your KEY LOCK patch also applied, though the expected outputs reflect the improvements from your patch. I add three isolation test specs: fk-contention: blocking-only test case from your blog post fk-deadlock: the deadlocking test case I used during patch review fk-deadlock2: Joel Jacobson's deadlocking test case When a spec permutation would have us run a command in a currently-blocked session, we cannot implement that permutation. Such permutations represent impossible real-world scenarios, anyway. For now, I just explicitly name the valid permutations in each spec file. If the test harness detects this problem, we abort the current test spec. It might be nicer to instead cancel all outstanding queries, issue rollbacks in all sessions, and continue with other permutations. I hesitated to do that, because we currently leave all transaction control in the hands of the test spec. I only support one waiting command at a time. As long as one commands continues to wait, I run other commands to completion synchronously. This decision has no impact on the current test specs, which all have two sessions. It avoided a touchy policy decision concerning deadlock detection. If two commands have blocked, it may be that a third command needs to run before they will unblock, or it may be that the two commands have formed a deadlock. We won't know for sure until deadlock_timeout elapses. If it's possible to run the next step in the permutation (i.e., it uses a different session from any blocked command), we can either do so immediately or wait out the deadlock_timeout first. The latter slows the test suite, but it makes the output more natural -- more like what one would typically after running the commands by hand. If anyone can think of a sound general policy, that would be helpful. For now, I've punted. With a default postgresql.conf, deadlock_timeout constitutes most of the run time. Reduce it to 20ms to accelerate things when running the tests repeatedly. Since timing dictates which query participating in a deadlock will be chosen for cancellation, the expected outputs bearing deadlock errors are unstable. I'm not sure how much it will come up in practice, so I have not included expected output variations to address this. I think this will work on Windows as well as pgbench does, but I haven't verified that. Sorry for the delay on this. nm *** /dev/null --- b/src/test/isolation/expected/fk-contention.out *** *** 0 --- 1,16 + Parsed test spec with 2 sessions + + starting permutation: ins com upd + step ins: INSERT INTO bar VALUES (42); + step com: COMMIT; + step upd: UPDATE foo SET b = 'Hello World'; + + starting permutation: ins upd com + step ins: INSERT INTO bar VALUES (42); + step upd: UPDATE foo SET b = 'Hello World'; + step com: COMMIT; + + starting permutation: upd ins com + step upd: UPDATE foo SET b = 'Hello World'; + step ins: INSERT INTO bar VALUES (42); + step com: COMMIT; *** /dev/null --- b/src/test/isolation/expected/fk-deadlock.out *** *** 0 --- 1,63 + Parsed test spec with 2 sessions + + starting permutation: s1i s1u s1c s2i s2u s2c + step s1i: INSERT INTO child VALUES (1, 1); + step s1u: UPDATE parent SET aux = 'bar'; + step s1c: COMMIT; + step s2i: INSERT INTO child VALUES (2, 1); + step s2u: UPDATE parent SET aux = 'baz'; + step s2c: COMMIT; + + starting permutation: s1i s1u s2i s1c s2u s2c + step s1i: INSERT INTO child VALUES (1, 1); + step s1u: UPDATE parent SET aux = 'bar'; + step s2i: INSERT INTO child VALUES (2, 1); waiting ... + step s1c: COMMIT; + step s2i: ... completed + step s2u: UPDATE parent SET aux = 'baz'; + step s2c: COMMIT; + + starting permutation: s1i s2i s1u s2u s1c s2c + step s1i: INSERT INTO child VALUES (1, 1); + step s2i: INSERT INTO child VALUES (2, 1); + step s1u: UPDATE parent SET aux = 'bar'; + step s2u: UPDATE parent SET aux = 'baz'; waiting ... + step s1c: COMMIT; + step s2u: ... completed + step s2c: COMMIT; + + starting permutation: s1i s2i s2u s1u s2c s1c + step s1i: INSERT INTO child VALUES (1, 1); + step s2i: INSERT INTO child VALUES (2, 1); + step s2u: UPDATE parent SET aux = 'baz'; + step s1u: UPDATE parent SET aux = 'bar'; waiting ... + step s2c: COMMIT; + step s1u: ... completed + step s1c: COMMIT; + + starting permutation: s2i s1i s1u s2u s1c s2c + step s2i: INSERT INTO child VALUES (2, 1); + step s1i:
Re: [HACKERS] FOR KEY LOCK foreign keys
On Mon, Feb 14, 2011 at 6:49 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Marti Raudsepp's message of lun feb 14 19:39:25 -0300 2011: On Fri, Feb 11, 2011 at 09:13, Noah Misch n...@leadboat.com wrote: The patch had a trivial conflict in planner.c, plus plenty of offsets. I've attached the rebased patch that I used for review. For anyone following along, all the interesting hunks touch heapam.c; the rest is largely mechanical. A diff -w patch is also considerably easier to follow. Here's a simple patch for the RelationGetIndexAttrBitmap() function, as explained in my last post. I don't know if it's any help to you, but since I wrote it I might as well send it up. This applies on top of Noah's rebased patch. Got it, thanks. I did some tests and it seems to work, although I also hit the same visibility bug as Noah. Yeah, that bug is fixed with the attached, though I am rethinking this bit. I am thinking that the statute of limitations has expired on this patch, and that we should mark it Returned with Feedback and continue working on it for 9.2. I know it's a valuable feature, but I think we're out of time. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Feb 15, 2011, at 1:15 PM, Robert Haas wrote: Yeah, that bug is fixed with the attached, though I am rethinking this bit. I am thinking that the statute of limitations has expired on this patch, and that we should mark it Returned with Feedback and continue working on it for 9.2. I know it's a valuable feature, but I think we're out of time. How is such a determination made, exactly? Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Robert Haas's message of mar feb 15 18:15:38 -0300 2011: I am thinking that the statute of limitations has expired on this patch, and that we should mark it Returned with Feedback and continue working on it for 9.2. I know it's a valuable feature, but I think we're out of time. Okay, I've marked it as such in the commitfest app. It'll be in 9.2's first commitfest. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
How is such a determination made, exactly? It's Feb 15th, and portions of the patch need a rework according to the author. I'm with Robert on this one. -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Noah Misch's message of vie feb 11 04:13:22 -0300 2011: I observe visibility breakage with this test case: [ ... ] The problem seems to be that funny t_cid (2249). Tracing through heap_update, the new code is not setting t_cid during this test case. So I can fix this problem by simply adding a call to HeapTupleHeaderSetCmin when the stuff about ComboCid does not hold, but seeing that screenful plus the subsequent call to HeapTupleHeaderAdjustCmax feels wrong. I think this needs to be rethought ... -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Fri, Feb 11, 2011 at 09:13, Noah Misch n...@leadboat.com wrote:
The patch had a trivial conflict in planner.c, plus plenty of offsets. I've
attached the rebased patch that I used for review. For anyone following
along,
all the interesting hunks touch heapam.c; the rest is largely mechanical. A
diff -w patch is also considerably easier to follow.
Here's a simple patch for the RelationGetIndexAttrBitmap() function,
as explained in my last post. I don't know if it's any help to you,
but since I wrote it I might as well send it up. This applies on top
of Noah's rebased patch.
I did some tests and it seems to work, although I also hit the same
visibility bug as Noah.
Test case I used:
THREAD A:
create table foo (pk int primary key, ak int);
create unique index on foo (ak) where ak != 0;
create unique index on foo ((-ak));
create table bar (foo_pk int references foo (pk));
insert into foo values(1,1);
begin; insert into bar values(1);
THREAD B:
begin; update foo set ak=2 where ak=1;
Regards,
Marti
From e069cef91c686aa87e220336198267e5a5a2aeac Mon Sep 17 00:00:00 2001
From: Marti Raudsepp ma...@juffo.org
Date: Tue, 15 Feb 2011 00:33:35 +0200
Subject: [PATCH] Only acquire KEY LOCK for colums that can be referenced by foreign keys
Don't consider columns in unique indexes that have expressions or WHERE
predicates.
---
src/backend/utils/cache/relcache.c | 23 +--
src/include/utils/rel.h|2 +-
src/include/utils/relcache.h |2 +-
3 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 4d37e8e..5119288 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3608,7 +3608,8 @@ RelationGetIndexPredicate(Relation relation)
* simple index keys, but attributes used in expressions and partial-index
* predicates.)
*
- * If unique is true, only attributes of unique indexes are considered.
+ * If keyAttrs is true, only attributes that can be referenced by foreign
+ * keys are considered.
*
* Attribute numbers are offset by FirstLowInvalidHeapAttributeNumber so that
* we can include system attributes (e.g., OID) in the bitmap representation.
@@ -3617,7 +3618,7 @@ RelationGetIndexPredicate(Relation relation)
* be bms_free'd when not needed anymore.
*/
Bitmapset *
-RelationGetIndexAttrBitmap(Relation relation, bool unique)
+RelationGetIndexAttrBitmap(Relation relation, bool keyAttrs)
{
Bitmapset *indexattrs;
Bitmapset *uindexattrs;
@@ -3627,7 +3628,7 @@ RelationGetIndexAttrBitmap(Relation relation, bool unique)
/* Quick exit if we already computed the result. */
if (relation-rd_indexattr != NULL)
- return bms_copy(unique ? relation-rd_uindexattr : relation-rd_indexattr);
+ return bms_copy(keyAttrs ? relation-rd_keyattr : relation-rd_indexattr);
/* Fast path if definitely no indexes */
if (!RelationGetForm(relation)-relhasindex)
@@ -3653,12 +3654,18 @@ RelationGetIndexAttrBitmap(Relation relation, bool unique)
Relation indexDesc;
IndexInfo *indexInfo;
int i;
+ bool isKey;
indexDesc = index_open(indexOid, AccessShareLock);
/* Extract index key information from the index's pg_index row */
indexInfo = BuildIndexInfo(indexDesc);
+ /* Can this index be referenced by a foreign key? */
+ isKey = indexInfo-ii_Unique
+indexInfo-ii_Expressions == NIL
+indexInfo-ii_Predicate == NIL;
+
/* Collect simple attribute references */
for (i = 0; i indexInfo-ii_NumIndexAttrs; i++)
{
@@ -3668,7 +3675,7 @@ RelationGetIndexAttrBitmap(Relation relation, bool unique)
{
indexattrs = bms_add_member(indexattrs,
attrnum - FirstLowInvalidHeapAttributeNumber);
-if (indexInfo-ii_Unique)
+if (isKey)
uindexattrs = bms_add_member(uindexattrs,
attrnum - FirstLowInvalidHeapAttributeNumber);
}
@@ -3676,13 +3683,9 @@ RelationGetIndexAttrBitmap(Relation relation, bool unique)
/* Collect all attributes used in expressions, too */
pull_varattnos((Node *) indexInfo-ii_Expressions, indexattrs);
- if (indexInfo-ii_Unique)
- pull_varattnos((Node *) indexInfo-ii_Expressions, uindexattrs);
/* Collect all attributes in the index predicate, too */
pull_varattnos((Node *) indexInfo-ii_Predicate, indexattrs);
- if (indexInfo-ii_Unique)
- pull_varattnos((Node *) indexInfo-ii_Predicate, uindexattrs);
index_close(indexDesc, AccessShareLock);
}
@@ -3692,11 +3695,11 @@ RelationGetIndexAttrBitmap(Relation relation, bool unique)
/* Now save a copy of the bitmap in the relcache entry. */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
relation-rd_indexattr = bms_copy(indexattrs);
- relation-rd_uindexattr = bms_copy(uindexattrs);
+ relation-rd_keyattr = bms_copy(uindexattrs);
MemoryContextSwitchTo(oldcxt);
/* We return our original working copy for caller to play with */
- return unique ? uindexattrs : indexattrs;
+
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Marti Raudsepp's message of lun feb 14 19:39:25 -0300 2011: On Fri, Feb 11, 2011 at 09:13, Noah Misch n...@leadboat.com wrote: The patch had a trivial conflict in planner.c, plus plenty of offsets. I've attached the rebased patch that I used for review. For anyone following along, all the interesting hunks touch heapam.c; the rest is largely mechanical. A diff -w patch is also considerably easier to follow. Here's a simple patch for the RelationGetIndexAttrBitmap() function, as explained in my last post. I don't know if it's any help to you, but since I wrote it I might as well send it up. This applies on top of Noah's rebased patch. Got it, thanks. I did some tests and it seems to work, although I also hit the same visibility bug as Noah. Yeah, that bug is fixed with the attached, though I am rethinking this bit. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support 0001-Fix-visibility-bug-and-poorly-worded-comment.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Noah Misch's message of vie feb 11 04:13:22 -0300 2011: Hello, First, thanks for the very thorough review. On Thu, Jan 13, 2011 at 06:58:09PM -0300, Alvaro Herrera wrote: Incidentally, HeapTupleSatisfiesMVCC has some bits of code like this (not new): /* MultiXacts are currently only allowed to lock tuples */ Assert(tuple-t_infomask HEAP_IS_LOCKED); They're specifically only allowed for SHARE and KEY locks, right? heap_lock_tuple seems to assume as much. Yeah, since FOR UPDATE acquires an exclusive lock on the tuple, you can't have a multixact there. Maybe we can make the assert more specific; I'll have a look. [ test case with funny visibility behavior ] Looking into the visibility bug. I published about this here: http://commandprompt.com/blogs/alvaro_herrera/2010/11/fixing_foreign_key_deadlocks_part_2/ So, as a rough design, 1. Create a new SELECT locking clause. For now, we're calling it SELECT FOR KEY LOCK 2. This will acquire a new type of lock in the tuple, dubbed a keylock. 3. This lock will conflict with DELETE, SELECT FOR UPDATE, and SELECT FOR SHARE. It does not conflict with SELECT FOR SHARE, does it? It doesn't; I think I copied old text there. (I had originally thought that they would conflict, but I had to change that due to implementation restrictions). The odd thing here is the checking of an outside condition to decide whether locks conflict. Normally, to get a different conflict list, we add another lock type. What about this? FOR KEY SHARE conflicts with FOR KEY UPDATE FOR SHARE conflicts with FOR KEY UPDATE, FOR UPDATE FOR UPDATE conflicts with FOR KEY UPDATE, FOR UPDATE, FOR SHARE FOR KEY UPDATE conflicts with FOR KEY UPDATE, FOR UPDATE, FOR SHARE, FOR KEY SHARE Hmm, let me see about this. 3. The original tuple needs to be marked with the Cmax of the locking command, to prevent it from being seen in the same transaction. Could you elaborate on this requirement? Consider an open cursor with a snapshot prior to the lock. If we leave the old tuple as is, the cursor would see that old tuple as visible. But the locked copy of the tuple is also visible, because the Cmax is just a locker, not an updater. 4. A non-conflicting update to the tuple must carry forward some fields from the original tuple into the updated copy. Those include Xmax, XMAX_IS_MULTI, XMAX_KEY_LOCK, and the CommandId and COMBO_CID flag. HeapTupleHeaderGetCmax() has this assertion: /* We do not store cmax when locking a tuple */ Assert(!(tup-t_infomask (HEAP_MOVED | HEAP_IS_LOCKED))); Assuming that assertion is still valid, there will never be a HEAP_COMBOCID flag to copy. Right? Hmm, I think the assert is wrong, but I'm still paging in the details of the patch after being away from it for so long. Let me think more about it. [ Lots more stuff ] I'll give careful consideration to all this. Thanks again for the detailed review. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Fri, Feb 11, 2011 at 02:15:20PM -0300, Alvaro Herrera wrote: Excerpts from Noah Misch's message of vie feb 11 04:13:22 -0300 2011: On Thu, Jan 13, 2011 at 06:58:09PM -0300, Alvaro Herrera wrote: 3. The original tuple needs to be marked with the Cmax of the locking command, to prevent it from being seen in the same transaction. Could you elaborate on this requirement? Consider an open cursor with a snapshot prior to the lock. If we leave the old tuple as is, the cursor would see that old tuple as visible. But the locked copy of the tuple is also visible, because the Cmax is just a locker, not an updater. Thanks. Today, a lock operation leaves t_cid unchanged, and an update fills its own cid into Cmax of the old tuple and Cmin of the new tuple. So, the cursor would only see the old tuple. What will make that no longer sufficient? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Thu, Jan 13, 2011 at 23:58, Alvaro Herrera alvhe...@commandprompt.com wrote: It goes like this: instead of acquiring a shared lock on the involved tuple, we only acquire a key lock, that is, something that prevents the tuple from going away entirely but not from updating fields that are not covered by any unique index. As discussed, this is still more restrictive than necessary (we could lock only those columns that are involved in the foreign key being checked), but that has all sorts of implementation level problems, so we settled for this, which is still much better than the current state of affairs. Seems to me that you can go a bit further without much trouble, if you only consider indexes that *can* be referenced by foreign keys -- indexes that don't have expressions or predicates. I frequently create unique indexes on (lower(name)) where I want case-insensitive unique indexes, or use predicates like WHERE deleted=false to allow duplicates after deleting the old item. So, instead of: if (indexInfo-ii_Unique) you can write: if (indexInfo-ii_Unique indexInfo-ii_Expressions == NIL indexInfo-ii_Predicate == NIL) This would slightly simplify RelationGetIndexAttrBitmap() because you no longer have to worry about including columns that are part of index expressions/predicates. I guess rd_uindexattr should be renamed to something like rd_keyindexattr or rd_keyattr. Is this worthwhile? I can write and submit a patch if it sounds good. Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Hi, This is a first level of review for the patch. I finally didn't get as much time as I hoped I would, so couldn't get familiar with the locking internals and machinery… as a result, I can't much comment on the code. The patch applies cleanly (patch moves one hunk all by itself) and compiles with no warning. It includes no docs, and I think it will be required to document the user visible SELECT … FOR KEY LOCK OF x new feature. Code wise, very few comments here. It looks like the new code had been there from the beginning by the reading of the patch. I only have one question about a variable naming: ! COPY_SCALAR_FIELD(forUpdate); ! COPY_SCALAR_FIELD(strength); forUpdate used to be a boolean, strength is now one of LCS_FORUPDATE, LCS_FORSHARE or LCS_FORKEYLOCK. I wonder if that's a fortunate naming here, but IANANS (I Am Not A Native Speaker). Alvaro Herrera alvhe...@commandprompt.com writes: As previously commented, here's a proposal with patch to turn foreign key checks into something less intrusive. The basic idea, as proposed by Simon Riggs, was discussed in a previous pgsql-hackers thread here: http://archives.postgresql.org/message-id/aanlktimo9xvcezfibr-ut3kvndkjm2vxh+t8kamwj...@mail.gmail.com This link here provides a test case that will issue a deadlock, and Something else that might be of interest: the patch as presented here does NOT solve the deadlock problem originally presented by Joel Indeed, that's the first thing I tried… I'm not sure about why fixing the deadlock issue wouldn't be in this patch scope? The thing that I'm able to confirm by running this test case is that the RI trigger check is done with the new code from the patch: CONTEXT: SQL statement SELECT 1 FROM ONLY public.a x WHERE aid OPERATOR(pg_catalog.=) $1 FOR KEY LOCK OF x Sorry for not posting more tests yet, but seeing how late I am to find the time for the first level review I figured I might as well send that already. I will try some other test cases, but sure enough, that should be part of the user level documentation… Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Sat, Jan 22, 2011 at 4:25 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Hi, This is a first level of review for the patch. I finally didn't get as much time as I hoped I would, so couldn't get familiar with the locking internals and machinery… as a result, I can't much comment on the code. The patch applies cleanly (patch moves one hunk all by itself) and compiles with no warning. It includes no docs, and I think it will be required to document the user visible SELECT … FOR KEY LOCK OF x new feature. I feel like this should be called KEY SHARE rather than KEY LOCK. It's essentially a weaker version of the SHARE lock we have now, but that's not clear from the name. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Jan 13, 2011, at 1:58 PM, Alvaro Herrera wrote: Something else that might be of interest: the patch as presented here does NOT solve the deadlock problem originally presented by Joel Jacobson. It does solve the second, simpler example I presented in my blog article referenced above, however. I need to have a closer look at that problem to figure out if we could fix the deadlock too. Sounds like a big win already. Should this be considered a WIP patch, though, if you still plan to look at Joel's deadlock example? Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
On Fri, Jan 14, 2011 at 1:00 PM, David E. Wheeler da...@kineticode.com wrote: On Jan 13, 2011, at 1:58 PM, Alvaro Herrera wrote: Something else that might be of interest: the patch as presented here does NOT solve the deadlock problem originally presented by Joel Jacobson. It does solve the second, simpler example I presented in my blog article referenced above, however. I need to have a closer look at that problem to figure out if we could fix the deadlock too. Sounds like a big win already. Should this be considered a WIP patch, though, if you still plan to look at Joel's deadlock example? Alvaro, are you planning to add this to the CF? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from David E. Wheeler's message of vie ene 14 15:00:48 -0300 2011: On Jan 13, 2011, at 1:58 PM, Alvaro Herrera wrote: Something else that might be of interest: the patch as presented here does NOT solve the deadlock problem originally presented by Joel Jacobson. It does solve the second, simpler example I presented in my blog article referenced above, however. I need to have a closer look at that problem to figure out if we could fix the deadlock too. Sounds like a big win already. Should this be considered a WIP patch, though, if you still plan to look at Joel's deadlock example? Not necessarily -- we can implement that as a later refinement/improvement. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FOR KEY LOCK foreign keys
Excerpts from Robert Haas's message of vie ene 14 15:08:27 -0300 2011: On Fri, Jan 14, 2011 at 1:00 PM, David E. Wheeler da...@kineticode.com wrote: On Jan 13, 2011, at 1:58 PM, Alvaro Herrera wrote: Something else that might be of interest: the patch as presented here does NOT solve the deadlock problem originally presented by Joel Jacobson. It does solve the second, simpler example I presented in my blog article referenced above, however. I need to have a closer look at that problem to figure out if we could fix the deadlock too. Sounds like a big win already. Should this be considered a WIP patch, though, if you still plan to look at Joel's deadlock example? Alvaro, are you planning to add this to the CF? Eh, yes. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
