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 <pg@bowt.ie>
Reviewed-By: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CAH2-Wzn4bEEqgmaUQL3aJ73yM9gAeK-wE4ngi7kjRjLztb+P0w@mail.gmail.com

diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index bcf9a9b1bbd9567405c5f26f7b06ce01232313bf..a71600134191d04d636c7bed5f5434e0111af3cb 100644 (file)
--- 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 3a28dcc43aa66dd75276dba6846550bfc9025a5f..7629904bbf736fbcca4a5767fa5e2359c9ec5bf0 100644 (file)
--- 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)