Fix locking a tuple updated by an aborted (sub)transaction

alvherre · alvherre · commit dfe7121dfbee · 2016-09-09T15:54:29.000-03:00
When heap_lock_tuple decides to follow the update chain, it tried to also lock any version of the tuple that was created by an update that was subsequently rolled back. This is pointless, since for all intents and purposes that tuple exists no more; and moreover it causes misbehavior, as reported independently by Marko Tiikkaja and Marti Raudsepp: some SELECT FOR UPDATE/SHARE queries may fail to return the tuples, and assertion-enabled builds crash. Fix by having heap_lock_updated_tuple test the xmin and return success immediately if the tuple was created by an aborted transaction. The condition where tuples become invisible occurs when an updated tuple chain is followed by heap_lock_updated_tuple, which reports the problem as HeapTupleSelfUpdated to its caller heap_lock_tuple, which in turn propagates that code outwards possibly leading the calling code (ExecLockRows) to believe that the tuple exists no longer. Backpatch to 9.3. Only on 9.5 and newer this leads to a visible failure, because of commit 27846f0; before that, heap_lock_tuple skips the whole dance when the tuple is already locked by the same transaction, because of the ancient HeapTupleSatisfiesUpdate behavior. Still, the buggy condition may also exist in more convoluted scenarios involving concurrent transactions, so it seems safer to fix the bug in the old branches too. Discussion: https://www.postgresql.org/message-id/CABRT9RC81YUf1=jsmWopcKJEro=VoeG2ou6sPwyOUTx_qteRsg@mail.gmail.com https://www.postgresql.org/message-id/48d3eade-98d3-8b9a-477e-1a8dc32a724d@joh.to
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
@@ -5048,6 +5048,17 @@ heap_lock_updated_tuple_rec(Relation rel, ItemPointer tid, TransactionId xid,
 			return HeapTupleMayBeUpdated;
 		}
 
+		/*
+		 * Also check Xmin: if this tuple was created by an aborted
+		 * (sub)transaction, then we already locked the last live one in the
+		 * chain, thus we're done, so return success.
+		 */
+		if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(mytup.t_data)))
+		{
+			UnlockReleaseBuffer(buf);
+			return HeapTupleMayBeUpdated;
+		}
+
 		old_infomask = mytup.t_data->t_infomask;
 		old_infomask2 = mytup.t_data->t_infomask2;
 		xmax = HeapTupleHeaderGetRawXmax(mytup.t_data);
diff --git a/src/test/regress/expected/combocid.out b/src/test/regress/expected/combocid.out
@@ -140,3 +140,30 @@ SELECT ctid,cmin,* FROM combocidtest;
  (0,6) |    0 |    444
 (3 rows)
 
+-- test for bug reported in
+-- CABRT9RC81YUf1=jsmWopcKJEro=VoeG2ou6sPwyOUTx_qteRsg@mail.gmail.com
+CREATE TABLE IF NOT EXISTS testcase(
+	id int PRIMARY KEY,
+	balance numeric
+);
+INSERT INTO testcase VALUES (1, 0);
+BEGIN;
+SELECT * FROM testcase WHERE testcase.id = 1 FOR UPDATE;
+ id | balance 
+----+---------
+  1 |       0
+(1 row)
+
+UPDATE testcase SET balance = balance + 400 WHERE id=1;
+SAVEPOINT subxact;
+UPDATE testcase SET balance = balance - 100 WHERE id=1;
+ROLLBACK TO SAVEPOINT subxact;
+-- should return one tuple
+SELECT * FROM testcase WHERE id = 1 FOR UPDATE;
+ id | balance 
+----+---------
+  1 |     400
+(1 row)
+
+ROLLBACK;
+DROP TABLE testcase;
diff --git a/src/test/regress/sql/combocid.sql b/src/test/regress/sql/combocid.sql
@@ -91,3 +91,21 @@ SELECT ctid,cmin,* FROM combocidtest;
 COMMIT;
 
 SELECT ctid,cmin,* FROM combocidtest;
+
+-- test for bug reported in
+-- CABRT9RC81YUf1=jsmWopcKJEro=VoeG2ou6sPwyOUTx_qteRsg@mail.gmail.com
+CREATE TABLE IF NOT EXISTS testcase(
+	id int PRIMARY KEY,
+	balance numeric
+);
+INSERT INTO testcase VALUES (1, 0);
+BEGIN;
+SELECT * FROM testcase WHERE testcase.id = 1 FOR UPDATE;
+UPDATE testcase SET balance = balance + 400 WHERE id=1;
+SAVEPOINT subxact;
+UPDATE testcase SET balance = balance - 100 WHERE id=1;
+ROLLBACK TO SAVEPOINT subxact;
+-- should return one tuple
+SELECT * FROM testcase WHERE id = 1 FOR UPDATE;
+ROLLBACK;
+DROP TABLE testcase;
