public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Robert Haas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: New WAL record to detect the checkpoint redo location
Date: Fri, 13 Oct 2023 16:29:12 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoaaMge7NNBE45kSWz9HkgbSoe9mwicOuOOxLAkb6i2JdQ@mail.gmail.com>
References: <CAFiTN-s-K=mVA=HPr_VoU-5bvyLQpNeuzjq1ebPJMEfCJZKFsg@mail.gmail.com>
<[email protected]>
<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>
<[email protected]>
<CA+Tgmob90XtBJ+WdSHA+y8ZikHPSxQvZwYDefEBLC7Bkwx=3-g@mail.gmail.com>
<[email protected]>
<CA+TgmoaaMge7NNBE45kSWz9HkgbSoe9mwicOuOOxLAkb6i2JdQ@mail.gmail.com>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (13+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: New WAL record to detect the checkpoint redo location
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox