Date: Thu, 28 Apr 2022 15:49:42 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: nathandbossart@gmail.com, pgsql-hackers@lists.postgresql.org,
	masao.fujii@oss.nttdata.com
Subject: Re: Possible corruption by CreateRestartPoint at promotion
Message-ID: <Ymo5BlhEzvXhHObF@paquier.xyz>
References: <YmjRkYo2Ce+mO5WO@paquier.xyz>
 <20220427180945.GA3222843@nathanxps13>
 <Ymnb3XlT4YwJ5yOH@paquier.xyz>
 <20220428.114357.476887320744154615.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="VKPg5Q1jTsQIbsR5"
Content-Disposition: inline
In-Reply-To: <20220428.114357.476887320744154615.horikyota.ntt@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Ymo5BlhEzvXhHObF%40paquier.xyz>
Precedence: bulk


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

On Thu, Apr 28, 2022 at 11:43:57AM +0900, Kyotaro Horiguchi wrote:
> At Thu, 28 Apr 2022 09:12:13 +0900, Michael Paquier <michael@paquier.xyz>=
 wrote in=20
>> On Wed, Apr 27, 2022 at 11:09:45AM -0700, Nathan Bossart wrote:
>>> On Wed, Apr 27, 2022 at 02:16:01PM +0900, Michael Paquier wrote:
>>>> -   if (ControlFile->state =3D=3D DB_IN_ARCHIVE_RECOVERY &&
>>>> -       ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
>>>> -   {
>>>> 7ff23c6 has removed the last call to CreateCheckpoint() outside the
>>>> checkpointer, meaning that there is one less concurrent race to worry
>>>> about, but I have to admit that this change, to update the control
>>>> file's checkPoint and checkPointCopy even if we don't check after
>>>> ControlFile->checkPointCopy.redo < lastCheckPoint.redo would make the
>>>> code less robust in ~14.  So I am questioning whether a backpatch
>>>> is actually worth the risk here.
>>>=20
>>> IMO we should still check this before updating ControlFile to be safe.
>>=20
>> Sure.  Fine by me to play it safe.
>=20
> Why do we consider concurrent check/restart points here while we don't
> consider the same for ControlFile->checkPointCopy?

I am not sure what you mean here.  FWIW, I am translating the
suggestion of Nathan to split the existing check in
CreateRestartPoint() that we are discussing here into two if blocks,
instead of just one:
- Move the update of checkPoint and checkPointCopy into its own if
block, controlled only by the check on
(ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
- Keep the code updating minRecoveryPoint and minRecoveryPointTLI
mostly as-is, but just do the update when the control file state is
DB_IN_ARCHIVE_RECOVERY.  Of course we need to keep the check on
(minRecoveryPoint < lastCheckPointEndPtr).

v5 is mostly doing that.
--
Michael

--VKPg5Q1jTsQIbsR5
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJqOQYACgkQnvQgOdby
QH37Ug//R16pNCF36STNFldlJTU0N5Xyqk3BDCUMfLDQF+ahCwhmS9TmQ9SaL1WF
MTdkOjTEog25mRzDjQkpO8ggVoeyGtkws3kj04LLiPa+tWPuN1rNjoZzvAiCqVKX
PiPwd2th7oTAm+/3VKOYWMeWwDVecMFaCVVQXxZQThOsSR39ZGBxbFwNk9G9Ox/0
4kZ4oeaGkffax4Ikno/yEEExh5QL4L9jsZf3DmB98FfVOc/0VIHYxPp+QRrnV/gH
JSfU3uZSrpmeGZhq02TC16VG7bm4fOb4teRxWXjBDIfYEdDkZc9Yhm9sTmAYD1q/
sMCPBTfxpkfY72B3pPTyepaLsKewdJ0k2iSQc9ZWfQAMt/StLCByXoagALNce43t
XD5Ka2EZuQv2h16IK9W8b1HCnUiJn6qpQZRXQJzwRoL7M/4SS0LsVQdMQp/qWkyk
fGL5Qoa03jzzZRzHXWi6FYX1yvQQwHamnabPtyyZRgpWvvhebBhS8PT0E8SU6zFS
TF9nnDTszNasjUCWEYCkv+NBDdp2Y+sK5hQSRf14dY6q7hygKOov149Bs5iSR6ec
5eFnU/AhbkwlQX7PTTPpL6XRPsXiVwE9HVW3MbfCsrUkZ7rfrb7AMeVx07aNorhi
NOB40p7WjZpA3rH8BMZpTA/2JzOpaF2QLTFi0Jc00aRB6BwAJnw=
=NAR9
-----END PGP SIGNATURE-----

--VKPg5Q1jTsQIbsR5--