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

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)
+ * TransactionIdIsInProgress (which looks in the PGPROC array) before
- * before TransactionIdDidCommit/TransactionIdDidAbort (which look in
+ * TransactionIdDidCommit (which look in pg_xact).  Otherwise we have a race
- * pg_xact).  Otherwise we have a race condition: we might decide that a
+ * condition: we might decide that a just-committed transaction crashed,
- * just-committed transaction crashed, because none of the tests succeed.
+ * because none of the tests succeed.  xact.c is careful to record
- * xact.c is careful to record commit/abort in pg_xact before it unsets
+ * commit/abort in pg_xact before it unsets MyProc->xid in the PGPROC array.
- * MyProc->xid in the PGPROC array.  That fixes that problem, but it
+ * That fixes that problem, but it also means there is a window where
- * also means there is a window where TransactionIdIsInProgress and
+ * TransactionIdIsInProgress and TransactionIdDidCommit will both return true.
- * TransactionIdDidCommit will both return true.  If we check only
+ * If we check only TransactionIdDidCommit, we could consider a tuple
- * TransactionIdDidCommit, we could consider a tuple committed when a
+ * committed when a later GetSnapshotData call will still think the
- * later GetSnapshotData call will still think the originating transaction
+ * originating transaction is in progress, which leads to application-level
- * is in progress, which leads to application-level inconsistency.  The
+ * inconsistency.  The upshot is that we gotta check TransactionIdIsInProgress
- * upshot is that we gotta check TransactionIdIsInProgress first in all
+ * first in all code paths, except for a few cases where we are looking at
- * 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
- * subtransactions of our own main transaction and so there can't be any
+ * condition.
- * 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

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)