From fbc06f4323fd3bf38c5923a9bd6090c041053b3b Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Fri, 6 Jan 2023 11:12:31 -0800 Subject: [PATCH v1] Document TransactionIdDidAbort() hard crash behavior. --- src/backend/access/heap/heapam_visibility.c | 33 +++++++++++---------- src/backend/access/transam/transam.c | 4 +++ 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c index bcf9a9b1b..b22e1664a 100644 --- a/src/backend/access/heap/heapam_visibility.c +++ b/src/backend/access/heap/heapam_visibility.c @@ -11,21 +11,24 @@ * shared buffer content lock on the buffer containing the tuple. * * NOTE: When using a non-MVCC snapshot, we must check - * TransactionIdIsInProgress (which looks in the PGPROC array) - * before TransactionIdDidCommit/TransactionIdDidAbort (which look in - * pg_xact). 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_xact before it unsets - * MyProc->xid in the 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. + * TransactionIdIsInProgress (which looks in the PGPROC array) before + * TransactionIdDidCommit (which look in pg_xact). 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_xact before it unsets MyProc->xid in the 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. + * + * We can't use TransactionIdDidAbort here because it won't treat transactions + * that were in progress during a crash as aborted by now. We determine that + * transactions aborted/crashed through process of elimination instead. * * When using an MVCC snapshot, we rely on XidInMVCCSnapshot rather than * TransactionIdIsInProgress, but the logic is otherwise the same: do not diff --git a/src/backend/access/transam/transam.c b/src/backend/access/transam/transam.c index 3a28dcc43..5e92cc168 100644 --- a/src/backend/access/transam/transam.c +++ b/src/backend/access/transam/transam.c @@ -176,6 +176,10 @@ TransactionIdDidCommit(TransactionId transactionId) * * Note: * Assumes transaction identifier is valid and exists in clog. + * + * Not all transactions that must be treated as aborted will be + * explicitly marked as such in clog. Transactions that were in + * progress during a crash are never reported as aborted by us. */ bool /* true if given transaction aborted */ TransactionIdDidAbort(TransactionId transactionId) -- 2.38.1