Avoid serializability errors when locking a tuple with a committed update

When key-share locking a tuple that has been not-key-updated, and the
update is a committed transaction, in some cases we raised
serializability errors:
    ERROR:  could not serialize access due to concurrent update

Because the key-share doesn't conflict with the update, the error is
unnecessary and inconsistent with the case that the update hasn't
committed yet.  This causes problems for some usage patterns, even if it
can be claimed that it's sufficient to retry the aborted transaction:
given a steady stream of updating transactions and a long locking
transaction, the long transaction can be starved indefinitely despite
multiple retries.

To fix, we recognize that HeapTupleSatisfiesUpdate can return
HeapTupleUpdated when an updating transaction has committed, and that we
need to deal with that case exactly as if it were a non-committed
update: verify whether the two operations conflict, and if not, carry on
normally.  If they do conflict, however, there is a difference: in the
HeapTupleBeingUpdated case we can just sleep until the concurrent
transaction is gone, while in the HeapTupleUpdated case this is not
possible and we must raise an error instead.

Per trouble report from Olivier Dony.

In addition to a couple of test cases that verify the changed behavior,
I added a test case to verify the behavior that remains unchanged,
namely that errors are raised when a update that modifies the key is
used.  That must still generate serializability errors.  One
pre-existing test case changes behavior; per discussion, the new
behavior is actually the desired one.

Discussion: https://www.postgresql.org/message-id/560AA479.4080807@odoo.com
  https://www.postgresql.org/message-id/20151014164844.3019.25750@wrigleys.postgresql.org

Backpatch to 9.3, where the problem appeared.

src/backend/access/heap/heapam.c

@@ -4540,7 +4540,7 @@ l3:
 		 */
 		return HeapTupleInvisible;
 	}
-	else if (result == HeapTupleBeingUpdated)
+	else if (result == HeapTupleBeingUpdated || result == HeapTupleUpdated)
 	{
 		TransactionId xwait;
 		uint16		infomask;
@@ -4800,12 +4800,22 @@ l3:
 		}
 
 		/*
+		 * Time to sleep on the other transaction/multixact, if necessary.
+		 *
+		 * If the other transaction is an update that's already committed,
+		 * then sleeping cannot possibly do any good: if we're required to
+		 * sleep, get out to raise an error instead.
+		 *
 		 * By here, we either have already acquired the buffer exclusive lock,
 		 * or we must wait for the locking transaction or multixact; so below
 		 * we ensure that we grab buffer lock after the sleep.
 		 */
-
-		if (require_sleep)
+		if (require_sleep && result == HeapTupleUpdated)
+		{
+			LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE);
+			goto failed;
+		}
+		else if (require_sleep)
 		{
 			/*
 			 * Acquire tuple lock to establish our priority for the tuple, or

src/test/isolation/expected/lock-update-delete.out

@@ -156,7 +156,7 @@ t
 step s1l: <... completed>
 key            value          
 
-1              2              
+1              1              
 
 starting permutation: s2b s1l s2u s2_blocker1 s2r s2_unlock
 pg_advisory_lock

src/test/isolation/expected/update-locked-tuple.out

+Parsed test spec with 2 sessions
+
+starting permutation: s1b s2b s2u s2c s1u1 s1u2 s1c
+step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2u: UPDATE users SET sometime = '1830-10-04' WHERE id = 1;
+step s2c: COMMIT;
+step s1u1: UPDATE orders SET name = 'order of olivier (2)', user_id = 1 WHERE id = 1;
+step s1u2: UPDATE orders SET name = 'order of olivier (3)', user_id = 1 WHERE id = 1;
+step s1c: COMMIT;
+
+starting permutation: s1b s2b s2u s1u1 s2c s1u2 s1c
+step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2u: UPDATE users SET sometime = '1830-10-04' WHERE id = 1;
+step s1u1: UPDATE orders SET name = 'order of olivier (2)', user_id = 1 WHERE id = 1;
+step s2c: COMMIT;
+step s1u2: UPDATE orders SET name = 'order of olivier (3)', user_id = 1 WHERE id = 1;
+step s1c: COMMIT;
+
+starting permutation: s1b s2b s1u1 s2u s2c s1u2 s1c
+step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s1u1: UPDATE orders SET name = 'order of olivier (2)', user_id = 1 WHERE id = 1;
+step s2u: UPDATE users SET sometime = '1830-10-04' WHERE id = 1;
+step s2c: COMMIT;
+step s1u2: UPDATE orders SET name = 'order of olivier (3)', user_id = 1 WHERE id = 1;
+step s1c: COMMIT;
+
+starting permutation: s1b s1u1 s2b s2u s2c s1u2 s1c
+step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s1u1: UPDATE orders SET name = 'order of olivier (2)', user_id = 1 WHERE id = 1;
+step s2b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2u: UPDATE users SET sometime = '1830-10-04' WHERE id = 1;
+step s2c: COMMIT;
+step s1u2: UPDATE orders SET name = 'order of olivier (3)', user_id = 1 WHERE id = 1;
+step s1c: COMMIT;
+
+starting permutation: s1b s1u1 s2b s1u2 s2u s2c s1c
+step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s1u1: UPDATE orders SET name = 'order of olivier (2)', user_id = 1 WHERE id = 1;
+step s2b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s1u2: UPDATE orders SET name = 'order of olivier (3)', user_id = 1 WHERE id = 1;
+step s2u: UPDATE users SET sometime = '1830-10-04' WHERE id = 1;
+step s2c: COMMIT;
+step s1c: COMMIT;
+
+starting permutation: s1b s1u1 s1u2 s2b s2u s2c s1c
+step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s1u1: UPDATE orders SET name = 'order of olivier (2)', user_id = 1 WHERE id = 1;
+step s1u2: UPDATE orders SET name = 'order of olivier (3)', user_id = 1 WHERE id = 1;
+step s2b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2u: UPDATE users SET sometime = '1830-10-04' WHERE id = 1;
+step s2c: COMMIT;
+step s1c: COMMIT;

src/test/isolation/isolation_schedule

@@ -33,6 +33,9 @@ test: delete-abort-savept-2
 test: aborted-keyrevoke
 test: multixact-no-deadlock
 test: multixact-no-forget
+test: lock-committed-update
+test: lock-committed-keyupdate
+test: update-locked-tuple
 test: propagate-lock-delete
 test: tuplelock-conflict
 test: tuplelock-update

src/test/isolation/specs/lock-committed-keyupdate.spec

+# Test locking of a tuple with a committed key-update.  In this case,
+# the update conflicts with the lock, so failures are expected, except
+# in READ COMMITTED isolation mode.
+#
+# Some of the permutations are commented out that work fine in the
+# lock-committed-update test, because in this case the update blocks.
+
+setup
+{
+    DROP TABLE IF EXISTS lcku_table;
+    CREATE TABLE lcku_table (id INTEGER PRIMARY KEY, value TEXT);
+    INSERT INTO lcku_table VALUES (1, 'one');
+    INSERT INTO lcku_table VALUES (3, 'two');
+}
+
+teardown
+{
+    DROP TABLE lcku_table;
+}
+
+session "s1"
+step "s1b"    { BEGIN; }
+step "s1l"    { SELECT pg_advisory_lock(578902068); }
+step "s1u"    { UPDATE lcku_table SET id = 2 WHERE id = 3; }
+step "s1hint" { SELECT * FROM lcku_table; }
+step "s1ul"   { SELECT pg_advisory_unlock(578902068); }
+step "s1c"    { COMMIT; }
+teardown      { SELECT pg_advisory_unlock_all(); }
+
+session "s2"
+step "s2b1"    { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step "s2b2"    { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step "s2b3"    { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step "s2l"    { SELECT * FROM lcku_table WHERE pg_advisory_lock(578902068) IS NOT NULL FOR KEY SHARE; }
+step "s2c"    { COMMIT; }
+teardown      { SELECT pg_advisory_unlock_all(); }
+
+permutation "s1b" "s2b1" "s1l" "s2l"        "s1u" "s1c"          "s1ul" "s2c"
+permutation "s1b" "s2b1" "s1l" "s1u"        "s2l" "s1c"          "s1ul" "s2c"
+#permutation "s1b" "s2b1" "s1l" "s2l" "s1ul" "s1u" "s1c"                 "s2c"
+permutation "s1b" "s2b1" "s1l" "s1u" "s1ul" "s2l" "s1c"                 "s2c"
+
+permutation "s1b" "s2b1" "s1l" "s2l"        "s1u" "s1c" "s1hint" "s1ul" "s2c"
+permutation "s1b" "s2b1" "s1l" "s1u"        "s2l" "s1c" "s1hint" "s1ul" "s2c"
+#permutation "s1b" "s2b1" "s1l" "s2l" "s1ul" "s1u" "s1c" "s1hint"        "s2c"
+permutation "s1b" "s2b1" "s1l" "s1u" "s1ul" "s2l" "s1c" "s1hint"        "s2c"
+
+permutation "s1b" "s2b2" "s1l" "s2l"        "s1u" "s1c"          "s1ul" "s2c"
+permutation "s1b" "s2b2" "s1l" "s1u"        "s2l" "s1c"          "s1ul" "s2c"
+#permutation "s1b" "s2b2" "s1l" "s2l" "s1ul" "s1u" "s1c"                 "s2c"
+permutation "s1b" "s2b2" "s1l" "s1u" "s1ul" "s2l" "s1c"                 "s2c"
+
+permutation "s1b" "s2b2" "s1l" "s2l"        "s1u" "s1c" "s1hint" "s1ul" "s2c"
+permutation "s1b" "s2b2" "s1l" "s1u"        "s2l" "s1c" "s1hint" "s1ul" "s2c"
+#permutation "s1b" "s2b2" "s1l" "s2l" "s1ul" "s1u" "s1c" "s1hint"        "s2c"
+permutation "s1b" "s2b2" "s1l" "s1u" "s1ul" "s2l" "s1c" "s1hint"        "s2c"
+
+permutation "s1b" "s2b3" "s1l" "s2l"        "s1u" "s1c"          "s1ul" "s2c"
+permutation "s1b" "s2b3" "s1l" "s1u"        "s2l" "s1c"          "s1ul" "s2c"
+#permutation "s1b" "s2b3" "s1l" "s2l" "s1ul" "s1u" "s1c"                 "s2c"
+permutation "s1b" "s2b3" "s1l" "s1u" "s1ul" "s2l" "s1c"                 "s2c"
+
+permutation "s1b" "s2b3" "s1l" "s2l"        "s1u" "s1c" "s1hint" "s1ul" "s2c"
+permutation "s1b" "s2b3" "s1l" "s1u"        "s2l" "s1c" "s1hint" "s1ul" "s2c"
+#permutation "s1b" "s2b3" "s1l" "s2l" "s1ul" "s1u" "s1c" "s1hint"        "s2c"
+permutation "s1b" "s2b3" "s1l" "s1u" "s1ul" "s2l" "s1c" "s1hint"        "s2c"

src/test/isolation/specs/lock-committed-update.spec

+# Test locking of a tuple with a committed update.  When the lock does not
+# conflict with the update, no blocking and no serializability errors should
+# occur.
+
+setup
+{
+    DROP TABLE IF EXISTS lcu_table;
+    CREATE TABLE lcu_table (id INTEGER PRIMARY KEY, value TEXT);
+    INSERT INTO lcu_table VALUES (1, 'one');
+}
+
+teardown
+{
+    DROP TABLE lcu_table;
+}
+
+session "s1"
+step "s1b"    { BEGIN; }
+step "s1l"    { SELECT pg_advisory_lock(380170116); }
+step "s1u"    { UPDATE lcu_table SET value = 'two' WHERE id = 1; }
+step "s1hint" { SELECT * FROM lcu_table; }
+step "s1ul"   { SELECT pg_advisory_unlock(380170116); }
+step "s1c"    { COMMIT; }
+teardown      { SELECT pg_advisory_unlock_all(); }
+
+session "s2"
+step "s2b1"    { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step "s2b2"    { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step "s2b3"    { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step "s2l"    { SELECT * FROM lcu_table WHERE pg_advisory_lock(380170116) IS NOT NULL FOR KEY SHARE; }
+step "s2c"    { COMMIT; }
+teardown      { SELECT pg_advisory_unlock_all(); }
+
+permutation "s1b" "s2b1" "s1l" "s2l"        "s1u" "s1c"          "s1ul" "s2c"
+permutation "s1b" "s2b1" "s1l" "s1u"        "s2l" "s1c"          "s1ul" "s2c"
+permutation "s1b" "s2b1" "s1l" "s2l" "s1ul" "s1u" "s1c"                 "s2c"
+permutation "s1b" "s2b1" "s1l" "s1u" "s1ul" "s2l" "s1c"                 "s2c"
+
+permutation "s1b" "s2b1" "s1l" "s2l"        "s1u" "s1c" "s1hint" "s1ul" "s2c"
+permutation "s1b" "s2b1" "s1l" "s1u"        "s2l" "s1c" "s1hint" "s1ul" "s2c"
+permutation "s1b" "s2b1" "s1l" "s2l" "s1ul" "s1u" "s1c" "s1hint"        "s2c"
+permutation "s1b" "s2b1" "s1l" "s1u" "s1ul" "s2l" "s1c" "s1hint"        "s2c"
+
+permutation "s1b" "s2b2" "s1l" "s2l"        "s1u" "s1c"          "s1ul" "s2c"
+permutation "s1b" "s2b2" "s1l" "s1u"        "s2l" "s1c"          "s1ul" "s2c"
+permutation "s1b" "s2b2" "s1l" "s2l" "s1ul" "s1u" "s1c"                 "s2c"
+permutation "s1b" "s2b2" "s1l" "s1u" "s1ul" "s2l" "s1c"                 "s2c"
+
+permutation "s1b" "s2b2" "s1l" "s2l"        "s1u" "s1c" "s1hint" "s1ul" "s2c"
+permutation "s1b" "s2b2" "s1l" "s1u"        "s2l" "s1c" "s1hint" "s1ul" "s2c"
+permutation "s1b" "s2b2" "s1l" "s2l" "s1ul" "s1u" "s1c" "s1hint"        "s2c"
+permutation "s1b" "s2b2" "s1l" "s1u" "s1ul" "s2l" "s1c" "s1hint"        "s2c"
+
+permutation "s1b" "s2b3" "s1l" "s2l"        "s1u" "s1c"          "s1ul" "s2c"
+permutation "s1b" "s2b3" "s1l" "s1u"        "s2l" "s1c"          "s1ul" "s2c"
+permutation "s1b" "s2b3" "s1l" "s2l" "s1ul" "s1u" "s1c"                 "s2c"
+permutation "s1b" "s2b3" "s1l" "s1u" "s1ul" "s2l" "s1c"                 "s2c"
+
+permutation "s1b" "s2b3" "s1l" "s2l"        "s1u" "s1c" "s1hint" "s1ul" "s2c"
+permutation "s1b" "s2b3" "s1l" "s1u"        "s2l" "s1c" "s1hint" "s1ul" "s2c"
+permutation "s1b" "s2b3" "s1l" "s2l" "s1ul" "s1u" "s1c" "s1hint"        "s2c"
+permutation "s1b" "s2b3" "s1l" "s1u" "s1ul" "s2l" "s1c" "s1hint"        "s2c"

src/test/isolation/specs/update-locked-tuple.spec

+# Test updating a locked tuple.  When the lock doesn't conflict with the
+# update, no blocking nor serializability problems should occur.
+
+setup
+{
+   DROP TABLE IF EXISTS users, orders;
+   CREATE TABLE users (id INTEGER PRIMARY KEY,
+                       name varchar,
+                       sometime timestamp);
+   CREATE TABLE orders (id INTEGER PRIMARY KEY,
+                        name varchar,
+                        user_id INTEGER REFERENCES users (id));
+   INSERT INTO users (id, name) VALUES (1, 'olivier');
+   INSERT INTO orders (id, name) VALUES (1, 'order of olivier (1)');
+}
+
+teardown
+{
+   DROP TABLE users, orders;
+}
+
+session "s1"
+step "s1b"	 { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step "s1u1"  { UPDATE orders SET name = 'order of olivier (2)', user_id = 1 WHERE id = 1; }
+step "s1u2"  { UPDATE orders SET name = 'order of olivier (3)', user_id = 1 WHERE id = 1; }
+step "s1c"   { COMMIT; }
+
+session "s2"
+step "s2b"   { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step "s2u"   { UPDATE users SET sometime = '1830-10-04' WHERE id = 1; }
+step "s2c"   { COMMIT; }
+
+permutation "s1b" "s2b" "s2u" "s2c" "s1u1" "s1u2" "s1c"
+permutation "s1b" "s2b" "s2u" "s1u1" "s2c" "s1u2" "s1c"
+permutation "s1b" "s2b" "s1u1" "s2u" "s2c" "s1u2" "s1c"
+permutation "s1b" "s1u1" "s2b" "s2u" "s2c" "s1u2" "s1c"
+permutation "s1b" "s1u1" "s2b" "s1u2" "s2u" "s2c" "s1c"
+permutation "s1b" "s1u1" "s1u2" "s2b" "s2u" "s2c" "s1c"