Adjust time qual checking code so that we always check TransactionIdIsInProgress

before we check commit/abort status.  Formerly this was done in some paths
but not all, with the result that a transaction might be considered
committed for some purposes before it became committed for others.
Per example found by Jan Wieck.

1 files changed, 93 insertions, 80 deletions

diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index cb1f7b41014..ef158d97bc3 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -11,12 +11,28 @@
  * "hint" status bits if we see that the inserting or deleting transaction
  * has now committed or aborted.
  *
+ * NOTE: must check TransactionIdIsInProgress (which looks in PGPROC array)
+ * before TransactionIdDidCommit/TransactionIdDidAbort (which look in
+ * pg_clog).  Otherwise we have a race condition: we might decide that a
+ * just-committed transaction crashed, because none of the tests succeed.
+ * xact.c is careful to record commit/abort in pg_clog before it unsets
+ * MyProc->xid in PGPROC array.  That fixes that problem, but it also
+ * means there is a window where TransactionIdIsInProgress and
+ * TransactionIdDidCommit will both return true.  If we check only
+ * TransactionIdDidCommit, we could consider a tuple committed when a
+ * later GetSnapshotData call will still think the originating transaction
+ * is in progress, which leads to application-level inconsistency.  The
+ * upshot is that we gotta check TransactionIdIsInProgress first in all
+ * code paths, except for a few cases where we are looking at
+ * subtransactions of our own main transaction and so there can't be any
+ * race condition.
+ *
  *
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/time/tqual.c,v 1.81 2004/12/31 22:02:56 pgsql Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/time/tqual.c,v 1.81.4.1 2005/05/07 21:22:36 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -144,19 +160,19 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple, Buffer buffer)
 
 			return false;
 		}
-		else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
-		{
-			if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
-			{
-				tuple->t_infomask |= HEAP_XMIN_INVALID;
-				SetBufferCommitInfoNeedsSave(buffer);
-			}
+		else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
 			return false;
+		else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
+		{
+			tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+			SetBufferCommitInfoNeedsSave(buffer);
 		}
 		else
 		{
-			tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+			/* it must have aborted or crashed */
+			tuple->t_infomask |= HEAP_XMIN_INVALID;
 			SetBufferCommitInfoNeedsSave(buffer);
+			return false;
 		}
 	}
 
@@ -179,13 +195,14 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple, Buffer buffer)
 		return false;
 	}
 
+	if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
+		return true;
+
 	if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
 	{
-		if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
-		{
-			tuple->t_infomask |= HEAP_XMAX_INVALID;
-			SetBufferCommitInfoNeedsSave(buffer);
-		}
+		/* it must have aborted or crashed */
+		tuple->t_infomask |= HEAP_XMAX_INVALID;
+		SetBufferCommitInfoNeedsSave(buffer);
 		return true;
 	}
 
@@ -318,19 +335,19 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple, Buffer buffer)
 			else
 				return false;	/* deleted before scan started */
 		}
-		else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
-		{
-			if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
-			{
-				tuple->t_infomask |= HEAP_XMIN_INVALID;
-				SetBufferCommitInfoNeedsSave(buffer);
-			}
+		else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
 			return false;
+		else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
+		{
+			tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+			SetBufferCommitInfoNeedsSave(buffer);
 		}
 		else
 		{
-			tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+			/* it must have aborted or crashed */
+			tuple->t_infomask |= HEAP_XMIN_INVALID;
 			SetBufferCommitInfoNeedsSave(buffer);
+			return false;
 		}
 	}
 
@@ -356,13 +373,14 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple, Buffer buffer)
 			return false;		/* deleted before scan started */
 	}
 
+	if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
+		return true;
+
 	if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
 	{
-		if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
-		{
-			tuple->t_infomask |= HEAP_XMAX_INVALID;
-			SetBufferCommitInfoNeedsSave(buffer);
-		}
+		/* it must have aborted or crashed */
+		tuple->t_infomask |= HEAP_XMAX_INVALID;
+		SetBufferCommitInfoNeedsSave(buffer);
 		return true;
 	}
 
@@ -532,19 +550,19 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,
 				return HeapTupleInvisible;		/* updated before scan
 												 * started */
 		}
-		else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
-		{
-			if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
-			{
-				tuple->t_infomask |= HEAP_XMIN_INVALID;
-				SetBufferCommitInfoNeedsSave(buffer);
-			}
+		else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
 			return HeapTupleInvisible;
+		else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
+		{
+			tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+			SetBufferCommitInfoNeedsSave(buffer);
 		}
 		else
 		{
-			tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+			/* it must have aborted or crashed */
+			tuple->t_infomask |= HEAP_XMIN_INVALID;
 			SetBufferCommitInfoNeedsSave(buffer);
+			return HeapTupleInvisible;
 		}
 	}
 
@@ -571,16 +589,15 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,
 			return HeapTupleInvisible;	/* updated before scan started */
 	}
 
+	if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
+		return HeapTupleBeingUpdated;
+
 	if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
 	{
-		if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
-		{
-			tuple->t_infomask |= HEAP_XMAX_INVALID;
-			SetBufferCommitInfoNeedsSave(buffer);
-			return HeapTupleMayBeUpdated;
-		}
-		/* running xact */
-		return HeapTupleBeingUpdated;	/* in updation by other */
+		/* it must have aborted or crashed */
+		tuple->t_infomask |= HEAP_XMAX_INVALID;
+		SetBufferCommitInfoNeedsSave(buffer);
+		return HeapTupleMayBeUpdated;
 	}
 
 	/* xmax transaction committed */
@@ -684,23 +701,24 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple, Buffer buffer)
 
 			return false;
 		}
-		else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
+		else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
 		{
-			if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
-			{
-				tuple->t_infomask |= HEAP_XMIN_INVALID;
-				SetBufferCommitInfoNeedsSave(buffer);
-				return false;
-			}
 			SnapshotDirty->xmin = HeapTupleHeaderGetXmin(tuple);
 			/* XXX shouldn't we fall through to look at xmax? */
 			return true;		/* in insertion by other */
 		}
-		else
+		else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
 		{
 			tuple->t_infomask |= HEAP_XMIN_COMMITTED;
 			SetBufferCommitInfoNeedsSave(buffer);
 		}
+		else
+		{
+			/* it must have aborted or crashed */
+			tuple->t_infomask |= HEAP_XMIN_INVALID;
+			SetBufferCommitInfoNeedsSave(buffer);
+			return false;
+		}
 	}
 
 	/* by here, the inserting transaction has committed */
@@ -723,17 +741,18 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple, Buffer buffer)
 		return false;
 	}
 
-	if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
+	if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
 	{
-		if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
-		{
-			tuple->t_infomask |= HEAP_XMAX_INVALID;
-			SetBufferCommitInfoNeedsSave(buffer);
-			return true;
-		}
-		/* running xact */
 		SnapshotDirty->xmax = HeapTupleHeaderGetXmax(tuple);
-		return true;			/* in updation by other */
+		return true;
+	}
+
+	if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
+	{
+		/* it must have aborted or crashed */
+		tuple->t_infomask |= HEAP_XMAX_INVALID;
+		SetBufferCommitInfoNeedsSave(buffer);
+		return true;
 	}
 
 	/* xmax transaction committed */
@@ -847,19 +866,19 @@ HeapTupleSatisfiesSnapshot(HeapTupleHeader tuple, Snapshot snapshot,
 			else
 				return false;	/* deleted before scan started */
 		}
-		else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
-		{
-			if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
-			{
-				tuple->t_infomask |= HEAP_XMIN_INVALID;
-				SetBufferCommitInfoNeedsSave(buffer);
-			}
+		else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
 			return false;
+		else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
+		{
+			tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+			SetBufferCommitInfoNeedsSave(buffer);
 		}
 		else
 		{
-			tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+			/* it must have aborted or crashed */
+			tuple->t_infomask |= HEAP_XMIN_INVALID;
 			SetBufferCommitInfoNeedsSave(buffer);
+			return false;
 		}
 	}
 
@@ -915,13 +934,14 @@ HeapTupleSatisfiesSnapshot(HeapTupleHeader tuple, Snapshot snapshot,
 				return false;	/* deleted before scan started */
 		}
 
+		if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
+			return true;
+
 		if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
 		{
-			if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
-			{
-				tuple->t_infomask |= HEAP_XMAX_INVALID;
-				SetBufferCommitInfoNeedsSave(buffer);
-			}
+			/* it must have aborted or crashed */
+			tuple->t_infomask |= HEAP_XMAX_INVALID;
+			SetBufferCommitInfoNeedsSave(buffer);
 			return true;
 		}
 
@@ -985,13 +1005,6 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin,
 	 *
 	 * If the inserting transaction aborted, then the tuple was never visible
 	 * to any other transaction, so we can delete it immediately.
-	 *
-	 * NOTE: must check TransactionIdIsInProgress (which looks in PROC array)
-	 * before TransactionIdDidCommit/TransactionIdDidAbort (which look in
-	 * pg_clog).  Otherwise we have a race condition where we might decide
-	 * that a just-committed transaction crashed, because none of the
-	 * tests succeed.  xact.c is careful to record commit/abort in pg_clog
-	 * before it unsets MyProc->xid in PROC array.
 	 */
 	if (!(tuple->t_infomask & HEAP_XMIN_COMMITTED))
 	{