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.
(If the UPDATE commits, then the serialization error is correctly
reported.)

When running a query in READ COMMITTED mode, what happens is that the
locking is aborted by the HeapTupleUpdated return value, then
EvalPlanQual fetches the newest version of the tuple, which is then the
only version that gets locked.  (The second time the tuple is checked
there is no misbehavior on the committed lock-only, because it's not
checked by the code that traverses update chains; so no bug.) Only the
newest version of the tuple is locked, not older ones, but this is
harmless.

The isolation test added by this commit illustrates the desired
behavior, including the proper serialization errors that get thrown.

Backpatch to 9.3.

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 64174b6ea251f08f22122f15cc5eeb10f04bb5ca..1a0dd2139a5513bdea3f84000b5c73273c931d95 100644 (file)
--- 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 (file)
index 0000000..38466bf
--- /dev/null
+++ b/src/test/isolation/expected/multixact-no-forget.out
@@ -0,0 +1,130 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1_show s1_commit s2_commit
+step s1_show: SELECT current_setting('default_transaction_isolation') <> 'read committed';
+?column?       
+
+f              
+step s1_commit: COMMIT;
+step s2_commit: COMMIT;
+
+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 NO KEY UPDATE;
+value          
+
+1              
+step s1_commit: COMMIT;
+
+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 NO KEY UPDATE;
+value          
+
+2              
+step s1_commit: COMMIT;
+
+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 NO KEY 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 UPDATE; <waiting ...>
+step s1_commit: COMMIT;
+step s3_forupd: <... completed>
+value          
+
+1              
+
+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 UPDATE; <waiting ...>
+step s1_commit: COMMIT;
+step s3_forupd: <... completed>
+value          
+
+2              
+
+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 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 (file)
index 0000000..1c37afe
--- /dev/null
+++ b/src/test/isolation/expected/multixact-no-forget_1.out
@@ -0,0 +1,126 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1_show s1_commit s2_commit
+step s1_show: SELECT current_setting('default_transaction_isolation') <> 'read committed';
+?column?       
+
+t              
+step s1_commit: COMMIT;
+step s2_commit: COMMIT;
+
+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 NO KEY UPDATE;
+value          
+
+1              
+step s1_commit: COMMIT;
+
+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 NO KEY UPDATE;
+value          
+
+2              
+step s1_commit: COMMIT;
+
+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 NO KEY 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 UPDATE; <waiting ...>
+step s1_commit: COMMIT;
+step s3_forupd: <... completed>
+value          
+
+1              
+
+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 UPDATE; <waiting ...>
+step s1_commit: COMMIT;
+step s3_forupd: <... completed>
+value          
+
+2              
+
+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 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/isolation_schedule b/src/test/isolation/isolation_schedule
index 329dbf12fa5586d2d41109b2bca64d911b5b7d73..dd4b404183674173bca44e76f7086d90787fc164 100644 (file)
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -20,5 +20,6 @@ test: delete-abort-savept
 test: delete-abort-savept-2
 test: aborted-keyrevoke
 test: multixact-no-deadlock
+test: multixact-no-forget
 test: drop-index-concurrently-1
 test: timeouts

diff --git a/src/test/isolation/specs/multixact-no-forget.spec b/src/test/isolation/specs/multixact-no-forget.spec
new file mode 100644 (file)
index 0000000..7fb93c1
--- /dev/null
+++ b/src/test/isolation/specs/multixact-no-forget.spec
@@ -0,0 +1,44 @@
+# 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_show" { SELECT current_setting('default_transaction_isolation') <> 'read committed'; }
+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 NO KEY UPDATE; }
+step "s3_forupd"       { SELECT * FROM dont_forget FOR UPDATE; }
+
+permutation "s1_show" "s1_commit" "s2_commit"
+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"