Date: Wed, 27 Apr 2022 14:16:01 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org, masao.fujii@oss.nttdata.com
Subject: Re: Possible corruption by CreateRestartPoint at promotion
Message-ID: <YmjRkYo2Ce+mO5WO@paquier.xyz>
References: <20220316.102444.2193181487576617583.horikyota.ntt@gmail.com>
 <20220426183349.GA3002960@nathanxps13>
 <20220427.104353.691219597727808197.horikyota.ntt@gmail.com>
 <20220427032609.GA3220948@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="sQubAm0nTkyi2d6C"
Content-Disposition: inline
In-Reply-To: <20220427032609.GA3220948@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/YmjRkYo2Ce%2BmO5WO%40paquier.xyz>
Precedence: bulk


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

On Tue, Apr 26, 2022 at 08:26:09PM -0700, Nathan Bossart wrote:
> On Wed, Apr 27, 2022 at 10:43:53AM +0900, Kyotaro Horiguchi wrote:
>> At Tue, 26 Apr 2022 11:33:49 -0700, Nathan Bossart <nathandbossart@gmail=
=2Ecom> wrote in=20
>>> I suspect we'll start seeing this problem more often once end-of-recove=
ry
>>> checkpoints are removed [0].  Would you mind creating a commitfest entry
>>> for this thread?  I didn't see one.
>>=20
>> I'm not sure the patch makes any change here, because restart points
>> don't run while crash recovery, since no checkpoint records seen
>> during a crash recovery.  Anyway the patch doesn't apply anymore so
>> rebased, but only the one for master for the lack of time for now.
>=20
> Thanks for the new patch!  Yeah, it wouldn't affect crash recovery, but
> IIUC Robert's patch also applies to archive recovery.
>=20
>> +	/* Update pg_control */
>> +	LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
>> +
>>  	/*
>>  	 * Remember the prior checkpoint's redo ptr for
>>  	 * UpdateCheckPointDistanceEstimate()
>>  	 */
>>  	PriorRedoPtr =3D ControlFile->checkPointCopy.redo;
>=20
> nitpick: Why move the LWLockAcquire() all the way up here?

Yeah, that should not be necessary.  InitWalRecovery() is the only
place outside the checkpointer that would touch this field, but that
happens far too early in the startup sequence to matter with the
checkpointer.

>> +	Assert (PriorRedoPtr < RedoRecPtr);
>=20
> I think this could use a short explanation.

That's just to make sure that the current redo LSN is always older
than the one prior that.  It does not seem really necessary to me to
add that.

>> +		/*
>> +		 * Aarchive recovery has ended. Crash recovery ever after should
>> +		 * always recover to the end of WAL
>> +		 */

s/Aarchive/Archive/.

>> +		ControlFile->minRecoveryPoint =3D InvalidXLogRecPtr;
>> +		ControlFile->minRecoveryPointTLI =3D 0;
>> +
>> +		/* also update local copy */
>> +		LocalMinRecoveryPoint =3D InvalidXLogRecPtr;
>> +		LocalMinRecoveryPointTLI =3D 0;
>=20
> Should this be handled by the code that changes the control file state to
> DB_IN_PRODUCTION instead?  It looks like this is ordinarily done in the
> next checkpoint.  It's not clear to me why it is done this way.

Anyway, that would be the work of the end-of-recovery checkpoint
requested at the end of StartupXLOG() once a promotion happens or of
the checkpoint requested by PerformRecoveryXLogAction() in the second
case, no?  So, I don't quite see why we need to update
minRecoveryPoint and minRecoveryPointTLI in the control file here, as
much as this does not have to be part of the end-of-recovery code
that switches the control file to DB_IN_PRODUCTION.

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

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJo0ZEACgkQnvQgOdby
QH2uGQ/8Cr9VQud6pWLsx/A6F8slY3RUKJR/xAoxStCooeGphSPtdROT6gAOYfxF
Jtb6ENx3P3QwVhICZdgttj0eAi60QmxtEl1Wl/hlGXOS+7ZwwsWX7WL3uml4eMBv
31+rDlM3FTS6iizXf2IaFRoR20//PiKi9GLzpORcswjzYTFJNysjW0V0CHDPGErf
tut05YYbLl9V9rDSGPtvwpjez+tqlhu4G+AJNcP694GGGCifSsk2+obHNy5MOdvX
Kb8C00sc6uRYodGMdwjejHAlJZP2zuzbtZc3/smWOio/so46fJMhHKMpArkIQ+4e
+5iW1Qf0a54mlZWihZNHfFYXKhZNqJj7zr0cjBxEfp3OJArbqCS6if1vcDI9Liji
98mh+57RXkbMqLv2Ysx58i6knSVDEuzZUh7jK40Xlqk0F+eztRgTHCi/DYwM/BCh
r0BaCUJ5ePzuuHXz0Ma65vTJkYl7l4bzpSA7xdrNVDpLw62l/pBTsLwyjXWNnuOT
AXEtsTrOHr/HQkvJbWxZCdL7lnyFuu6zlB9MpE6oqPiT73D1E4bccqbc0NyPmrYS
lhdIgBxEDRbzrZsiKb1QEZdh64GTYqsOiw7980GRrsvX139MHpR7oSG1QLp95Ytt
lZdKlgs4Y7QgkxYURvmrxQlp4XdR5rDFMnuDcw6pWUQBC3kMFjs=
=TYoE
-----END PGP SIGNATURE-----

--sQubAm0nTkyi2d6C--