Date: Tue, 12 Oct 2021 13:59:59 +0900 (JST)
Message-Id: <20211012.135959.877910916353346602.horikyota.ntt@gmail.com>
To: dilipbalaut@gmail.com
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Error "initial slot snapshot too large" in create replication
 slot
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: 
 <CAFiTN-uynCwLm+r_eHH57SftumTPuU+qSr0mxF6hQDUOz0r+BA@mail.gmail.com>
References: 
 <CAFiTN-tqopqpfS6HHug2nnOGieJJ_nm-Nvy0WBZ=Zpo-LqtSJA@mail.gmail.com>
	<20211011.195937.2061830189397708214.horikyota.ntt@gmail.com>
	<CAFiTN-uynCwLm+r_eHH57SftumTPuU+qSr0mxF6hQDUOz0r+BA@mail.gmail.com>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Tue_Oct_12_13_59_59_2021_346)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20211012.135959.877910916353346602.horikyota.ntt%40gmail.com>
Precedence: bulk

----Next_Part(Tue_Oct_12_13_59_59_2021_346)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

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

----Next_Part(Tue_Oct_12_13_59_59_2021_346)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="0001-Allow-overflowed-snapshot-when-creating-logical-repl.patch"