Fix a bug with SSI and prepared transactions:

If there's a dangerous structure T0 ---> T1 ---> T2, and T2 commits first,
we need to abort something. If T2 commits before both conflicts appear,
then it should be caught by OnConflict_CheckForSerializationFailure. If
both conflicts appear before T2 commits, it should be caught by
PreCommit_CheckForSerializationFailure. But that is actually run when
T2 *prepares*. Fix that in OnConflict_CheckForSerializationFailure, by
treating a prepared T2 as if it committed already.

This is mostly a problem for prepared transactions, which are in prepared
state for some time, but also for regular transactions because they also go
through the prepared state in the SSI code for a short moment when they're
committed.

Kevin Grittner and Dan Ports

4 files changed, 40 insertions, 8 deletions

diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 1707a746646..f0c8ee48c34 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -244,6 +244,14 @@
 
 #define SxactIsOnFinishedList(sxact) (!SHMQueueIsDetached(&((sxact)->finishedLink)))
 
+/*
+ * Note that a sxact is marked "prepared" once it has passed
+ * PreCommit_CheckForSerializationFailure, even if it isn't using
+ * 2PC. This is the point at which it can no longer be aborted.
+ *
+ * The PREPARED flag remains set after commit, so SxactIsCommitted
+ * implies SxactIsPrepared.
+ */
 #define SxactIsCommitted(sxact) (((sxact)->flags & SXACT_FLAG_COMMITTED) != 0)
 #define SxactIsPrepared(sxact) (((sxact)->flags & SXACT_FLAG_PREPARED) != 0)
 #define SxactIsRolledBack(sxact) (((sxact)->flags & SXACT_FLAG_ROLLED_BACK) != 0)
@@ -3165,6 +3173,13 @@ ReleasePredicateLocks(bool isCommit)
 		 */
 		MySerializableXact->flags |= SXACT_FLAG_DOOMED;
 		MySerializableXact->flags |= SXACT_FLAG_ROLLED_BACK;
+		/*
+		 * If the transaction was previously prepared, but is now failing due
+		 * to a ROLLBACK PREPARED or (hopefully very rare) error after the
+		 * prepare, clear the prepared flag.  This simplifies conflict
+		 * checking.
+		 */
+		MySerializableXact->flags &= ~SXACT_FLAG_PREPARED;
 	}
 
 	if (!topLevelIsDeclaredReadOnly)
@@ -4363,6 +4378,11 @@ OnConflict_CheckForSerializationFailure(const SERIALIZABLEXACT *reader,
 	 * - the writer committed before T2
 	 * - the reader is a READ ONLY transaction and the reader was concurrent
 	 *	 with T2 (= reader acquired its snapshot before T2 committed)
+	 *
+	 * We also handle the case that T2 is prepared but not yet committed
+	 * here. In that case T2 has already checked for conflicts, so if it
+	 * commits first, making the above conflict real, it's too late for it
+	 * to abort.
 	 *------------------------------------------------------------------------
 	 */
 	if (!failure)
@@ -4381,7 +4401,12 @@ OnConflict_CheckForSerializationFailure(const SERIALIZABLEXACT *reader,
 		{
 			SERIALIZABLEXACT *t2 = conflict->sxactIn;
 
-			if (SxactIsCommitted(t2)
+			/*
+			 * Note that if T2 is merely prepared but not yet committed, we
+			 * rely on t->commitSeqNo being InvalidSerCommitSeqNo, which is
+			 * larger than any valid commit sequence number.
+			 */
+			if (SxactIsPrepared(t2)
 				&& (!SxactIsCommitted(reader)
 					|| t2->commitSeqNo <= reader->commitSeqNo)
 				&& (!SxactIsCommitted(writer)
@@ -4400,7 +4425,8 @@ OnConflict_CheckForSerializationFailure(const SERIALIZABLEXACT *reader,
 	}
 
 	/*------------------------------------------------------------------------
-	 * Check whether the reader has become a pivot with a committed writer:
+	 * Check whether the reader has become a pivot with a writer
+	 * that's committed (or prepared):
 	 *
 	 *		T0 ------> R ------> W
 	 *			 rw		   rw
@@ -4411,7 +4437,7 @@ OnConflict_CheckForSerializationFailure(const SERIALIZABLEXACT *reader,
 	 * - T0 is READ ONLY, and overlaps the writer
 	 *------------------------------------------------------------------------
 	 */
-	if (!failure && SxactIsCommitted(writer) && !SxactIsReadOnly(reader))
+	if (!failure && SxactIsPrepared(writer) && !SxactIsReadOnly(reader))
 	{
 		if (SxactHasSummaryConflictIn(reader))
 		{
@@ -4427,6 +4453,12 @@ OnConflict_CheckForSerializationFailure(const SERIALIZABLEXACT *reader,
 		{
 			SERIALIZABLEXACT *t0 = conflict->sxactOut;
 
+			/*
+			 * Note that if the writer is merely prepared but not yet
+			 * committed, we rely on writer->commitSeqNo being
+			 * InvalidSerCommitSeqNo, which is larger than any valid commit
+			 * sequence number.
+			 */
 			if (!SxactIsDoomed(t0)
 				&& (!SxactIsCommitted(t0)
 					|| t0->commitSeqNo >= writer->commitSeqNo)
diff --git a/src/test/regress/expected/prepared_xacts.out b/src/test/regress/expected/prepared_xacts.out
index e094476cb4a..9697ace16a5 100644
--- a/src/test/regress/expected/prepared_xacts.out
+++ b/src/test/regress/expected/prepared_xacts.out
@@ -131,12 +131,12 @@ SELECT * FROM pxtest1;
  ddd
 (2 rows)
 
-INSERT INTO pxtest1 VALUES ('fff');
 -- This should fail, because the two transactions have a write-skew anomaly
-PREPARE TRANSACTION 'foo5';
+INSERT INTO pxtest1 VALUES ('fff');
 ERROR:  could not serialize access due to read/write dependencies among transactions
-DETAIL:  Canceled on commit attempt with conflict in from prepared pivot.
+DETAIL:  Canceled on identification as a pivot, during write.
 HINT:  The transaction might succeed if retried.
+PREPARE TRANSACTION 'foo5';
 SELECT gid FROM pg_prepared_xacts;
  gid  
 ------
diff --git a/src/test/regress/expected/prepared_xacts_1.out b/src/test/regress/expected/prepared_xacts_1.out
index acd90467328..898f278c11e 100644
--- a/src/test/regress/expected/prepared_xacts_1.out
+++ b/src/test/regress/expected/prepared_xacts_1.out
@@ -134,8 +134,8 @@ SELECT * FROM pxtest1;
  aaa
 (1 row)
 
-INSERT INTO pxtest1 VALUES ('fff');
 -- This should fail, because the two transactions have a write-skew anomaly
+INSERT INTO pxtest1 VALUES ('fff');
 PREPARE TRANSACTION 'foo5';
 ERROR:  prepared transactions are disabled
 HINT:  Set max_prepared_transactions to a nonzero value.
diff --git a/src/test/regress/sql/prepared_xacts.sql b/src/test/regress/sql/prepared_xacts.sql
index e06c9d47c13..7902152775c 100644
--- a/src/test/regress/sql/prepared_xacts.sql
+++ b/src/test/regress/sql/prepared_xacts.sql
@@ -74,9 +74,9 @@ SELECT gid FROM pg_prepared_xacts;
 
 BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
 SELECT * FROM pxtest1;
-INSERT INTO pxtest1 VALUES ('fff');
 
 -- This should fail, because the two transactions have a write-skew anomaly
+INSERT INTO pxtest1 VALUES ('fff');
 PREPARE TRANSACTION 'foo5';
 
 SELECT gid FROM pg_prepared_xacts;