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.

diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index ac06d91dc485e5fe4428a6a69dd73d73d09ae45c..c3aa085996376bf5f6559c6108d301d7b176537d 100644 (file)
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -11,12 +11,26 @@
  * containing the tuple.  (VACUUM FULL assumes it's sufficient to have
  * exclusive lock on the containing relation, instead.)
  *
+ * 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.
+ *
  *
  * Portions Copyright (c) 1996-2001, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/utils/time/tqual.c,v 1.49 2002/01/16 23:51:56 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/utils/time/tqual.c,v 1.49.2.1 2005/05/07 21:23:49 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -109,14 +123,16 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple)
 
                        return false;
                }
-               else if (!TransactionIdDidCommit(tuple->t_xmin))
+               else if (TransactionIdIsInProgress(tuple->t_xmin))
+                       return false;
+               else if (TransactionIdDidCommit(tuple->t_xmin))
+                       tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+               else
                {
-                       if (TransactionIdDidAbort(tuple->t_xmin))
-                               tuple->t_infomask |= HEAP_XMIN_INVALID; /* aborted */
+                       /* it must have aborted or crashed */
+                       tuple->t_infomask |= HEAP_XMIN_INVALID;
                        return false;
                }
-               else
-                       tuple->t_infomask |= HEAP_XMIN_COMMITTED;
        }
 
        /* by here, the inserting transaction has committed */
@@ -138,10 +154,13 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple)
                return false;
        }
 
+       if (TransactionIdIsInProgress(tuple->t_xmax))
+               return true;
+
        if (!TransactionIdDidCommit(tuple->t_xmax))
        {
-               if (TransactionIdDidAbort(tuple->t_xmax))
-                       tuple->t_infomask |= HEAP_XMAX_INVALID;         /* aborted */
+               /* it must have aborted or crashed */
+               tuple->t_infomask |= HEAP_XMAX_INVALID;
                return true;
        }
 
@@ -254,14 +273,16 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple)
                        else
                                return false;   /* deleted before scan started */
                }
-               else if (!TransactionIdDidCommit(tuple->t_xmin))
+               else if (TransactionIdIsInProgress(tuple->t_xmin))
+                       return false;
+               else if (TransactionIdDidCommit(tuple->t_xmin))
+                       tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+               else
                {
-                       if (TransactionIdDidAbort(tuple->t_xmin))
-                               tuple->t_infomask |= HEAP_XMIN_INVALID; /* aborted */
+                       /* it must have aborted or crashed */
+                       tuple->t_infomask |= HEAP_XMIN_INVALID;
                        return false;
                }
-               else
-                       tuple->t_infomask |= HEAP_XMIN_COMMITTED;
        }
 
        /* by here, the inserting transaction has committed */
@@ -286,10 +307,13 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple)
                        return false;           /* deleted before scan started */
        }
 
+       if (TransactionIdIsInProgress(tuple->t_xmax))
+               return true;
+
        if (!TransactionIdDidCommit(tuple->t_xmax))
        {
-               if (TransactionIdDidAbort(tuple->t_xmax))
-                       tuple->t_infomask |= HEAP_XMAX_INVALID;         /* aborted */
+               /* it must have aborted or crashed */
+               tuple->t_infomask |= HEAP_XMAX_INVALID;
                return true;
        }
 
@@ -428,14 +452,16 @@ HeapTupleSatisfiesUpdate(HeapTuple htuple)
                                return HeapTupleInvisible;              /* updated before scan
                                                                                                 * started */
                }
-               else if (!TransactionIdDidCommit(tuple->t_xmin))
+               else if (TransactionIdIsInProgress(tuple->t_xmin))
+                       return HeapTupleInvisible;
+               else if (TransactionIdDidCommit(tuple->t_xmin))
+                       tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+               else
                {
-                       if (TransactionIdDidAbort(tuple->t_xmin))
-                               tuple->t_infomask |= HEAP_XMIN_INVALID; /* aborted */
+                       /* it must have aborted or crashed */
+                       tuple->t_infomask |= HEAP_XMIN_INVALID;
                        return HeapTupleInvisible;
                }
-               else
-                       tuple->t_infomask |= HEAP_XMIN_COMMITTED;
        }
 
        /* by here, the inserting transaction has committed */
@@ -461,15 +487,14 @@ HeapTupleSatisfiesUpdate(HeapTuple htuple)
                        return HeapTupleInvisible;      /* updated before scan started */
        }
 
+       if (TransactionIdIsInProgress(tuple->t_xmax))
+               return HeapTupleBeingUpdated;
+
        if (!TransactionIdDidCommit(tuple->t_xmax))
        {
-               if (TransactionIdDidAbort(tuple->t_xmax))
-               {
-                       tuple->t_infomask |= HEAP_XMAX_INVALID;         /* aborted */
-                       return HeapTupleMayBeUpdated;
-               }
-               /* running xact */
-               return HeapTupleBeingUpdated;   /* in updation by other */
+               /* it must have aborted or crashed */
+               tuple->t_infomask |= HEAP_XMAX_INVALID;
+               return HeapTupleMayBeUpdated;
        }
 
        /* xmax transaction committed */
@@ -553,19 +578,20 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple)
 
                        return false;
                }
-               else if (!TransactionIdDidCommit(tuple->t_xmin))
+               else if (TransactionIdIsInProgress(tuple->t_xmin))
                {
-                       if (TransactionIdDidAbort(tuple->t_xmin))
-                       {
-                               tuple->t_infomask |= HEAP_XMIN_INVALID;
-                               return false;
-                       }
                        SnapshotDirty->xmin = tuple->t_xmin;
                        /* XXX shouldn't we fall through to look at xmax? */
                        return true;            /* in insertion by other */
                }
-               else
+               else if (TransactionIdDidCommit(tuple->t_xmin))
                        tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+               else
+               {
+                       /* it must have aborted or crashed */
+                       tuple->t_infomask |= HEAP_XMIN_INVALID;
+                       return false;
+               }
        }
 
        /* by here, the inserting transaction has committed */
@@ -588,16 +614,17 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple)
                return false;
        }
 
-       if (!TransactionIdDidCommit(tuple->t_xmax))
+       if (TransactionIdIsInProgress(tuple->t_xmax))
        {
-               if (TransactionIdDidAbort(tuple->t_xmax))
-               {
-                       tuple->t_infomask |= HEAP_XMAX_INVALID;         /* aborted */
-                       return true;
-               }
-               /* running xact */
                SnapshotDirty->xmax = tuple->t_xmax;
-               return true;                    /* in updation by other */
+               return true;
+       }
+
+       if (!TransactionIdDidCommit(tuple->t_xmax))
+       {
+               /* it must have aborted or crashed */
+               tuple->t_infomask |= HEAP_XMAX_INVALID;
+               return true;
        }
 
        /* xmax transaction committed */
@@ -693,14 +720,16 @@ HeapTupleSatisfiesSnapshot(HeapTupleHeader tuple, Snapshot snapshot)
                        else
                                return false;   /* deleted before scan started */
                }
-               else if (!TransactionIdDidCommit(tuple->t_xmin))
+               else if (TransactionIdIsInProgress(tuple->t_xmin))
+                       return false;
+               else if (TransactionIdDidCommit(tuple->t_xmin))
+                       tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+               else
                {
-                       if (TransactionIdDidAbort(tuple->t_xmin))
-                               tuple->t_infomask |= HEAP_XMIN_INVALID;
+                       /* it must have aborted or crashed */
+                       tuple->t_infomask |= HEAP_XMIN_INVALID;
                        return false;
                }
-               else
-                       tuple->t_infomask |= HEAP_XMIN_COMMITTED;
        }
 
        /*
@@ -737,10 +766,13 @@ HeapTupleSatisfiesSnapshot(HeapTupleHeader tuple, Snapshot snapshot)
                                return false;   /* deleted before scan started */
                }
 
+               if (TransactionIdIsInProgress(tuple->t_xmax))
+                       return true;
+
                if (!TransactionIdDidCommit(tuple->t_xmax))
                {
-                       if (TransactionIdDidAbort(tuple->t_xmax))
-                               tuple->t_infomask |= HEAP_XMAX_INVALID; /* aborted */
+                       /* it must have aborted or crashed */
+                       tuple->t_infomask |= HEAP_XMAX_INVALID;
                        return true;
                }
 
@@ -785,13 +817,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))
        {
@@ -828,17 +853,9 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin)
                        return HEAPTUPLE_INSERT_IN_PROGRESS;
                else if (TransactionIdDidCommit(tuple->t_xmin))
                        tuple->t_infomask |= HEAP_XMIN_COMMITTED;
-               else if (TransactionIdDidAbort(tuple->t_xmin))
-               {
-                       tuple->t_infomask |= HEAP_XMIN_INVALID;
-                       return HEAPTUPLE_DEAD;
-               }
                else
                {
-                       /*
-                        * Not in Progress, Not Committed, Not Aborted - so it's from
-                        * crashed process. - vadim 11/26/96
-                        */
+                       /* it must be aborted or crashed */
                        tuple->t_infomask |= HEAP_XMIN_INVALID;
                        return HEAPTUPLE_DEAD;
                }
@@ -879,22 +896,12 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin)
                        return HEAPTUPLE_DELETE_IN_PROGRESS;
                else if (TransactionIdDidCommit(tuple->t_xmax))
                        tuple->t_infomask |= HEAP_XMAX_COMMITTED;
-               else if (TransactionIdDidAbort(tuple->t_xmax))
-               {
-                       tuple->t_infomask |= HEAP_XMAX_INVALID;
-                       return HEAPTUPLE_LIVE;
-               }
                else
                {
-                       /*
-                        * Not in Progress, Not Committed, Not Aborted - so it's from
-                        * crashed process. - vadim 06/02/97
-                        */
+                       /* it must be aborted or crashed */
                        tuple->t_infomask |= HEAP_XMAX_INVALID;
                        return HEAPTUPLE_LIVE;
                }
-               /* Should only get here if we set XMAX_COMMITTED */
-               Assert(tuple->t_infomask & HEAP_XMAX_COMMITTED);
        }
 
        /*