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

From: Yura Sokolov <[email-masked]>
Date: Sun, 13 Feb 2022 17:35:38 +0300

В Пн, 07/02/2022 в 13:52 +0530, Dilip Kumar пишет:
> On Mon, Jan 31, 2022 at 11:50 AM Kyotaro Horiguchi
> <[email-masked]> wrote:
> > At Mon, 17 Jan 2022 09:27:14 +0530, Dilip Kumar <[email-masked]> wrote in
> > 
> > me> Mmm. The size of the array cannot be larger than the numbers the
> > me> *Connt() functions return.  Thus we cannot attach the oversized array
> > me> to ->subxip.  (I don't recall clearly but that would lead to assertion
> > me> failure somewhere..)
> > 
> > Then, I fixed the v3 error and post v4.
> 
> Yeah you are right, SetTransactionSnapshot() has that assertion.
> Anyway after looking again it appears that
> GetMaxSnapshotSubxidCount is the correct size because this is
> PGPROC_MAX_CACHED_SUBXIDS +1, i.e. it considers top transactions as
> well so we don't need to add them separately.
> 
> > SnapBUildInitialSnapshot tries to store XIDS of both top and sub
> > transactions into snapshot->xip array but the array is easily
> > overflowed and CREATE_REPLICATOIN_SLOT command ends with an error.
> > 
> > To fix this, this patch is doing the following things.
> > 
> > - Use subxip array instead of xip array to allow us have larger array
> >   for xids.  So the snapshot is marked as takenDuringRecovery, which
> >   is a kind of abuse but largely reduces the chance of getting
> >   "initial slot snapshot too large" error.
> 
> Right. I think the patch looks fine to me.
> 

Good day.

I've looked to the patch. Personally I'd prefer dynamically resize
xip array. But I think there is issue with upgrade if replica source
is upgraded before destination, right?

Concerning patch, I think more comments should be written about new
usage case for `takenDuringRecovery`. May be this field should be renamed
at all?

And there are checks for `takenDuringRecovery` in `heapgetpage` and
`heapam_scan_sample_next_tuple`. Are this checks affected by the change?
Neither the preceding discussion nor commit message answer me.

-------

regards

Yura Sokolov
Postgres Professional
[email-masked]
[email-masked]

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 01 Apr 2022 14:44:30 +0900 (JST)

At Sun, 13 Feb 2022 17:35:38 +0300, Yura Sokolov <[email-masked]> wrote in 
> is upgraded before destination, right?

I don't think it is relevant.  I think we don't convey snapshot via
streaming replication.  But I think that expanding xip or subxip is
wrong, since it is tied with ProcArray structure. (Even though we
abuse the arrays in some situations, like this).

> usage case for `takenDuringRecovery`. May be this field should be renamed

I don't feel the need to rename it so much. It just signals that "this
snapshot is in the form for recovery".  The more significant reason is
that I don't come up better name:p

And the comment is slightly modified and gets a pointer to detailed
comment.

+	 * Although this snapshot is not acutally taken during recovery, all XIDs
+	 * are stored in subxip. See GetSnapshotData for the details of that form
+	 * of snapshot.

> Neither the preceding discussion nor commit message answer me.

The snapshot works correctly, but for the heapgetpage case, it foces
all_visible to be false.  That unnecessarily prevents visibility check
from skipping.

An annoying thing in SnapBuildInitialSnapshot is that we don't know
the number of xids before looping over the xid range, and we don't
want to bother sorting top-level xids and subxids unless we have to do
so.

Is it better that we hassle in SnapBuildInitialSnapshot to create a
!takenDuringRecovery snapshot?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 01 Apr 2022 15:53:16 +0900 (JST)

At Fri, 01 Apr 2022 14:44:30 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> !takenDuringRecovery snapshot?

So this is that. v5 creates a regular snapshot.

By the way, is there any chance this could be committed to 15?
Otherwise I'll immediately move this to the next CF.

regards.

>From 6f3cfb58bc2c6294124831d6b410b0e51d067fb2 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 31 Jan 2022 15:03:33 +0900
Subject: [PATCH v5] Create correct snapshot during CREATE_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 easily fails with "initial slot snapshot too large" error.

We could instead create a "takenDuringRecovery" snapshot, which later
leads to unnecessary visibility checks. Therefore we take trouble to
create a regular snapshot by identifying whether each xids is
top-level and storing it in the appropriate xid array.

Author: Dilip Kumar and Kyotaro Horiguchi
---
 .../replication/logical/reorderbuffer.c       | 20 +++++++++
 src/backend/replication/logical/snapbuild.c   | 45 +++++++++++++++----
 src/include/replication/reorderbuffer.h       |  1 +
 3 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index c2d9be81fa..42adbe5f15 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3658,6 +3658,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 83fca8a77d..fa08e406e3 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -532,7 +532,10 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	Snapshot	snap;
 	TransactionId xid;
 	TransactionId *newxip;
-	int			newxcnt = 0;
+	TransactionId *newsubxip;
+	int			newxcnt;
+	int			newsubxcnt;
+	bool		overflowed = false;
 
 	Assert(!FirstSnapshotSet);
 	Assert(XactIsoLevel == XACT_REPEATABLE_READ);
@@ -568,9 +571,17 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 
 	MyProc->xmin = snap->xmin;
 
-	/* allocate in transaction context */
+	/*
+	 * Allocate in transaction context.	
+	 *
+	 * We could use only subxip to store all xids (takenduringrecovery
+	 * snapshot) but that causes useless visibility checks later so we hasle to
+	 * create a normal snapshot.
+	 */
 	newxip = (TransactionId *)
 		palloc(sizeof(TransactionId) * GetMaxSnapshotXidCount());
+	newsubxip = (TransactionId *)
+		palloc(sizeof(TransactionId) * GetMaxSnapshotSubxidCount());
 
 	/*
 	 * snapbuild.c builds transactions in an "inverted" manner, which means it
@@ -578,6 +589,9 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	 * classical snapshot by marking all non-committed transactions as
 	 * in-progress. This can be expensive.
 	 */
+	newxcnt = 0;
+	newsubxcnt = 0;
+
 	for (xid = snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax);)
 	{
 		void	   *test;
@@ -591,12 +605,24 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 
 		if (test == NULL)
 		{
-			if (newxcnt >= GetMaxSnapshotXidCount())
-				ereport(ERROR,
-						(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
-						 errmsg("initial slot snapshot too large")));
-
-			newxip[newxcnt++] = xid;
+			/* Store the xid to the appropriate xid array */
+			if (ReorderBufferXidIsKnownSubXact(builder->reorder, xid))
+			{
+				if (!overflowed)
+				{
+					if (newsubxcnt >= GetMaxSnapshotSubxidCount())
+						overflowed = true;
+					else
+						newsubxip[newsubxcnt++] = xid;
+				}
+			}
+			else
+			{
+				if (newxcnt >= GetMaxSnapshotXidCount())
+					elog(ERROR,
+						 "too many transactions while creating snapshot");
+				newxip[newxcnt++] = xid;
+			}
 		}
 
 		TransactionIdAdvance(xid);
@@ -606,6 +632,9 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	snap->snapshot_type = SNAPSHOT_MVCC;
 	snap->xcnt = newxcnt;
 	snap->xip = newxip;
+	snap->subxcnt = newsubxcnt;
+	snap->subxip = newsubxip;
+	snap->suboverflowed = overflowed;
 
 	return snap;
 }
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 0bcc150b33..b26c34ce9d 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -707,6 +707,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,
-- 
2.27.0

---

From: Jacob Champion <[email-masked]>
Date: Tue, 5 Jul 2022 11:32:42 -0700

> So this is that. v5 creates a regular snapshot.

This patch will need a quick rebase over 905c020bef9, which added
`extern` to several missing locations.

Thanks,
--Jacob

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 19 Jul 2022 11:55:06 +0900 (JST)

At Tue, 5 Jul 2022 11:32:42 -0700, Jacob Champion <[email-masked]> wrote in 
> `extern` to several missing locations.

Thanks! Just rebased.

regards.

---

From: Robert Haas <[email-masked]>
Date: Fri, 9 Sep 2022 13:19:14 -0400

> Thanks! Just rebased.

Hi,

I mentioned this patch to Andres in conversation, and he expressed a
concern that there might be no guarantee that we retain enough CLOG to
look up XIDs. Presumably this wouldn't be an issue when the snapshot
doesn't get marked suboverflowed, but what if it does?

Adding Andres in the hopes that he may comment further.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Andres Freund <[email-masked]>
Date: Mon, 12 Sep 2022 14:51:56 -0700

Hi,

Thanks for working on this!

I think this should include a test that fails without this change and succeeds
with it...

On 2022-07-19 11:55:06 +0900, Kyotaro Horiguchi wrote:
> From abcf0a0e0b3e2de9927d8943a3e3c145ab189508 Mon Sep 17 00:00:00 2001
> Date: Tue, 19 Jul 2022 11:50:29 +0900
> Subject: [PATCH v6] Create correct snapshot during CREATE_REPLICATION_SLOT

This sees a tad misleading - the previous snapshot wasn't borken, right?

> +	if (txn && rbtxn_is_known_subxact(txn))

The comments here just seem to restate the code....

It's not obvious to me that it's the right design (or even correct) to ask
reorderbuffer about an xid being a subxid. Maybe I'm missing something, but
why would reorderbuffer even be guaranteed to know about all these subxids?

> +	 * create a normal snapshot.

I can't really parse this comment at this point, and I seriously doubt I could
later on.

> +				newxip[newxcnt++] = xid;

Hm, this is starting to be pretty deeply nested...

I wonder if a better fix here wouldn't be to allow importing a snapshot with a
larger ->xid array. Yes, we can't do that in CurrentSnapshotData, but IIRC we
need to be in a transactional snapshot anyway, which is copied anyway?

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Mon, 12 Sep 2022 14:53:17 -0700

Hi,

On 2022-09-09 13:19:14 -0400, Robert Haas wrote:
> look up XIDs.

I was concerned we wouldn't keep enough subtrans, rather than clog. But I
think we're ok, because we need to have an appropriate ->xmin for exporting /
importing the snapshot.

Greetings,

Andres Freund

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 13 Sep 2022 07:00:42 +0530

> why would reorderbuffer even be guaranteed to know about all these subxids?

Yeah, you are right, the reorderbuffer will only know about the
transaction for which changes got added to the reorder buffer.  So
this seems not to be the right design idea.

> need to be in a transactional snapshot anyway, which is copied anyway?

Yeah when I first found this issue, I thought that should be the
solution.  But later it went in a different direction.

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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 13 Sep 2022 15:22:37 +0900 (JST)

Thanks for raizing this up, Robert and the comment, Andres.

At Tue, 13 Sep 2022 07:00:42 +0530, Dilip Kumar <[email-masked]> wrote in 
> this seems not to be the right design idea.

That function is called after the SnapBuild reaches
SNAPBUILD_CONSISTENT state ,or SnapBuildInitialSnapshot() rejects
other than that state. That is, IIUC the top-sub relationship of all
the currently running transactions is fully known to reorder buffer.
We need a comment about that.

> solution.  But later it went in a different direction.

I think that that is the best solution if rbtxn_is_known_subxzact() is
known to be unreliable at the time.

regards.

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 13 Sep 2022 12:08:18 +0530

> We need a comment about that.

I don't think this assumption is true, any xid started after switching
to the SNAPBUILD_FULL_SNAPSHOT and before switching to the
SNAPBUILD_CONSISTENT, might still be in progress so we can not
identify whether they are subxact or not from reorder buffer.

refer to this comment:
/*
* c) transition from FULL_SNAPSHOT to CONSISTENT.
*
* In FULL_SNAPSHOT state (see d) ), and this xl_running_xacts'
* oldestRunningXid is >= than nextXid from when we switched to
* FULL_SNAPSHOT. This means all transactions that are currently in
* progress have a catalog snapshot, and all their changes have been
* collected. Switch to CONSISTENT.
*/

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 13 Sep 2022 15:45:07 +0900 (JST)

At Mon, 12 Sep 2022 14:51:56 -0700, Andres Freund <[email-masked]> wrote in 
> This sees a tad misleading - the previous snapshot wasn't borken, right?

I saw it kind of broken that ->xip contains sub transactions.  But I
didn't meant it's broken by "correct". Is "proper" suitable there?

> The comments here just seem to restate the code....

Yeah, it is pulled from the existing code but result looks like so..

> why would reorderbuffer even be guaranteed to know about all these subxids?

I think you're missing that the code is visited only after the reorder
buffer's state becomes SNAPBUILD_CONSISTENT. I think
rbtxn_is_known_subxact() is reliable at that stage.

> I can't really parse this comment at this point, and I seriously doubt I could

Mmm. The "takenduringrecovery" is relly perplexing (it has been
somehow lower-cased..), but after removing the parenthesized part, it
looks like this. And it had a misspelling but I removed that word.  Is
this still unreadable?

We could use only subxip to store all xids but that causes useless
visibility checks later so we create a normal snapshot.

> Hm, this is starting to be pretty deeply nested...

Yeah, at least one if() is removable.

> need to be in a transactional snapshot anyway, which is copied anyway?

The other reason for oversized xip array is it causes visibility check
when it is used. AFAICS XidInMVCCSnapshot has additional path for
takenDuringRecovery snapshots that contains a linear search (currently
it is replaced by pg_lfind32()). This saves us from doing this for
that snapshot.

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 13 Sep 2022 16:10:59 +0900 (JST)

At Tue, 13 Sep 2022 12:08:18 +0530, Dilip Kumar <[email-masked]> wrote in 
> identify whether they are subxact or not from reorder buffer.

Yeah, I misunderstood that the relationship is recorded earlier
(how?).  Thus it is not reliable in the first place.

I agree that the best way is oversized xip. 

By the way, I feel that "is >= than" is redundant or plain wrong..

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 13 Sep 2022 16:15:34 +0900 (JST)

At Tue, 13 Sep 2022 15:45:07 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> didn't meant it's broken by "correct". Is "proper" suitable there?

No. It's not broken if it is takenDuringRecovery.  So this flag can be
used to notify that xip can be oversized.

I realized that rbtxn_is_known_subxact is not reliable. I'm
redirecting to oversized xip. Pleas wait for a while.

regareds.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 13 Sep 2022 16:30:06 +0900 (JST)

At Tue, 13 Sep 2022 16:10:59 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 

By the way GetSnapshotData() does this:

>		snapshot->subxip = (TransactionId *)
>			malloc(GetMaxSnapshotSubxidCount() * sizeof(TransactionId));
...
>	if (!snapshot->takenDuringRecovery)
...
>	else
>	{
>		subcount = KnownAssignedXidsGetAndSetXmin(snapshot->subxip, &xmin,
>												  xmax);

It is possible that the subxip is overrun. We need to expand the array
somehow. Or assign the array of the size (GetMaxSnapshotXidCount() +
GetMaxSnapshotSubxidCount()) for takenDuringRecovery snapshots.

(I feel deja vu..)

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 13 Sep 2022 16:31:22 +0900 (JST)

Sigh..

At Tue, 13 Sep 2022 16:30:06 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> GetMaxSnapshotSubxidCount()) for takenDuringRecovery snapshots.

And I found that this is already done.  What we should is doing the
same thing in snapbuild.

Sorry for the noise..

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 13 Sep 2022 17:31:05 +0900 (JST)

At Tue, 13 Sep 2022 16:15:34 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> redirecting to oversized xip. Pleas wait for a while.

However, the reader of saved snapshots (ImportSnapshot) has the
restriction that

>	if (xcnt < 0 || xcnt > GetMaxSnapshotXidCount())
>		ereport(ERROR,

and

>		if (xcnt < 0 || xcnt > GetMaxSnapshotSubxidCount())
>			ereport(ERROR,
 (this xid is subxcnt)

And ExportSnapshot repalces oversized subxip with overflowed.

So even when GetSnapshotData() returns a snapshot with oversized
subxip, it is saved as just "overflowed" on exporting. I don't think
this is the expected behavior since such (no xip and overflowed)
snapshot no longer works.

On the other hand, it seems to me that snapbuild doesn't like
takenDuringRecovery snapshots.

So snapshot needs additional flag signals that xip is oversized and
all xid are holded there. And also need to let takenDuringRecovery
suggest subxip is oversized.

regards.