From 2e6482493a2446a488550545e0d1dd6b1a79c70b Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 7 May 2005 21:23:49 +0000 Subject: [PATCH] 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. --- src/backend/utils/time/tqual.c | 159 +++++++++++++++++---------------- 1 file changed, 83 insertions(+), 76 deletions(-) diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c index ac06d91dc4..c3aa085996 100644 --- 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 (TransactionIdIsInProgress(tuple->t_xmax)) + { + SnapshotDirty->xmax = tuple->t_xmax; + return true; + } + if (!TransactionIdDidCommit(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 */ + /* 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); } /*