postgres.git / summary / log / commit / refs

commit    070e21268b2cde728dbd4aae529ac591f26afb27
Author:   Álvaro Herrera <alvherre@kurilemu.de>
Date:     Thu Sep 24 09:52:47 2026 +0000

    Wait for transactions of an initial decoding snapshot to commit
    
    SnapBuildInitialSnapshot() converts the snapshot builder's list of
    committed transactions into a regular MVCC snapshot, which is then used
    with HeapTupleSatisfiesMVCC().  However, a snapshot produced that way
    and used as an MVCC snapshot can potentially cause data corruption, if
    it captures a transaction after it writes its commit WAL record but
    before it has updated its CLOG entry: it will be incorrectly used to set
    hint bits as if that transaction had aborted.
    
    Fix by having SnapBuildInitialSnapshot() wait until such transactions
    have removed themselves from procarray, which guarantees correct
    visibility.
    
    Other uses of SnapBuildBuildSnapshot only use the snapshot as
    historical, so they don't need the waits, but add commentary in that
    function to alert possible future callers.
    
    While at this, backpatch commit 504fe10d1d26, "Fix misplaced comment in
    snapbuild.c."  The comment it moved was nearby the place we modify.
    
    This bug was diagnosed by stress-testing of REPACK (CONCURRENTLY) in
    branch 19, so add a test case that tickles it using that feature in that
    branch and master.  However, the bug exists in all branches, and in
    branch 18 we can add a test with regular logical decoding using
    injection points that tickles it.  Unfortunately that test cannot be
    backpatched to earlier branches for lack of facilities.
    
    Reported-by: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
    Author: Antonin Houska <ah@cybertec.at>
    Author: Rui Zhao <zhaorui126@gmail.com>
    Backpatch-through: 14
    Discussion: https://postgr.es/m/85833.1768840165@localhost
    Discussion: https://postgr.es/m/CADzfLwU78as45To9a%3D-Qkr5jEg3tMxc5rUtdKy2MTv4r_SDGng%40mail.gmail.com


 src/backend/replication/logical/snapbuild.c | 50 +++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 67f0b9e089a..bcee0896500 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -485,6 +485,12 @@ SnapBuildSnapDecRefcount(Snapshot snap)
  * In-progress transactions with catalog access are *not* allowed to modify
  * these snapshots; they have to copy them and fill in appropriate ->curcid
  * and ->subxip/subxcnt values.
+ *
+ * Note that some of the transactions in the returned snapshot->xip might not
+ * have finished committing yet (we saw their commit WAL records, but we don't
+ * know if they've removed themselves from procarray).  Callers that want to
+ * use the returned snapshot as an MVCC one must wait for this to happen; see
+ * SnapBuildInitialSnapshot().
  */
 static Snapshot
 SnapBuildBuildSnapshot(SnapBuild *builder)
@@ -590,9 +596,42 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	snap = SnapBuildBuildSnapshot(builder);
 
 	/*
-	 * We know that snap->xmin is alive, enforced by the logical xmin
-	 * mechanism. Due to that we can do this without locks, we're only
-	 * changing our own value.
+	 * Transactions appear in snap->xip as soon as their commit WAL records
+	 * are decoded, but that doesn't imply they can be considered committed in
+	 * a regular MVCC snapshot: that happens only when they remove themselves
+	 * from procarray.  Wait for this to happen; otherwise somebody using the
+	 * snapshot might set hint bits incorrectly.
+	 *
+	 * This is not needed during recovery, because the decoded commit record
+	 * has been replayed already.
+	 */
+	if (!RecoveryInProgress())
+	{
+		RunningTransactions running;
+
+		running = GetRunningTransactionData();
+		LWLockRelease(ProcArrayLock);
+		LWLockRelease(XidGenLock);
+
+		/*
+		 * Note we walk the array up to running->xcnt only, omitting this wait
+		 * for subtransactions: the subxacts are covered by their top-level
+		 * transaction already, there's no need for an additional wait.
+		 */
+		for (int i = 0; i < running->xcnt; i++)
+		{
+			TransactionId running_xid = running->xids[i];
+
+			if (bsearch(&running_xid, snap->xip, snap->xcnt,
+						sizeof(TransactionId), xidComparator) != NULL)
+				XactLockTableWait(running_xid, NULL, NULL, XLTW_None);
+		}
+	}
+
+	/*
+	 * Building an initial snapshot is expensive and an unenforced xmin
+	 * horizon would have bad consequences, therefore always double-check that
+	 * the horizon is enforced.
 	 */
 #ifdef USE_ASSERT_CHECKING
 	{
@@ -606,6 +645,11 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	}
 #endif
 
+	/*
+	 * We know that snap->xmin is alive, enforced by the logical xmin
+	 * mechanism. Due to that we can do this without locks, we're only
+	 * changing our own value.
+	 */
 	MyProc->xmin = snap->xmin;
 
 	/* allocate in transaction context */


[parent: c84dd31bd9f5]