From af3855cb77b94a0321b77d7d9a9849700ea6a758 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Wed, 11 Jan 2023 15:31:42 -0800 Subject: [PATCH] Improve TransactionIdDidAbort() documentation. Document that TransactionIdDidAbort() won't indicate that transactions that were in-progress during a crash have aborted. Tie this to existing discussion of the TransactionIdDidCommit() and TransactionIdDidCommit() protocol that code in heapam_visibility.c (and a few other places) must observe. Follow-up to bugfix commit eb5ad4ff. Author: Peter Geoghegan Reviewed-By: Andres Freund Discussion: https://postgr.es/m/CAH2-Wzn4bEEqgmaUQL3aJ73yM9gAeK-wE4ngi7kjRjLztb+P0w@mail.gmail.com --- src/backend/access/heap/heapam_visibility.c | 33 +++++++++++---------- src/backend/access/transam/transam.c | 9 +++++- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c index bcf9a9b1bb..a716001341 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. 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 3a28dcc43a..7629904bbf 100644 --- a/src/backend/access/transam/transam.c +++ b/src/backend/access/transam/transam.c @@ -110,7 +110,8 @@ TransactionLogFetch(TransactionId transactionId) * transaction tree. * * See also TransactionIdIsInProgress, which once was in this module - * but now lives in procarray.c. + * but now lives in procarray.c, as well as comments at the top of + * heapam_visibility.c that explain how everything fits together. * ---------------------------------------------------------------- */ @@ -176,6 +177,12 @@ TransactionIdDidCommit(TransactionId transactionId) * * Note: * Assumes transaction identifier is valid and exists in clog. + * + * Returns true only for explicitly aborted transactions, as transactions + * implicitly aborted due to a crash will commonly still appear to be + * in-progress in the clog. Most of the time TransactionIdDidCommit(), + * with a preceding TransactionIdIsInProgress() check, should be used + * instead of TransactionIdDidAbort(). */ bool /* true if given transaction aborted */ TransactionIdDidAbort(TransactionId transactionId)