Subject: Error "initial slot snapshot too large" in create replication slot

From: Dilip Kumar <[email-masked]>
Date: Mon, 11 Oct 2021 11:49:41 +0530

While creating an "export snapshot" I don't see any protection why the
number of xids in the snapshot can not cross the
"GetMaxSnapshotXidCount()"?.

Basically, while converting the HISTORIC snapshot to the MVCC snapshot
in "SnapBuildInitialSnapshot()", we add all the xids between
snap->xmin to snap->xmax to the MVCC snap->xip array (xids for which
commit were not recorded).  The problem is that we add both topxids as
well as the subxids into the same array and expect that the "xid"
count does not cross the "GetMaxSnapshotXidCount()".  So it seems like
an issue but I am not sure what is the fix for this, some options are
a) Don't limit the xid count in the exported snapshot and dynamically
resize the array b) Increase the limit to GetMaxSnapshotXidCount() +
GetMaxSnapshotSubxidCount().  But in option b) there would still be a
problem that how do we handle the overflowed subtransaction?

I have locally, reproduced the issue,

1. Configuration
max_connections= 5
autovacuum = off
max_worker_processes = 0

2.Then from pgbench I have run the attached script (test.sql) from 5 clients.
./pgbench -i postgres
./pgbench -c4 -j4 -T 3000 -f test1.sql -P1 postgres

3. Concurrently, create replication slot,
[dilipkumar@localhost bin]$ ./psql "dbname=postgres replication=database"
postgres[7367]=#
postgres[6463]=# CREATE_REPLICATION_SLOT "slot" LOGICAL "test_decoding";
ERROR:  40001: initial slot snapshot too large
LOCATION:  SnapBuildInitialSnapshot, snapbuild.c:597
postgres[6463]=# CREATE_REPLICATION_SLOT "slot" LOGICAL "test_decoding";
ERROR:  XX000: clearing exported snapshot in wrong transaction state
LOCATION:  SnapBuildClearExportedSnapshot, snapbuild.c:690

I could reproduce this issue, at least once in 8-10 attempts of
creating the replication slot.

Note:  After that issue, I have noticed one more issue "clearing
exported snapshot in wrong transaction state", that is because the
"ExportInProgress" is not cleared on the transaction abort, for this,
a simple fix is we can clear this state on the transaction abort,
maybe I will raise this as a separate issue?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 11 Oct 2021 19:59:37 +0900 (JST)

At Mon, 11 Oct 2021 11:49:41 +0530, Dilip Kumar <[email-masked]> wrote in 
> an issue but I am not sure what is the fix for this, some options are

It seems to me that it is a compromise between the restriction of the
legitimate snapshot and snapshots created by snapbuild.  If the xids
overflow, the resulting snapshot may lose a siginificant xid, i.e, a
top-level xid.

> problem that how do we handle the overflowed subtransaction?

I'm afraid that we shouldn't expand the size limits.  If I understand
it correctly, we only need the top-level xids in the exported snapshot
and reorder buffer knows whether a xid is a top-level or not after
establishing a consistent snapshot.

The attached diff tries to make SnapBuildInitialSnapshot exclude
subtransaction from generated snapshots.  It seems working fine for
you repro. (Of course, I'm not confident that it is the correct thing,
though..)

What do you think about this?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 46e66608cf..4e452cce7c 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3326,6 +3326,26 @@ ReorderBufferXidHasBaseSnapshot(ReorderBuffer *rb, TransactionId xid)
 }

+/*
+ * ReorderBufferXidIsKnownSubXact
+ *		Returns true if the xid is a known subtransaction.
+ */
+bool
+ReorderBufferXidIsKnownSubXact(ReorderBuffer *rb, TransactionId xid)
+{
+	ReorderBufferTXN *txn;
+
+	txn = ReorderBufferTXNByXid(rb, xid, false,
+								NULL, InvalidXLogRecPtr, false);
+
+	/* a known subtxn? */
+	if (txn && rbtxn_is_known_subxact(txn))
+		return true;
+
+	return false;
+}
+
+
 /*
  * ---------------------------------------
  * Disk serialization support
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index a5333349a8..12d283f4de 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -591,12 +591,18 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 
 		if (test == NULL)
 		{
-			if (newxcnt >= GetMaxSnapshotXidCount())
-				ereport(ERROR,
-						(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
-						 errmsg("initial slot snapshot too large")));
+			bool issubxact =
+				ReorderBufferXidIsKnownSubXact(builder->reorder, xid);
 
-			newxip[newxcnt++] = xid;
+			if (!issubxact)
+			{				
+				if (newxcnt >= GetMaxSnapshotXidCount())
+					ereport(ERROR,
+							(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+							 errmsg("initial slot snapshot too large")));
+				
+				newxip[newxcnt++] = xid;
+			}
 		}
 
 		TransactionIdAdvance(xid);
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 5b40ff75f7..e5fa1051d7 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -669,6 +669,7 @@ void		ReorderBufferXidSetCatalogChanges(ReorderBuffer *, TransactionId xid, XLog
 bool		ReorderBufferXidHasCatalogChanges(ReorderBuffer *, TransactionId xid);
 bool		ReorderBufferXidHasBaseSnapshot(ReorderBuffer *, TransactionId xid);
 
+bool		ReorderBufferXidIsKnownSubXact(ReorderBuffer *rb, TransactionId xid);
 bool		ReorderBufferRememberPrepareInfo(ReorderBuffer *rb, TransactionId xid,
 											 XLogRecPtr prepare_lsn, XLogRecPtr end_lsn,
 											 TimestampTz prepare_time,

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 11 Oct 2021 16:48:10 +0530

> it correctly, we only need the top-level xids in the exported snapshot

But since we are using this as an MVCC snapshot, if we don't have the
subxid and if we also don't mark the "suboverflowed" flag then IMHO
the sub-transaction visibility might be wrong, Am I missing something?

> What do you think about this?

If your statement that we only need top-xids in the exported snapshot,
is true then this fix looks fine to me.   If not then we might need to
add the sub-xids in the snapshot->subxip array and if it crosses the
limit of GetMaxSnapshotSubxidCount(), then we can mark "suboverflowed"
flag.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 12 Oct 2021 13:59:59 +0900 (JST)

At Mon, 11 Oct 2021 16:48:10 +0530, Dilip Kumar <[email-masked]> wrote in 
> the sub-transaction visibility might be wrong, Am I missing something?

Sorry I should have set suboverflowed in the generated snapshot.
However, we can store subxid list as well when the snapshot (or
running_xact) is not overflown. These (should) works the same way.

On physical standby, snapshot is created just filling up only subxip
with all top and sub xids (procrray.c:2400).  It would be better we do
the same thing here.

> limit of GetMaxSnapshotSubxidCount(), then we can mark "suboverflowed"

So I came up with the attached version.

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 12 Oct 2021 14:05:38 +0900 (JST)

At Tue, 12 Oct 2021 13:59:59 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> So I came up with the attached version.

Sorry, it was losing a piece of change.

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 12 Oct 2021 11:30:43 +0530

> Sorry, it was losing a piece of change.

Yeah, at a high level this is on the idea I have in mind, I will do a
detailed review in a day or two.  Thanks for working on this.

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 19 Oct 2021 14:25:07 +0530

> detailed review in a day or two.  Thanks for working on this.

While doing the detailed review, I think there are a couple of
problems with the patch,  the main problem of storing all the xid in
the snap->subxip is that once we mark the snapshot overflown then the
XidInMVCCSnapshot, will not search the subxip array, instead it will
fetch the topXid and search in the snap->xip array.  Another issue is
that the total xids could be GetMaxSnapshotSubxidCount()
+GetMaxSnapshotXidCount().

I think what we should be doing is that if the xid is know subxid then
add in the snap->subxip array otherwise in snap->xip array.  So
snap->xip array size will be GetMaxSnapshotXidCount() whereas the
snap->subxip array size will be GetMaxSnapshotSubxidCount().  And if
the array size is full then we can stop adding the subxids to the
array.

What is your thought on this?

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 2 Nov 2021 16:40:39 +0530

On Tue, Oct 19, 2021 at 2:25 PM Dilip Kumar <[email-masked]> wrote:
> fetch the topXid and search in the snap->xip array.

I missed that you are marking the snapshot as takenDuringRecovery so
your fix looks fine.

Another issue is
> +GetMaxSnapshotXidCount().

I have fixed this, the updated patch is attached.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From b6ca7a05b05a9244bf400d14b75dfcd1f99c76cd Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 12 Oct 2021 13:53:27 +0900
Subject: [PATCH v3] Allow overflowed snapshot when creating logical
 replication slot

Snapshot can hold top XIDs up to the number of server processes but
SnapBuildInitialSnapshot tries to store all top-level and sub XIDs to
there and fails for certain circumstances. Instead, create a
"takenDuringRecovery" snapshot instead, which stores all XIDs in
subxip array.  Addition to that, allow to create an overflowed
snapshot by adding to reorder buffer a function to tell whether an XID
is a top-level or not.
---
 src/backend/replication/logical/reorderbuffer.c | 20 +++++++++
 src/backend/replication/logical/snapbuild.c     | 57 +++++++++++++++++++------
 src/include/replication/reorderbuffer.h         |  1 +
 3 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 46e6660..4e452cc 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3327,6 +3327,26 @@ ReorderBufferXidHasBaseSnapshot(ReorderBuffer *rb, TransactionId xid)

 /*
+ * ReorderBufferXidIsKnownSubXact
+ *		Returns true if the xid is a known subtransaction.
+ */
+bool
+ReorderBufferXidIsKnownSubXact(ReorderBuffer *rb, TransactionId xid)
+{
+	ReorderBufferTXN *txn;
+
+	txn = ReorderBufferTXNByXid(rb, xid, false,
+								NULL, InvalidXLogRecPtr, false);
+
+	/* a known subtxn? */
+	if (txn && rbtxn_is_known_subxact(txn))
+		return true;
+
+	return false;
+}
+
+
+/*
  * ---------------------------------------
  * Disk serialization support
  * ---------------------------------------
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index dbdc172..9fd28d4 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -531,8 +531,9 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 {
 	Snapshot	snap;
 	TransactionId xid;
-	TransactionId *newxip;
-	int			newxcnt = 0;
+	TransactionId *newsubxip;
+	int			maxxidcnt;
+	bool		overflowed = false;
 
 	Assert(!FirstSnapshotSet);
 	Assert(XactIsoLevel == XACT_REPEATABLE_READ);
@@ -568,9 +569,13 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 
 	MyProc->xmin = snap->xmin;
 
-	/* allocate in transaction context */
-	newxip = (TransactionId *)
-		palloc(sizeof(TransactionId) * GetMaxSnapshotXidCount());
+	/*
+	 * Allocate in transaction context. We use only subxip to store xids. See
+	 * GetSnapshotData for details.
+	 */
+	newsubxip = (TransactionId *) palloc(sizeof(TransactionId) *
+										 (GetMaxSnapshotXidCount() +
+										  GetMaxSnapshotSubxidCount()));
 
 	/*
 	 * snapbuild.c builds transactions in an "inverted" manner, which means it
@@ -578,6 +583,9 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	 * classical snapshot by marking all non-committed transactions as
 	 * in-progress. This can be expensive.
 	 */
+retry:
+	newsubxcnt = 0;
+
 	for (xid = snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax);)
 	{
 		void	   *test;
@@ -591,12 +599,29 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 
 		if (test == NULL)
 		{
+			bool add = true;
 
-			newxip[newxcnt++] = xid;
+			/* exlude subxids when subxip is overflowed */
+			if (overflowed &&
+				ReorderBufferXidIsKnownSubXact(builder->reorder, xid))
+				add = false;
+
+			if (add)
+			{
+				if (newsubxcnt >= GetMaxSnapshotSubxidCount())
+				{
+					if (overflowed)
+						ereport(ERROR,
+								(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+								 errmsg("initial slot snapshot too large")));
+
+					/* retry excluding subxids */
+					overflowed = true;
+					goto retry;
+				}
+
+				newsubxip[newsubxcnt++] = xid;
+			}
 		}
 
 		TransactionIdAdvance(xid);
@@ -604,8 +629,16 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 
 	/* adjust remaining snapshot fields as needed */
 	snap->snapshot_type = SNAPSHOT_MVCC;
-	snap->xcnt = newxcnt;
-	snap->xip = newxip;
+	snap->xcnt = 0;
+	snap->subxcnt = newsubxcnt;
+	snap->subxip = newsubxip;
+	snap->suboverflowed = overflowed;
+
+	/*
+	 * Although this snapshot is taken actually not during recovery, all XIDs
+	 * are stored in subxip even if it is not overflowed.
+	 */
+	snap->takenDuringRecovery = true;
 
 	return snap;
 }
index 5b40ff7..e5fa105 100644
 
1.8.3.1