Re: New WAL record to detect the checkpoint redo location

On Thu, Aug 31, 2023 at 9:36 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Aug 30, 2023 at 04:51:19PM +0530, Dilip Kumar wrote:
> > Your suggestions LGTM so modified accordingly
>
> I have been putting my HEAD on this patch for a few hours, reviewing
> the surroundings, and somewhat missed that this computation is done
> while we do not hold the WAL insert locks:
> +       checkPoint.redo = ProcLastRecPtr;
>
> Then a few lines down the shared Insert.RedoRecPtr is updated while
> holding an exclusive lock.
>     RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;
>
> If we have a bunch of records inserted between the moment when the
> REDO record is inserted and the moment when the checkpointer takes the
> exclusive WAL lock, aren't we potentially missing a lot of FPW's that
> should exist since the redo LSN?

Yeah, good catch.  With this, it seems like we can not move this new
WAL Insert out of the Exclusive WAL insertion lock right? because if
we want to set the LSN of this record as the checkpoint. redo then
there should not be any concurrent insertion until we expose the
XLogCtl->Insert.RedoRecPtr. Otherwise, we will miss the FPW for all
the record which has been inserted after the checkpoint. redo before
we acquired the exclusive WAL insertion lock.

So maybe I need to restart from the first version of the patch but
instead of moving the insertion of the new record out of the exclusive
lock need to do some better refactoring so that XLogInsertRecord()
doesn't look ugly.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com