Andres Freund wrote:
> On 2013-12-03 19:55:40 -0300, Alvaro Herrera wrote:
> > I added a new isolation spec to test this specific case, and noticed
> > something that seems curious to me when that test is run in REPEATABLE
> > READ mode: when the UPDATE is aborted, the concurrent FOR UPDATE gets a
> > "can't serialize due to concurrent update", but when the UPDATE is
> > committed, FOR UPDATE works fine. Shouldn't it happen pretty much
> > exactly the other way around?
>
> That's 247c76a989097f1b4ab6fae898f24e75aa27fc1b . Specifically the
> DidCommit() branch in test_lockmode_for_conflict(). You forgot something
> akin to
> /* locker has finished, all good to go */
> if (!ISUPDATE_from_mxstatus(status))
> return HeapTupleMayBeUpdated;
So I did. Here are two patches, one to fix this issue, and the other to
fix the issue above. I intend to apply these two to 9.3 and master, and
then apply your freeze fix on top (which I'm cleaning up a bit -- will
resend later.)
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 48b6f5eead73a41880dd311b7a55d2b88794ca3d Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 4 Dec 2013 17:20:14 -0300
Subject: [PATCH 1/4] Avoid resetting Xmax when it's a multi with an aborted
update
HeapTupleSatisfiesUpdate can very easily "forget" tuple locks while
checking the contents of a multixact and finding it contains an aborted
update, by setting the HEAP_XMAX_INVALID bit. This would lead to
concurrent transactions not noticing any previous locks held by
transactions that might still be running, and thus being able to acquire
subsequent locks they wouldn't be normally able to acquire.
This bug was introduced in commit 1ce150b7bb; backpatch this fix to 9.3,
like that commit.
This change reverts the change to the delete-abort-savept isolation test
in 1ce150b7bb, because that behavior change was caused by this bug.
Noticed by Andres Freund while investigating a different issue reported
by Noah Misch.
---
src/backend/utils/time/tqual.c | 21 ++++++++++++++++----
.../isolation/expected/delete-abort-savept.out | 13 +++++-------
2 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 4d63b1c..f787f2c 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -789,13 +789,26 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,
if (TransactionIdDidCommit(xmax))
return HeapTupleUpdated;
- /* no member, even just a locker, alive anymore */
+ /*
+ * By here, the update in the Xmax is either aborted or crashed, but
+ * what about the other members?
+ */
+
if (!MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
+ {
+ /*
+ * There's no member, even just a locker, alive anymore, so we can
+ * mark the Xmax as invalid.
+ */
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
InvalidTransactionId);
-
- /* it must have aborted or crashed */
- return HeapTupleMayBeUpdated;
+ return HeapTupleMayBeUpdated;
+ }
+ else
+ {
+ /* There are lockers running */
+ return HeapTupleBeingUpdated;
+ }
}
if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmax(tuple)))
diff --git a/src/test/isolation/expected/delete-abort-savept.out b/src/test/isolation/expected/delete-abort-savept.out
index 5b8c444..3420cf4 100644
--- a/src/test/isolation/expected/delete-abort-savept.out
+++ b/src/test/isolation/expected/delete-abort-savept.out
@@ -23,11 +23,12 @@ key value
step s1svp: SAVEPOINT f;
step s1d: DELETE FROM foo;
step s1r: ROLLBACK TO f;
-step s2l: SELECT * FROM foo FOR UPDATE;
+step s2l: SELECT * FROM foo FOR UPDATE; <waiting ...>
+step s1c: COMMIT;
+step s2l: <... completed>
key value
1 1
-step s1c: COMMIT;
step s2c: COMMIT;
starting permutation: s1l s1svp s1d s1r s2l s2c s1c
@@ -38,12 +39,8 @@ key value
step s1svp: SAVEPOINT f;
step s1d: DELETE FROM foo;
step s1r: ROLLBACK TO f;
-step s2l: SELECT * FROM foo FOR UPDATE;
-key value
-
-1 1
-step s2c: COMMIT;
-step s1c: COMMIT;
+step s2l: SELECT * FROM foo FOR UPDATE; <waiting ...>
+invalid permutation detected
starting permutation: s1l s1svp s1d s2l s1r s1c s2c
step s1l: SELECT * FROM foo FOR KEY SHARE;
--
1.7.10.4
>From fbe641eeb7eb535c1cac422e3f5962928e3b0362 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 4 Dec 2013 17:26:07 -0300
Subject: [PATCH 2/4] Fix improper abort during update chain locking
In 247c76a98909, I added some code to do fine-grained checking of
MultiXact status of locking/updating transactions when traversing an
update chain. There was a thinko in that patch which would have the
traversing abort, that is return HeapTupleUpdated, when the other
transaction is a committed lock-only. In this case we should ignore it
and return success instead. Of course, in the case where there is a
committed update, HeapTupleUpdated is the correct return value.
A user-visible symptom of this bug is that in REPEATABLE READ and
SERIALIZABLE transaction isolation modes spurious serializability errors
can occur:
ERROR: could not serialize access due to concurrent update
In order for this to happen, there needs to be a tuple that's key-share-
locked and also updated, and the update must abort; a subsequent
transaction trying to acquire a new lock on that tuple would abort with
the above error. The reason is that the initial FOR KEY SHARE is seen
as committed by the new locking transaction, which triggers this bug.
The isolation test added by this commit illustrates the desired
behavior.
Backpatch to 9.3.
---
src/backend/access/heap/heapam.c | 19 ++-
.../isolation/expected/multixact-no-forget.out | 122 ++++++++++++++++++++
.../isolation/expected/multixact-no-forget_1.out | 118 +++++++++++++++++++
src/test/isolation/specs/multixact-no-forget.spec | 42 +++++++
4 files changed, 298 insertions(+), 3 deletions(-)
create mode 100644 src/test/isolation/expected/multixact-no-forget.out
create mode 100644 src/test/isolation/expected/multixact-no-forget_1.out
create mode 100644 src/test/isolation/specs/multixact-no-forget.spec
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 64174b6..1a0dd21 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4842,10 +4842,23 @@ test_lockmode_for_conflict(MultiXactStatus status, TransactionId xid,
else if (TransactionIdDidCommit(xid))
{
/*
- * If the updating transaction committed, what we do depends on whether
- * the lock modes conflict: if they do, then we must report error to
- * caller. But if they don't, we can fall through to lock it.
+ * The other transaction committed. If it was only a locker, then the
+ * lock is completely gone now and we can return success; but if it
+ * was an update, then what we do depends on whether the two lock
+ * modes conflict. If they conflict, then we must report error to
+ * caller. But if they don't, we can fall through to allow the current
+ * transaction to lock the tuple.
+ *
+ * Note: the reason we worry about ISUPDATE here is because as soon as
+ * a transaction ends, all its locks are gone and meaningless, and
+ * thus we can ignore them; whereas its updates persist. In the
+ * TransactionIdIsInProgress case, above, we don't need to check
+ * because we know the lock is still "alive" and thus a conflict needs
+ * always be checked.
*/
+ if (!ISUPDATE_from_mxstatus(status))
+ return HeapTupleMayBeUpdated;
+
if (DoLockModesConflict(LOCKMODE_from_mxstatus(status),
LOCKMODE_from_mxstatus(wantedstatus)))
/* bummer */
diff --git a/src/test/isolation/expected/multixact-no-forget.out b/src/test/isolation/expected/multixact-no-forget.out
new file mode 100644
index 0000000..20f01a6
--- /dev/null
+++ b/src/test/isolation/expected/multixact-no-forget.out
@@ -0,0 +1,122 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1_lock s2_update s2_abort s3_forkeyshr s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_abort: ROLLBACK;
+step s3_forkeyshr: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s1_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s2_commit s3_forkeyshr s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_commit: COMMIT;
+step s3_forkeyshr: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+2
+step s1_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s1_commit s3_forkeyshr s2_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s1_commit: COMMIT;
+step s3_forkeyshr: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s2_abort s3_fornokeyupd s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_abort: ROLLBACK;
+step s3_fornokeyupd: SELECT * FROM dont_forget FOR UPDATE; <waiting ...>
+step s1_commit: COMMIT;
+step s3_fornokeyupd: <... completed>
+value
+
+1
+
+starting permutation: s1_lock s2_update s2_commit s3_fornokeyupd s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_commit: COMMIT;
+step s3_fornokeyupd: SELECT * FROM dont_forget FOR UPDATE; <waiting ...>
+step s1_commit: COMMIT;
+step s3_fornokeyupd: <... completed>
+value
+
+2
+
+starting permutation: s1_lock s2_update s1_commit s3_fornokeyupd s2_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s1_commit: COMMIT;
+step s3_fornokeyupd: SELECT * FROM dont_forget FOR UPDATE; <waiting ...>
+step s2_commit: COMMIT;
+step s3_fornokeyupd: <... completed>
+value
+
+2
+
+starting permutation: s1_lock s2_update s2_abort s3_forupd s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_abort: ROLLBACK;
+step s3_forupd: SELECT * FROM dont_forget FOR NO KEY UPDATE;
+value
+
+1
+step s1_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s2_commit s3_forupd s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_commit: COMMIT;
+step s3_forupd: SELECT * FROM dont_forget FOR NO KEY UPDATE;
+value
+
+2
+step s1_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s1_commit s3_forupd s2_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s1_commit: COMMIT;
+step s3_forupd: SELECT * FROM dont_forget FOR NO KEY UPDATE; <waiting ...>
+step s2_commit: COMMIT;
+step s3_forupd: <... completed>
+value
+
+2
diff --git a/src/test/isolation/expected/multixact-no-forget_1.out b/src/test/isolation/expected/multixact-no-forget_1.out
new file mode 100644
index 0000000..7c0adcf
--- /dev/null
+++ b/src/test/isolation/expected/multixact-no-forget_1.out
@@ -0,0 +1,118 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1_lock s2_update s2_abort s3_forkeyshr s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_abort: ROLLBACK;
+step s3_forkeyshr: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s1_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s2_commit s3_forkeyshr s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_commit: COMMIT;
+step s3_forkeyshr: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+2
+step s1_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s1_commit s3_forkeyshr s2_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s1_commit: COMMIT;
+step s3_forkeyshr: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s2_abort s3_fornokeyupd s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_abort: ROLLBACK;
+step s3_fornokeyupd: SELECT * FROM dont_forget FOR UPDATE; <waiting ...>
+step s1_commit: COMMIT;
+step s3_fornokeyupd: <... completed>
+value
+
+1
+
+starting permutation: s1_lock s2_update s2_commit s3_fornokeyupd s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_commit: COMMIT;
+step s3_fornokeyupd: SELECT * FROM dont_forget FOR UPDATE; <waiting ...>
+step s1_commit: COMMIT;
+step s3_fornokeyupd: <... completed>
+value
+
+2
+
+starting permutation: s1_lock s2_update s1_commit s3_fornokeyupd s2_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s1_commit: COMMIT;
+step s3_fornokeyupd: SELECT * FROM dont_forget FOR UPDATE; <waiting ...>
+step s2_commit: COMMIT;
+step s3_fornokeyupd: <... completed>
+error in steps s2_commit s3_fornokeyupd: ERROR: could not serialize access due to concurrent update
+
+starting permutation: s1_lock s2_update s2_abort s3_forupd s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_abort: ROLLBACK;
+step s3_forupd: SELECT * FROM dont_forget FOR NO KEY UPDATE;
+value
+
+1
+step s1_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s2_commit s3_forupd s1_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s2_commit: COMMIT;
+step s3_forupd: SELECT * FROM dont_forget FOR NO KEY UPDATE;
+value
+
+2
+step s1_commit: COMMIT;
+
+starting permutation: s1_lock s2_update s1_commit s3_forupd s2_commit
+step s1_lock: SELECT * FROM dont_forget FOR KEY SHARE;
+value
+
+1
+step s2_update: UPDATE dont_forget SET value = 2;
+step s1_commit: COMMIT;
+step s3_forupd: SELECT * FROM dont_forget FOR NO KEY UPDATE; <waiting ...>
+step s2_commit: COMMIT;
+step s3_forupd: <... completed>
+error in steps s2_commit s3_forupd: ERROR: could not serialize access due to concurrent update
diff --git a/src/test/isolation/specs/multixact-no-forget.spec b/src/test/isolation/specs/multixact-no-forget.spec
new file mode 100644
index 0000000..c1cfb7c
--- /dev/null
+++ b/src/test/isolation/specs/multixact-no-forget.spec
@@ -0,0 +1,42 @@
+# If transaction A holds a lock, and transaction B does an update,
+# make sure we don't forget the lock if B aborts.
+setup
+{
+ CREATE TABLE dont_forget (
+ value int
+ );
+
+ INSERT INTO dont_forget VALUES (1);
+}
+
+teardown
+{
+ DROP TABLE dont_forget;
+}
+
+session "s1"
+setup { BEGIN; }
+step "s1_lock" { SELECT * FROM dont_forget FOR KEY SHARE; }
+step "s1_commit" { COMMIT; }
+
+session "s2"
+setup { BEGIN; }
+step "s2_update" { UPDATE dont_forget SET value = 2; }
+step "s2_abort" { ROLLBACK; }
+step "s2_commit" { COMMIT; }
+
+session "s3"
+# try cases with both a non-conflicting lock with s1's and a conflicting one
+step "s3_forkeyshr" { SELECT * FROM dont_forget FOR KEY SHARE; }
+step "s3_fornokeyupd" { SELECT * FROM dont_forget FOR UPDATE; }
+step "s3_forupd" { SELECT * FROM dont_forget FOR NO KEY UPDATE; }
+
+permutation "s1_lock" "s2_update" "s2_abort" "s3_forkeyshr" "s1_commit"
+permutation "s1_lock" "s2_update" "s2_commit" "s3_forkeyshr" "s1_commit"
+permutation "s1_lock" "s2_update" "s1_commit" "s3_forkeyshr" "s2_commit"
+permutation "s1_lock" "s2_update" "s2_abort" "s3_fornokeyupd" "s1_commit"
+permutation "s1_lock" "s2_update" "s2_commit" "s3_fornokeyupd" "s1_commit"
+permutation "s1_lock" "s2_update" "s1_commit" "s3_fornokeyupd" "s2_commit"
+permutation "s1_lock" "s2_update" "s2_abort" "s3_forupd" "s1_commit"
+permutation "s1_lock" "s2_update" "s2_commit" "s3_forupd" "s1_commit"
+permutation "s1_lock" "s2_update" "s1_commit" "s3_forupd" "s2_commit"
--
1.7.10.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
