Date: Fri, 01 Apr 2022 14:44:30 +0900 (JST)
Message-Id: <20220401.144430.1701887207072649814.horikyota.ntt@gmail.com>
To: y.sokolov@postgrespro.ru
Cc: dilipbalaut@gmail.com, rjuju123@gmail.com,
 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: <755c7bb23ad97a4a5319ebd13cdf8afccf5f05ed.camel@postgrespro.ru>
References: <20220131.152011.148738317202375552.horikyota.ntt@gmail.com>
	<CAFiTN-s_PYicXeb6+uZkERqATpZLzpfsg0qP6G_GMPhu=rgxUA@mail.gmail.com>
	<755c7bb23ad97a4a5319ebd13cdf8afccf5f05ed.camel@postgrespro.ru>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=koi8-r
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/20220401.144430.1701887207072649814.horikyota.ntt%40gmail.com>
Precedence: bulk

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

> =F7 =F0=CE, 07/02/2022 =D7 13:52 +0530, Dilip Kumar =D0=C9=DB=C5=D4:
> > 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 ren=
amed
> 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 chan=
ge?
> 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