Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 13 Oct 2023 16:29:12 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Dilip Kumar <dilipbalaut@gmail.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: New WAL record to detect the checkpoint redo location
Message-ID: <ZSjxyCP9JaE4dJUA@paquier.xyz>
References: <CAFiTN-s-K=mVA=HPr_VoU-5bvyLQpNeuzjq1ebPJMEfCJZKFsg@mail.gmail.com>
 <20230714151626.rhgae7taigk2xrq7@awork3.anarazel.de>
 <CA+TgmoYy-Vc6G9QKcAKNksCa29cv__czr+N9X_QCxEfQVpp_8w@mail.gmail.com>
 <CA+TgmoZ_C1DKuNT0w13_b_6J8JgSupHFSLBCKgQdhJf3FannbA@mail.gmail.com>
 <CA+TgmoZAM24Ub=uxP0aWuWstNYTUJQ64j976FYJeVaMJ+qD0uw@mail.gmail.com>
 <20231005183400.n5myso7vu6crd656@alap3.anarazel.de>
 <CA+Tgmob90XtBJ+WdSHA+y8ZikHPSxQvZwYDefEBLC7Bkwx=3-g@mail.gmail.com>
 <20231009204702.cy3ilp6wgh7a5qms@awork3.anarazel.de>
 <CA+TgmoaaMge7NNBE45kSWz9HkgbSoe9mwicOuOOxLAkb6i2JdQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="vOrhidVr5q2OttQm"
Content-Disposition: inline
In-Reply-To: <CA+TgmoaaMge7NNBE45kSWz9HkgbSoe9mwicOuOOxLAkb6i2JdQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZSjxyCP9JaE4dJUA%40paquier.xyz>
Precedence: bulk


--vOrhidVr5q2OttQm
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Oct 10, 2023 at 02:43:34PM -0400, Robert Haas wrote:
> Here's a new patch set. I think I've incorporated the performance
> fixes that you've suggested so far into this version. I also adjusted
> a couple of other things:

Now looking at 0002, where you should be careful about the code
indentation or koel will complain.

> - After further study of a point previously raised by Amit, I adjusted
> CreateCheckPoint slightly to call WALInsertLockAcquireExclusive
> significantly later than before. I think that there's no real reason
> to do it so early and that the current coding is probably just a
> historical leftover, but it would be good to have some review here.

This makes the new code call LocalSetXLogInsertAllowed() and what we
set for checkPoint.PrevTimeLineID after taking the insertion locks,
which should be OK.

> - I added a cross-check that when starting redo from a checkpoint
> whose redo pointer points to an earlier LSN that the checkpoint
> itself, the record we read from that LSN must an XLOG_CHECKPOINT_REDO
> record.

I've mentioned as well a test in pg_walinspect after one of the
checkpoints generated there, but what you do here is enough for the
online case.

+       /*
+        * XLogInsertRecord will have updated RedoRecPtr, but we need to copy
+        * that into the record that will be inserted when the checkpoint is
+        * complete.
+        */
+       checkPoint.redo = RedoRecPtr;

For online checkpoints, a very important point is that
XLogCtl->Insert.RedoRecPtr is also updated in XLogInsertRecord().
Perhaps that's worth an addition?  I was a bit confused first that we
do the following for shutdown checkpoints:
RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;

Then repeat this pattern for non-shutdown checkpoints a few lines down
without touching the copy of the redo LSN in XLogCtl->Insert, because
of course we don't hold the WAL insert locks in an exclusive fashion
here:
checkPoint.redo = RedoRecPtr;

My point is that this is not only about RedoRecPtr, but also about
XLogCtl->Insert.RedoRecPtr here.  The comment in ReserveXLogSwitch()
says that.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmUo8cgACgkQnvQgOdby
QH1kYA//cKs/BcQO1D6LvLzZMsBxK6fxZGHdNVtvNQN5Mz5cWHizYb96wyGzxM93
eARciHwiUAIS+Xi4GXCJiQ4+mDO0oenpuwfdAo+KWVuN8vTxfu7oH8RygSV1cuWB
HuVeSw2olzeVlFWVuS3WW4IDMQpwCkez44sZ25wJ82Xvz8P3q/NlIdvib7+9Dv8S
cCkDCwaUQT8PoUPtX96Z0SAwj7H+ZOoA6cBxG1AUQ2+vUyqu2GX2pP6Wixoscc92
exlv2KA+F0k/HHKCg1Lpn5MTubmKwB85qD7Vodc9vtCUVIcsmnxhhfNvDnCFg3yb
fVGqQJZHyQ4Xg46CN9H5HvUXluYcwKJwlBcgtDpSDMIENI9bhu7Qg9kijsWM526V
MGVk4rRMKKxxQvoRsO/RRiRhxsohc7iai56PTbfXqTaUxUGnEBgzox7Sics/vgnN
Ovjp5tD94sCjIf7wU+jsAx+HGIUyXx0eRw2IueyJfnMIvNBVWPl2KikONr2iaUly
LfWbUxMc6vyQPlEGU0wI+u0hAvrZjK3OYvN8ILhHi6XDv571q8uxaA0VPTiVJhDM
Y0LiD4EVWEgPHTtTGPxJLfa2utLJEDGoW4PZ66fGSDT9I/oZJuTB06fz4Yyns6X6
I/x9K5jDgJ5LJLj94lLk81CwivO81ABFIIu2OTdwrBnmjT2Ug00=
=THPH
-----END PGP SIGNATURE-----

--vOrhidVr5q2OttQm--