Doom serializable transactions before SSI errors

src/backend/storage/lmgr/predicate.c

@@ -3994,19 +3994,30 @@ CheckForSerializableConflictOut(Relation relation, TransactionId xid, Snapshot s
 				&& (!SxactIsReadOnly(MySerializableXact)
 					|| conflictCommitSeqNo
 					<= MySerializableXact->SeqNo.lastCommitBeforeSnapshot))
+			{
+				/*
+				 * Doom ourselves before raising the error, so a savepoint
+				 * cannot swallow it and let the transaction commit.
+				 */
+				MySerializableXact->flags |= SXACT_FLAG_DOOMED;
 				ereport(ERROR,
 						(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 						 errmsg("could not serialize access due to read/write dependencies among transactions"),
 						 errdetail_internal("Reason code: Canceled on conflict out to old pivot %u.", xid),
 						 errhint("The transaction might succeed if retried.")));
+			}
 
 			if (SxactHasSummaryConflictIn(MySerializableXact)
 				|| !dlist_is_empty(&MySerializableXact->inConflicts))
+			{
+				/* See comment above: doom ourselves before raising the error. */
+				MySerializableXact->flags |= SXACT_FLAG_DOOMED;
 				ereport(ERROR,
 						(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 						 errmsg("could not serialize access due to read/write dependencies among transactions"),
 						 errdetail_internal("Reason code: Canceled on identification as a pivot, with conflict out to old committed transaction %u.", xid),
 						 errhint("The transaction might succeed if retried.")));
+			}
 
 			MySerializableXact->flags |= SXACT_FLAG_SUMMARY_CONFLICT_OUT;
 		}
@@ -4040,6 +4051,8 @@ CheckForSerializableConflictOut(Relation relation, TransactionId xid, Snapshot s
 		}
 		else
 		{
+			/* See comment above: doom ourselves before raising the error. */
+			MySerializableXact->flags |= SXACT_FLAG_DOOMED;
 			LWLockRelease(SerializableXactHashLock);
 			ereport(ERROR,
 					(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
@@ -4589,6 +4602,12 @@ OnConflict_CheckForSerializationFailure(const SERIALIZABLEXACT *reader,
 		 */
 		if (MySerializableXact == writer)
 		{
+			/*
+			 * Mark ourselves doomed before raising the error.  Otherwise a
+			 * subtransaction abort (ROLLBACK TO SAVEPOINT) could swallow this
+			 * error and let the transaction commit anyway, defeating SSI.
+			 */
+			MySerializableXact->flags |= SXACT_FLAG_DOOMED;
 			LWLockRelease(SerializableXactHashLock);
 			ereport(ERROR,
 					(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
@@ -4598,10 +4617,13 @@ OnConflict_CheckForSerializationFailure(const SERIALIZABLEXACT *reader,
 		}
 		else if (SxactIsPrepared(writer))
 		{
-			LWLockRelease(SerializableXactHashLock);
-
 			/* if we're not the writer, we have to be the reader */
 			Assert(MySerializableXact == reader);
+
+			/* See comment above: doom ourselves before raising the error. */
+			MySerializableXact->flags |= SXACT_FLAG_DOOMED;
+			LWLockRelease(SerializableXactHashLock);
+
 			ereport(ERROR,
 					(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 					 errmsg("could not serialize access due to read/write dependencies among transactions"),

src/test/isolation/expected/serializable-savepoint.out

@@ -0,0 +1,25 @@
+Parsed test spec with 3 sessions
+
+starting permutation: r1 w2 w1 c1 sp2 r2 rb2 c2 rall
+step r1: SELECT v FROM t WHERE id = 2;
+v
+-
+0
+(1 row)
+
+step w2: UPDATE t SET v = 1 WHERE id = 2;
+step w1: UPDATE t SET v = 1 WHERE id = 1;
+step c1: COMMIT;
+step sp2: SAVEPOINT f;
+step r2: SELECT v FROM t WHERE id = 1;
+ERROR:  could not serialize access due to read/write dependencies among transactions
+step rb2: ROLLBACK TO SAVEPOINT f;
+step c2: COMMIT;
+ERROR:  could not serialize access due to read/write dependencies among transactions
+step rall: SELECT id, v FROM t ORDER BY id;
+id|v
+--+-
+ 1|1
+ 2|0
+(2 rows)
+

src/test/isolation/isolation_schedule

@@ -127,3 +127,5 @@ test: matview-write-skew
 test: lock-nowait
 test: for-portion-of
 test: ddl-dependency-locking
+
+test: serializable-savepoint

src/test/isolation/specs/serializable-savepoint.spec

@@ -0,0 +1,52 @@
+# Test that a serialization failure raised while *checking* a read (the
+# "conflict out to pivot, during read" cancellation) cannot be discarded by
+# rolling back to a SAVEPOINT.
+#
+# s1 and s2 form the classic write-skew dangerous structure under SERIALIZABLE:
+#   s1 reads row 2 and writes row 1
+#   s2 writes row 2 and reads row 1
+# s1 commits first.  When s2 then reads row 1 it is correctly identified as the
+# pivot of a dangerous structure and PostgreSQL raises
+#   ERROR:  could not serialize access ...
+#   (Canceled on conflict out to pivot ..., during read).
+#
+# Crucially, s2's *write* to row 2 happened BEFORE the savepoint, so it is not
+# undone.  s2 only wraps the offending READ in a SAVEPOINT, rolls back to it
+# (swallowing the error) and commits.  That COMMIT must fail: allowing it
+# leaves both s1's and s2's writes committed, which is the write-skew anomaly
+# SSI is supposed to prevent (no serial order exists).
+
+setup
+{
+  CREATE TABLE t (id int PRIMARY KEY, v int);
+  INSERT INTO t VALUES (1, 0), (2, 0);
+}
+
+teardown
+{
+  DROP TABLE t;
+}
+
+session s1
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step r1  { SELECT v FROM t WHERE id = 2; }
+step w1  { UPDATE t SET v = 1 WHERE id = 1; }
+step c1  { COMMIT; }
+
+session s2
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step w2   { UPDATE t SET v = 1 WHERE id = 2; }
+step sp2  { SAVEPOINT f; }
+step r2   { SELECT v FROM t WHERE id = 1; }
+step rb2  { ROLLBACK TO SAVEPOINT f; }
+step c2   { COMMIT; }
+
+# Used to observe the final committed state.
+session s3
+step rall { SELECT id, v FROM t ORDER BY id; }
+
+# s2 takes its snapshot at w2 (before s1 commits), writes row 2, then after s1
+# commits it reads row 1 inside a savepoint and is cancelled.  After rolling
+# back to the savepoint it must not be able to commit.  If it does (the bug),
+# rall shows both rows updated -- the non-serializable write-skew outcome.
+permutation r1 w2 w1 c1 sp2 r2 rb2 c2 rall