From 501ec7b64c25c8fd595020e02f1cb939f176b30c Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 7 May 2005 21:23:02 +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 | 137 +++++++++++++++++++-------------- 1 file changed, 81 insertions(+), 56 deletions(-) diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c index d7d22b77866..7e073610900 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-2003, 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.70 2003/10/01 21:30:52 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/time/tqual.c,v 1.70.2.1 2005/05/07 21:23:02 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -122,14 +136,16 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple) return false; } - else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple))) + else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple))) + return false; + else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple))) + tuple->t_infomask |= HEAP_XMIN_COMMITTED; + else { - if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple))) - 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 */ @@ -151,10 +167,13 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple) return false; } + if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple))) + return true; + if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple))) { - if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple))) - tuple->t_infomask |= HEAP_XMAX_INVALID; /* aborted */ + /* it must have aborted or crashed */ + tuple->t_infomask |= HEAP_XMAX_INVALID; return true; } @@ -271,14 +290,16 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple) else return false; /* deleted before scan started */ } - else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple))) + else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple))) + return false; + else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple))) + tuple->t_infomask |= HEAP_XMIN_COMMITTED; + else { - if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple))) - 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 */ @@ -303,10 +324,13 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple) 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; /* aborted */ + /* it must have aborted or crashed */ + tuple->t_infomask |= HEAP_XMAX_INVALID; return true; } @@ -453,14 +477,16 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid) return HeapTupleInvisible; /* updated before scan * started */ } - else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple))) + else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple))) + return HeapTupleInvisible; + else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple))) + tuple->t_infomask |= HEAP_XMIN_COMMITTED; + else { - if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple))) - 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 */ @@ -486,15 +512,14 @@ 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; /* 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 */ @@ -582,19 +607,20 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple) return false; } - else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple))) + else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple))) { - if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple))) - { - tuple->t_infomask |= HEAP_XMIN_INVALID; - 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; + else + { + /* it must have aborted or crashed */ + tuple->t_infomask |= HEAP_XMIN_INVALID; + return false; + } } /* by here, the inserting transaction has committed */ @@ -617,16 +643,17 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple) return false; } + if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple))) + { + SnapshotDirty->xmax = HeapTupleHeaderGetXmax(tuple); + return true; + } + if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple))) { - if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple))) - { - tuple->t_infomask |= HEAP_XMAX_INVALID; /* aborted */ - return true; - } - /* running xact */ - SnapshotDirty->xmax = HeapTupleHeaderGetXmax(tuple); - return true; /* in updation by other */ + /* it must have aborted or crashed */ + tuple->t_infomask |= HEAP_XMAX_INVALID; + return true; } /* xmax transaction committed */ @@ -722,14 +749,16 @@ HeapTupleSatisfiesSnapshot(HeapTupleHeader tuple, Snapshot snapshot) else return false; /* deleted before scan started */ } - else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple))) + else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple))) + return false; + else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple))) + tuple->t_infomask |= HEAP_XMIN_COMMITTED; + else { - if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple))) - 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; } /* @@ -769,10 +798,13 @@ 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; /* aborted */ + /* it must have aborted or crashed */ + tuple->t_infomask |= HEAP_XMAX_INVALID; return true; } @@ -821,13 +853,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)) {