Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 8 Jul 2024 08:56:42 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
	"Callahan, Drew" <callaan@amazon.com>,
	"pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Subject: Re: Potential data loss due to race condition during logical
 replication slot creation
Message-ID: <ZosrOu7RFnrrsuHL@paquier.xyz>
References: 
 <CAA4eK1+Q+gHADNeEuC=spzCh=JXBMNSWhK1SScu6dQ1owjrBVw@mail.gmail.com>
 <CAD21AoDzjFxFiRYE-Qpxe83weewegbWf7f+zK3r4pSFZwxW_rw@mail.gmail.com>
 <CAA4eK1JWGXcnT2=PYW4U--j8GME4o3+SR6k+bs_r0thG9RVSpQ@mail.gmail.com>
 <CAD21AoCf+PiedCR5ODquL=tnp_2+nj_jemydnyu+KwPQ8beovA@mail.gmail.com>
 <CAA4eK1JbN=jTNtejjk3Jb-4Gr6mZ7PXRn6A_Boda0afB0js1oQ@mail.gmail.com>
 <CAD21AoDYrJMDazvcR-k3SbNAWFiingyQbDYbPLqBp8Q3ewyZJw@mail.gmail.com>
 <CAA4eK1LbpagXYw6eP+qBz2SYjQ3x26ZdVCJBkp033aExqA2MbQ@mail.gmail.com>
 <CAD21AoDR3h78U0hxdzWPuPL11nvJCWYMB8h+QoOjd82ZmXjfgw@mail.gmail.com>
 <CAA4eK1LEy2oUhncuu5AH1RdYgc+NEJn9NcH_8FB+D3m06hrCTg@mail.gmail.com>
 <CAD21AoCS7Xfswo0v_jxZqJ97ndq=59s4Z2GVY8YjQZU-t6198w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Iwr6qlAw+Wfzqpqk"
Content-Disposition: inline
In-Reply-To: 
 <CAD21AoCS7Xfswo0v_jxZqJ97ndq=59s4Z2GVY8YjQZU-t6198w@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZosrOu7RFnrrsuHL%40paquier.xyz>
Precedence: bulk


--Iwr6qlAw+Wfzqpqk
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Jul 05, 2024 at 11:52:52PM +0900, Masahiko Sawada wrote:
> I've attached updated patches for HEAD and pg17 for now (I will create
> the patch for other backbranches).
>=20
> In the patches, I used a different approach in between HEAD and
> backbranches. On HEAD, we store a flag indicating whether or not we
> should skip snapshot restores into the SnapBuild struct and set it
> only while finding the start point. Therefore we have to bump
> SNAPBUILD_VERSION. On backbranches, I used the approach we discussed
> above; store the flag in LogicalDecodingContext and set it when
> creating the LogicalDecodingContext for a new logical slot. A possible
> downside of the approach taken for backbranches is that we implicitly
> require for users to use the same LogicalDecodingContext for  both
> initializing the context for a new slot and finding its start point.
> IIUC it was not strictly required. This restriction would not be a
> problem at least in the core, but I'm not sure if there are no
> external extensions that create a logical slot in that way. This is
> the main reason why I used a different approach on HEAD and
> backbranches. Therefore, if it does not matter, I think we can use the
> same approach on all branches, which is better in terms of
> maintainability.

--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -189,6 +189,9 @@ struct SnapBuild
 	/* Indicates if we are building full snapshot or just catalog one. */
 	bool		building_full_snapshot;
=20
+	/* Indicates if we are finding the start point to extract changes */
+	bool		finding_start_point;
+

FYI, I think that it is still OK to bump SNAPBUILD_VERSION on
REL_17_STABLE.  That will reduce by 1 year the time window required to
maintain the tweaks implemented for the versions in the back-branches.
So I'd suggest to do what the v17 version of the patch does for ~16,
and use the snapshot format changes in 17~.
--
Michael

--Iwr6qlAw+Wfzqpqk
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmaLKzoACgkQnvQgOdby
QH21wQ//fEhkdkmvmWplbrNN8WeN5rfA/PiMSut0dk2mX/XlKvN3QvWRRtuBpSLl
SssPY8NvZmubEzF1B5DCKZb7EB0dykb0SN1NLyUUdPAhI3ZxaSo8ZaDXq/XcbI+7
cpFsOz+PIyxsMeXoHAFVm1r0t5Oxw4cvvir2oKyg7qG9POq5Qw2jcOYFKijbWFYI
jRYOk3aD5vwP2NyM5ikqPoR84ok6tUMstVLqptWZubHy9NsQnmVkiFSuIWuAKoIP
efwmei6YPPeZho0+mxCzkOWizDImxZOlCI0tJMcOFuY7bFaIWICFxgex5s+tI2AB
6vywbEbUrAqfnWRG1gyouHd1H4TCRvw8fPbbP8EiR2bPG3Opf0bsZyKXb70/9Ion
1nbB21JdwJw2YlTH7FfEGVGWleD5O/SCkfEHkRe+MVNf/77eTzKWn2MqrOjGczeT
EslOb1FfPBL84xF7NpGLRyrRRCXNtsMjX21abRCeWxidwQ7LfTvYKYFTg22+Au3T
tnyuxfC7klDWfpCueWjkj3SYDVSAVjavUqbwerQBaBy+DBae84MBgJ58oMhgcGFz
hYPmAbOfMzWpChnuftI7/wQ8+gNytu10WGZ3LQXaSSkWxBVXBxtTxsk6wN4LHJyN
fXejsyvblNrDvQ4CRQNbzh7dGhmc6ZyZqFalscT0SNW9W1VQgYI=
=cErJ
-----END PGP SIGNATURE-----

--Iwr6qlAw+Wfzqpqk--