projects / postgresql.git / commitdiff
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: 68ab4de)
Adjust time qual checking code so that we always check TransactionIdIsInProgress
authorTom Lane <tgl@sss.pgh.pa.us>
Sat, 7 May 2005 21:23:49 +0000 (21:23 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Sat, 7 May 2005 21:23:49 +0000 (21:23 +0000)
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.

src/backend/utils/time/tqual.c patch | blob | blame | history

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);
    }
 
    /*
This is the main PostgreSQL git repository.