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

At Mon, 11 Oct 2021 11:49:41 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

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

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.

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'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

On Mon, Oct 11, 2021 at 4:29 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 11 Oct 2021 11:49:41 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

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

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.

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'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

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?

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?

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.

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

At Mon, 11 Oct 2021 16:48:10 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

On Mon, Oct 11, 2021 at 4:29 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 11 Oct 2021 11:49:41 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

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

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.

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'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

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?

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.

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?

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.

So I came up with the attached version.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 12 Oct 2021 13:59:59 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

So I came up with the attached version.

Sorry, it was losing a piece of change.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Oct 12, 2021 at 10:35 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Tue, 12 Oct 2021 13:59:59 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

So I came up with the attached version.

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.

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

On Tue, Oct 12, 2021 at 11:30 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Oct 12, 2021 at 10:35 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Tue, 12 Oct 2021 13:59:59 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

So I came up with the attached version.

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.

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?

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

On Tue, Oct 19, 2021 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Oct 12, 2021 at 11:30 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Oct 12, 2021 at 10:35 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Tue, 12 Oct 2021 13:59:59 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

So I came up with the attached version.

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.

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.

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

Another issue is

that the total xids could be GetMaxSnapshotSubxidCount()
+GetMaxSnapshotXidCount().

I have fixed this, the updated patch is attached.

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

Hi,

On Tue, Nov 02, 2021 at 04:40:39PM +0530, Dilip Kumar wrote:

I have fixed this, the updated patch is attached.

The cfbot reports that this patch doesn't compile:
https://cirrus-ci.com/task/5642000073490432?logs=build

[03:01:24.477] snapbuild.c: In function ‘SnapBuildInitialSnapshot’:
[03:01:24.477] snapbuild.c:587:2: error: ‘newsubxcnt’ undeclared (first use in this function); did you mean ‘newsubxip’?
[03:01:24.477] 587 | newsubxcnt = 0;
[03:01:24.477] | ^~~~~~~~~~
[03:01:24.477] | newsubxip
[03:01:24.477] snapbuild.c:587:2: note: each undeclared identifier is reported only once for each function it appears in
[03:01:24.477] snapbuild.c:535:8: warning: unused variable ‘maxxidcnt’ [-Wunused-variable]
[03:01:24.477] 535 | int maxxidcnt;
[03:01:24.477] | ^~~~~~~~~

Could you send a new version? In the meantime I will switch the patch to
Waiting on Author.

On Wed, Jan 12, 2022 at 4:09 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

Hi,

On Tue, Nov 02, 2021 at 04:40:39PM +0530, Dilip Kumar wrote:

I have fixed this, the updated patch is attached.

The cfbot reports that this patch doesn't compile:
https://cirrus-ci.com/task/5642000073490432?logs=build

[03:01:24.477] snapbuild.c: In function ‘SnapBuildInitialSnapshot’:
[03:01:24.477] snapbuild.c:587:2: error: ‘newsubxcnt’ undeclared (first
use in this function); did you mean ‘newsubxip’?
[03:01:24.477] 587 | newsubxcnt = 0;
[03:01:24.477] | ^~~~~~~~~~
[03:01:24.477] | newsubxip
[03:01:24.477] snapbuild.c:587:2: note: each undeclared identifier is
reported only once for each function it appears in
[03:01:24.477] snapbuild.c:535:8: warning: unused variable ‘maxxidcnt’
[-Wunused-variable]
[03:01:24.477] 535 | int maxxidcnt;
[03:01:24.477] | ^~~~~~~~~

Could you send a new version? In the meantime I will switch the patch to
Waiting on Author.

Thanks for notifying, I will work on this and send the update patch soon.

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

At Tue, 2 Nov 2021 16:40:39 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

On Tue, Oct 19, 2021 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Oct 12, 2021 at 11:30 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Oct 12, 2021 at 10:35 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Tue, 12 Oct 2021 13:59:59 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

So I came up with the attached version.

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.

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.

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

Another issue is

that the total xids could be GetMaxSnapshotSubxidCount()
+GetMaxSnapshotXidCount().

I have fixed this, the updated patch is attached.

Mmm. The size of the array cannot be larger than the numbers the
*Connt() functions return. Thus we cannot attach the oversized array
to ->subxip. (I don't recall clearly but that would lead to assertion
failure somewhere..)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Mon, 17 Jan 2022 09:27:14 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

On Wed, Jan 12, 2022 at 4:09 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

The cfbot reports that this patch doesn't compile:
https://cirrus-ci.com/task/5642000073490432?logs=build

[03:01:24.477] snapbuild.c: In function ‘SnapBuildInitialSnapshot’:
[03:01:24.477] snapbuild.c:587:2: error: ‘newsubxcnt’ undeclared (first
use in this function); did you mean ‘newsubxip’?
[03:01:24.477] 587 | newsubxcnt = 0;
[03:01:24.477] | ^~~~~~~~~~
[03:01:24.477] | newsubxip
[03:01:24.477] snapbuild.c:587:2: note: each undeclared identifier is
reported only once for each function it appears in
[03:01:24.477] snapbuild.c:535:8: warning: unused variable ‘maxxidcnt’
[-Wunused-variable]
[03:01:24.477] 535 | int maxxidcnt;
[03:01:24.477] | ^~~~~~~~~

Could you send a new version? In the meantime I will switch the patch to
Waiting on Author.

Thanks for notifying, I will work on this and send the update patch soon.

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.

To recap:

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.

- Still if subxip is overflowed, retry with excluding subtransactions
then set suboverflowed. This causes XidInMVCCSnapshot (finally)
scans over subxip array for targetted top-level xid.

We could take another way: make a !takenDuringRecovery snapshot by
using xip instead of subxip. It is cleaner but it has far larger
chance of needing to retry.

(renamed the patch since it represented a part of the patch)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jan 31, 2022 at 11:50 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

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.

- Still if subxip is overflowed, retry with excluding subtransactions
then set suboverflowed. This causes XidInMVCCSnapshot (finally)
scans over subxip array for targetted top-level xid.

We could take another way: make a !takenDuringRecovery snapshot by
using xip instead of subxip. It is cleaner but it has far larger
chance of needing to retry.

(renamed the patch since it represented a part of the patch)

Thanks for the updated version. I will look into it this week.

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

On Mon, Jan 31, 2022 at 11:50 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 17 Jan 2022 09:27:14 +0530, Dilip Kumar <dilipbalaut@gmail.com> 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.

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

В Пн, 07/02/2022 в 13:52 +0530, Dilip Kumar пишет:

On Mon, Jan 31, 2022 at 11:50 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 17 Jan 2022 09:27:14 +0530, Dilip Kumar <dilipbalaut@gmail.com> 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
y.sokolov@postgrespro.ru
funny.falcon@gmail.com

At Sun, 13 Feb 2022 17:35:38 +0300, Yura Sokolov <y.sokolov@postgrespro.ru> wrote in

В Пн, 07/02/2022 в 13:52 +0530, Dilip Kumar пишет:

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?

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).

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

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.

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.

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

At Fri, 01 Apr 2022 14:44:30 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Sun, 13 Feb 2022 17:35:38 +0300, Yura Sokolov <y.sokolov@postgrespro.ru> wrote in

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.

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?

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.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Mar 31, 2022 at 11:53 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

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

At Tue, 5 Jul 2022 11:32:42 -0700, Jacob Champion <jchampion@timescale.com> wrote in

On Thu, Mar 31, 2022 at 11:53 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

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! Just rebased.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jul 18, 2022 at 10:55 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

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

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
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
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?

+/*
+ * 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;
+}

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?

@@ -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.
+	 */

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

@@ -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;
+			}
}

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

Hi,

On 2022-09-09 13:19:14 -0400, Robert Haas wrote:

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.

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

On Tue, Sep 13, 2022 at 3:22 AM Andres Freund <andres@anarazel.de> wrote:

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?

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.

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?

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

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

At Tue, 13 Sep 2022 07:00:42 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

On Tue, Sep 13, 2022 at 3:22 AM Andres Freund <andres@anarazel.de> wrote:

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?

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.

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.

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?

Yeah when I first found this issue, I thought that should be the
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.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Sep 13, 2022 at 11:52 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

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

At Tue, 13 Sep 2022 07:00:42 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

On Tue, Sep 13, 2022 at 3:22 AM Andres Freund <andres@anarazel.de> wrote:

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?

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.

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.

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.
*/

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

At Mon, 12 Sep 2022 14:51:56 -0700, Andres Freund <andres@anarazel.de> wrote in

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
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
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?

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?

+/*
+ * 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;
+}

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

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

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?

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.

@@ -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.
+	 */

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

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.

@@ -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;
+			}
}

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

Yeah, at least one if() is removable.

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?

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.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 13 Sep 2022 12:08:18 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

On Tue, Sep 13, 2022 at 11:52 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

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.

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.

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.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 13 Sep 2022 15:45:07 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Mon, 12 Sep 2022 14:51:56 -0700, Andres Freund <andres@anarazel.de> 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?

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.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 13 Sep 2022 16:10:59 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Tue, 13 Sep 2022 12:08:18 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

On Tue, Sep 13, 2022 at 11:52 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

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.

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.

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..

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.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Sigh..

At Tue, 13 Sep 2022 16:30:06 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

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.

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

Sorry for the noise..

regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 13 Sep 2022 16:15:34 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Tue, 13 Sep 2022 15:45:07 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Mon, 12 Sep 2022 14:51:56 -0700, Andres Freund <andres@anarazel.de> 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?

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.

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.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Sep 13, 2022 at 05:31:05PM +0900, Kyotaro Horiguchi wrote:

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.

The discussion seems to have stopped here. As this is classified as a
bug fix, I have moved this patch to next CF, waiting on author for the
moment.
--
Michael

Hi,

On 2022-10-12 14:10:15 +0900, Michael Paquier wrote:

The discussion seems to have stopped here. As this is classified as a
bug fix, I have moved this patch to next CF, waiting on author for the
moment.

FWIW, I view this more as lifting a limitation. I wouldn't want to
backpatch the change.

Greetings,

Andres Freund

On Wed, 12 Oct 2022 at 01:10, Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Sep 13, 2022 at 05:31:05PM +0900, Kyotaro Horiguchi wrote:

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.

The discussion seems to have stopped here. As this is classified as a
bug fix, I have moved this patch to next CF, waiting on author for the
moment.

Kyotoro Horiguchi, any chance you'll be able to work on this for this
commitfest? If so shout (or anyone else is planning to push it over
the line.... Andres?) otherwise I'll move it on to the next release.

--
Gregory Stark
As Commitfest Manager

At Mon, 20 Mar 2023 13:46:51 -0400, "Gregory Stark (as CFM)" <stark.cfm@gmail.com> wrote in

On Wed, 12 Oct 2022 at 01:10, Michael Paquier <michael@paquier.xyz> wrote:

The discussion seems to have stopped here. As this is classified as a
bug fix, I have moved this patch to next CF, waiting on author for the
moment.

Kyotoro Horiguchi, any chance you'll be able to work on this for this
commitfest? If so shout (or anyone else is planning to push it over
the line.... Andres?) otherwise I'll move it on to the next release.

Ugg. sorry for being lazy. I have lost track of the conversation. I'm
currently working on this and will come back soon with a new version.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 22 Mar 2023 14:27:40 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Mon, 20 Mar 2023 13:46:51 -0400, "Gregory Stark (as CFM)" <stark.cfm@gmail.com> wrote in

Kyotoro Horiguchi, any chance you'll be able to work on this for this
commitfest? If so shout (or anyone else is planning to push it over
the line.... Andres?) otherwise I'll move it on to the next release.

Ugg. sorry for being lazy. I have lost track of the conversation. I'm
currently working on this and will come back soon with a new version.

I relized that attempting to make SnapshotData.xip expansible was
making procarray.c and snapmgr.c too complicated. The main reason is
that SnapShotData is allocated in various ways, like on the stack,
using palloc including xip/subxip arrays, with palloc then allocating
xip/subxip arrays separately, or statically allocated and then having
xip/subxip arrays malloc'ed later. This variety was making the
expansion logic a mess.

So I went back to square one and decided to use subxip as an extension
for the xip array instead.

Like the comment added in the function SnapBuildInitialSnapshot
mentions, I don't think we can reliably identify top-level XIDs. So,
this patch just increases the allowed number of XIDs by using the
subxip array.

(The title of the patch was changed accordingly.)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Mar 23, 2023 at 10:53 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Wed, 22 Mar 2023 14:27:40 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Mon, 20 Mar 2023 13:46:51 -0400, "Gregory Stark (as CFM)" <stark.cfm@gmail.com> wrote in

Kyotoro Horiguchi, any chance you'll be able to work on this for this
commitfest? If so shout (or anyone else is planning to push it over
the line.... Andres?) otherwise I'll move it on to the next release.

Ugg. sorry for being lazy. I have lost track of the conversation. I'm
currently working on this and will come back soon with a new version.

I relized that attempting to make SnapshotData.xip expansible was
making procarray.c and snapmgr.c too complicated. The main reason is
that SnapShotData is allocated in various ways, like on the stack,
using palloc including xip/subxip arrays, with palloc then allocating
xip/subxip arrays separately, or statically allocated and then having
xip/subxip arrays malloc'ed later. This variety was making the
expansion logic a mess.

So I went back to square one and decided to use subxip as an extension
for the xip array instead.

Like the comment added in the function SnapBuildInitialSnapshot
mentions, I don't think we can reliably identify top-level XIDs. So,
this patch just increases the allowed number of XIDs by using the
subxip array.

Thanks for working on this, your idea looks fine but my only worry is
that in the future if someone tries to change the logic of
XidInMVCCSnapshot() then they must be aware that the snap->xip array
and snap->subxip array no long distinguishes the top xids and subxids.
I agree with the current logic if we are not marking sub-overflow then
there is no issue, so can we document this in the SnapshotData
structure?

Also, there are some typos in the patch
/idetify/identify
/carete/create
/Aallocate/Allocate

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

Thanks for looking this!

At Thu, 23 Mar 2023 14:15:12 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in

On Thu, Mar 23, 2023 at 10:53 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
Thanks for working on this, your idea looks fine but my only worry is
that in the future if someone tries to change the logic of
XidInMVCCSnapshot() then they must be aware that the snap->xip array
and snap->subxip array no long distinguishes the top xids and subxids.

Yeah, I had the same thought when I was working on the posted version.

I agree with the current logic if we are not marking sub-overflow then
there is no issue, so can we document this in the SnapshotData
structure?

(I found that it was alrady mentioned...)

In a unpublished version (what I referred to as "a mess"), I added a
flag called "topsub_mixed" to SnapshotData, indicating that XIDs of
top and sub transactions are stored in xip and subxip arrays in a
mixed manner. However, I eventually removed it since it could only be
used for sanity checks related to suboverflowed.

I inserted the following sentense in the middle of the comments for
xip and subxip.

In the case of !suboverflowed, there's a situation where this
contains both top and sub-transaction IDs in a mixed manner.

And added similar a similar sentense to a comment of
XidInMVCCSnapshot.

While doning this, I realized that we could simplify and optimize XID
search code by combining the two XID arrays. If !suboverflowed, the
array stored all active XIDs of both top and
sub-transactions. Otherwise it only stores active top XIDs and maybe
XIDs of some sub-transactions. If many subXIDs are stored when
overflowed, there might lead to some degradation but I think the win
we gain from searching just one XID array in most cases outweighs
that. (I didn't do this (of course) in this version.)

Also, there are some typos in the patch
/idetify/identify
/carete/create
/Aallocate/Allocate

Oops! Thanks for pointing out them.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Mar 23, 2023 at 11:02 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

[ new patch ]

Well, I guess nobody is too excited about fixing this, because it's
been another 10 months with no discussion. Andres doesn't even seem to
think this is as much a bug as it is a limitation, for all that it's
filed in the CF application under bug fixes. I kind of wonder if we
should just close this entry in the CF, but I'll hold off on that for
now.

  /*
  * For normal MVCC snapshot this contains the all xact IDs that are in
  * progress, unless the snapshot was taken during recovery in which case
- * it's empty. For historic MVCC snapshots, the meaning is inverted, i.e.
- * it contains *committed* transactions between xmin and xmax.
+ * it's empty. In the case of !suboverflowed, there's a situation where
+ * this contains both top and sub-transaction IDs in a mixed manner.  For
+ * historic MVCC snapshots, the meaning is inverted, i.e.  it contains
+ * *committed* transactions between xmin and xmax.
  *
  * note: all ids in xip[] satisfy xmin <= xip[i] < xmax
  */

I have to say that I don't like this at all. It's bad enough that we
already use the xip/subxip arrays in two different ways depending on
the situation. Increasing that to three different ways seems painful.
How is anyone supposed to keep track of how the array is being used at
which point in the code?

If we are going to do that, I suspect it needs comment updates in more
places than what the patch does currently. For instance:

+ if (newxcnt < xiplen)
+ newxip[newxcnt++] = xid;
+ else
+ newsubxip[newsubxcnt++] = xid;

Just imagine coming across this code in 5 or 10 years and finding that
it had no comment explaining anything. Yikes!

Aside from the details of the patch, and perhaps more seriously, I'm
not really clear that we have consensus on an approach. A few
different proposals seem to have been floated, and it doesn't seem
like anybody hates anybody else's idea completely, but it doesn't
really seem like everyone agrees on what to do, either.

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

On Sat, 6 Jan 2024 at 01:47, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 23, 2023 at 11:02 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

[ new patch ]

Well, I guess nobody is too excited about fixing this, because it's
been another 10 months with no discussion. Andres doesn't even seem to
think this is as much a bug as it is a limitation, for all that it's
filed in the CF application under bug fixes. I kind of wonder if we
should just close this entry in the CF, but I'll hold off on that for
now.

I have changed the status of the patch to "Waiting on Author" as we
don't have a concrete patch with an accepted design which is in a
reviewable shape. We can think if we want to pursue this patch further
or probably close this in the current commitfest and start it again
when someone wants to work on this more actively.

Regards,
Vignesh

Thank you for the comments. This will help move the discussion
forward.

At Fri, 5 Jan 2024 15:17:11 -0500, Robert Haas <robertmhaas@gmail.com> wrote in

On Thu, Mar 23, 2023 at 11:02 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

[ new patch ]

Well, I guess nobody is too excited about fixing this, because it's
been another 10 months with no discussion. Andres doesn't even seem to
think this is as much a bug as it is a limitation, for all that it's
filed in the CF application under bug fixes. I kind of wonder if we
should just close this entry in the CF, but I'll hold off on that for
now.

Perhaps you are correct. Ultimately, this issue is largely
theoretical, and I don't believe anyone would be inconvenienced by
imposing this contraint.

* note: all ids in xip[] satisfy xmin <= xip[i] < xmax
*/

I have to say that I don't like this at all. It's bad enough that we
already use the xip/subxip arrays in two different ways depending on
the situation. Increasing that to three different ways seems painful.
How is anyone supposed to keep track of how the array is being used at
which point in the code?

I understand. So, summirizing the current state briefly, I believe it
as follows:

a. snapbuild lacks a means to differentiate between top and sub xids
during snapshot building.

b. Abusing takenDuringRecovery could lead to potential issues, so it
has been rejected.

c. Dynamic sizing of xip is likely to have a significant impact on
performance (as mentioned in the comments of GetSnapshotData).

d. (new!) Adding a third storing method is not favored.

As a method to satisfy these prerequisites, I think one direction
could be to split takenDuringRecovery into flags indicating the
storage method and creation timing. I present this as a last-ditch
effort. It's a rough proposal, and further validation should be
necessary. If this direction is also not acceptable, I'll proceed with
removing the CF entry.

If we are going to do that, I suspect it needs comment updates in more
places than what the patch does currently. For instance:

+ if (newxcnt < xiplen)
+ newxip[newxcnt++] = xid;
+ else
+ newsubxip[newsubxcnt++] = xid;

Just imagine coming across this code in 5 or 10 years and finding that
it had no comment explaining anything. Yikes!

^^;

Aside from the details of the patch, and perhaps more seriously, I'm
not really clear that we have consensus on an approach. A few
different proposals seem to have been floated, and it doesn't seem
like anybody hates anybody else's idea completely, but it doesn't
really seem like everyone agrees on what to do, either.

I don't fully agree with that.It's not so much that I dislike other
proposals, but rather that we haven't been able to find a definitive
solution that stands out.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Jan 11, 2024 at 9:47 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

I understand. So, summirizing the current state briefly, I believe it
as follows:

a. snapbuild lacks a means to differentiate between top and sub xids
during snapshot building.

b. Abusing takenDuringRecovery could lead to potential issues, so it
has been rejected.

c. Dynamic sizing of xip is likely to have a significant impact on
performance (as mentioned in the comments of GetSnapshotData).

d. (new!) Adding a third storing method is not favored.

As a method to satisfy these prerequisites, I think one direction
could be to split takenDuringRecovery into flags indicating the
storage method and creation timing. I present this as a last-ditch
effort. It's a rough proposal, and further validation should be
necessary. If this direction is also not acceptable, I'll proceed with
removing the CF entry.

I think that the idea of evolving takenDuringRecovery could
potentially work for this problem and maybe for some other things as
well. I remember studying that flag before and coming to the
conclusion that it had some pretty specific and surprising semantics
that weren't obvious from the name -- I don't remember the details --
and so I think improving it could be useful. Also, I think that
multiple storage methods could be more palatable if there were a clear
way to distinguish which one was in use and good comments explaining
the distinction in relevant places.

However, I wonder whether this whole area is in need of a bigger
rethink. There seem to be a number of situations in which the split
into xip and subxip arrays is not very convenient, and also some
situations where it's quite important. Sometimes we want to record
what's committed, and sometimes what isn't. It's all a bit messy and
inconsistent. The necessity of limiting snapshot size is annoying,
too. I have no real idea what can be done about all of this, but what
strikes me is that the current system has grown up incrementally: we
started with a data structure designed for the original use case, and
now by gradually adding new use cases things have gotten complicated.
If we were designing things over from scratch, maybe we'd do it
differently and end up with something less messy. And maybe someone
can imagine a redesign that is likewise less messy.

But on the other hand, maybe not. Perhaps we can't really do any
better than what we have. Then the question becomes whether this case
is important enough to justify additional code complexity. I don't
think I've personally seen users run into this problem so I have no
special reason to think that it's important, but if it's causing
issues for other people then maybe it is.

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

At Fri, 12 Jan 2024 11:28:09 -0500, Robert Haas <robertmhaas@gmail.com> wrote in

However, I wonder whether this whole area is in need of a bigger
rethink. There seem to be a number of situations in which the split
into xip and subxip arrays is not very convenient, and also some
situations where it's quite important. Sometimes we want to record
what's committed, and sometimes what isn't. It's all a bit messy and
inconsistent. The necessity of limiting snapshot size is annoying,
too. I have no real idea what can be done about all of this, but what
strikes me is that the current system has grown up incrementally: we
started with a data structure designed for the original use case, and
now by gradually adding new use cases things have gotten complicated.
If we were designing things over from scratch, maybe we'd do it
differently and end up with something less messy. And maybe someone
can imagine a redesign that is likewise less messy.

But on the other hand, maybe not. Perhaps we can't really do any
better than what we have. Then the question becomes whether this case
is important enough to justify additional code complexity. I don't
think I've personally seen users run into this problem so I have no
special reason to think that it's important, but if it's causing
issues for other people then maybe it is.

Thank you for the deep insights. I have understood your points. As I
can't think of any further simple modifications on this line, I will
withdraw this CF entry. At the moment, I also lack a fundamental,
comprehensive solution, but should if I or anyone else come up with
such a solution in the future, I believe it would worth a separate
discussion.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jan 15, 2024 at 9:18 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

Thank you for the deep insights. I have understood your points. As I
can't think of any further simple modifications on this line, I will
withdraw this CF entry. At the moment, I also lack a fundamental,
comprehensive solution, but should if I or anyone else come up with
such a solution in the future, I believe it would worth a separate
discussion.

I completely agree.

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