From: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: Mihail Nikalayeu <mihailnikalayeu@gmail.com>,
    Amit Kapila <amit.kapila16@gmail.com>,
    Andres Freund <andres@anarazel.de>,
    Srinath Reddy Sadipiralla <srinath2133@gmail.com>,
    Matthias van de Meent <boekewurm+postgres@gmail.com>,
    Pg Hackers <pgsql-hackers@lists.postgresql.org>,
    Robert Treat <rob@xzilla.net>
Subject: Re: Adding REPACK [concurrently]
In-reply-to: <afoF8ypBbknkp8_b@alvherre.pgsql>
References: <afoF8ypBbknkp8_b@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
   message dated "Tue, 05 May 2026 17:02:39 +0200."
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Date: Wed, 06 May 2026 10:25:44 +0200
Message-ID: <77611.1778055944@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/77611.1778055944%40localhost>
Precedence: bulk

--=-=-=
Content-Type: text/plain

Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> On 2026-May-05, Antonin Houska wrote:
> 
> > However, I failed to notice that COMMIT record of
> > a transaction listed in the xl_running_xacts WAL record is not guaranteed to
> > follow the xl_running_xacts record in WAL. In other words, even if
> > xl_running_xacts is created before a COMMIT record of the contained
> > transaction, it may end up at higher LSN in WAL. So the cleanup I relied on
> > might not take place.
> 
> That's pretty bad news.
> 
> > I've got no good idea how to fix that.

One idea occurred to me yet, effectively it's just a cleanup. Part of it was
already proposed [1].

[1] https://www.postgresql.org/message-id/flat/CAHg%2BQDcQak4jx_6X2_Ws98rzG%3DxBARLjqm_%3D56wTRUtNsY4DZQ%40mail.gmail.com

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=0001-Distinguish-properly-when-database-specific-transact.patch

From 33715b1fbba9131744864f23d0310070eaa9c91d Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Wed, 6 May 2026 09:38:11 +0200
Subject: [PATCH] Distinguish properly when database-specific transaction list
 should be used.

Currently, decoding session that relies on database-specific xl_running_xacts
WAL records can also use some information of cluster-wide records and vice
versa. Although this approach might reduce the total number of
xl_running_xacts records, it's simply not correct.

SnapBuildProcessRunningXacts() now decides at the very beginning whether
particular record can be used or not.
---
 src/backend/replication/logical/snapbuild.c | 55 +++++++++++++--------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index c8309b96ed4..b9661b7d70a 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1157,6 +1157,39 @@ SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, xl_running_xact
 	ReorderBufferTXN *txn;
 	TransactionId xmin;
 
+	/*
+	 * Each decoding session should use either cluster-wide snapshots or
+	 * database-specific ones, but not both. Since the latter can have more
+	 * recent value of oldestRunningXid, builder->xmin could go backwards if
+	 * it was followed by cluster-wide snapshot - see the ->xmin adjustment
+	 * below.
+	 */
+	if (!db_specific && OidIsValid(running->dbid))
+		return;
+
+	if (db_specific)
+	{
+		/*
+		 * Make sure that we have the snapshots needed for startup, as well as
+		 * those for regular cleanup: each time the cluster-wide snapshot is
+		 * created (typically on slot creation or by checkpointer), the
+		 * database-specific snapshot is requested here if the current session
+		 * needs it.
+		 */
+		if (!OidIsValid(running->dbid))
+		{
+			LogStandbySnapshot(MyDatabaseId);
+
+			return;
+		}
+		/*
+		 * Snapshots issued for other databases do not contain the information
+		 * about transactions in our database.
+		 */
+		else if (running->dbid != MyDatabaseId)
+			return;
+	}
+
 	/*
 	 * If we're not consistent yet, inspect the record to see whether it
 	 * allows to get closer to being consistent. If we are consistent, dump
@@ -1171,17 +1204,7 @@ SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, xl_running_xact
 		 */
 		if (db_specific)
 		{
-			/*
-			 * If we must only keep track of transactions running in the
-			 * current database, we need transaction info from exactly that
-			 * database.
-			 */
-			if (running->dbid != MyDatabaseId)
-			{
-				LogStandbySnapshot(MyDatabaseId);
-
-				return;
-			}
+			Assert(running->dbid == MyDatabaseId);
 
 			/*
 			 * We'd better be able to check during scan if the plugin does not
@@ -1198,16 +1221,6 @@ SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, xl_running_xact
 	else
 		SnapBuildSerialize(builder, lsn);
 
-	/*
-	 * Database specific transaction info may exist to reach CONSISTENT state
-	 * faster, however the code below makes no use of it. Moreover, such
-	 * record might cause problems because the following normal (cluster-wide)
-	 * record can have lower value of oldestRunningXid. In that case, let's
-	 * wait with the cleanup for the next regular cluster-wide record.
-	 */
-	if (OidIsValid(running->dbid))
-		return;
-
 	/*
 	 * Update range of interesting xids based on the running xacts
 	 * information. We don't increase ->xmax using it, because once we are in
-- 
2.47.3


--=-=-=--