Re: New WAL record to detect the checkpoint redo location

Hi,

As I think I mentioned before, I like this idea. However, I don't like the
implementation too much.

On 2023-06-15 13:11:57 +0530, Dilip Kumar wrote:
> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index b2430f617c..a025fb91e2 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -744,6 +744,7 @@ XLogInsertRecord(XLogRecData *rdata,
>      XLogRecPtr    StartPos;
>      XLogRecPtr    EndPos;
>      bool        prevDoPageWrites = doPageWrites;
> +    bool        callerHoldingExlock = holdingAllLocks;
>      TimeLineID    insertTLI;
>  
>      /* we assume that all of the record header is in the first chunk */
> @@ -792,10 +793,18 @@ XLogInsertRecord(XLogRecData *rdata,
>       *----------
>       */
>      START_CRIT_SECTION();
> -    if (isLogSwitch)
> -        WALInsertLockAcquireExclusive();
> -    else
> -        WALInsertLockAcquire();
> +
> +    /*
> +     * Acquire wal insertion lock, nothing to do if the caller is already
> +     * holding the exclusive lock.
> +     */
> +    if (!callerHoldingExlock)
> +    {
> +        if (isLogSwitch)
> +            WALInsertLockAcquireExclusive();
> +        else
> +            WALInsertLockAcquire();
> +    }
>  
>      /*
>       * Check to see if my copy of RedoRecPtr is out of date. If so, may have

This might work right now, but doesn't really seem maintainable. Nor do I like
adding branches into this code a whole lot.


> @@ -6597,6 +6612,32 @@ CreateCheckPoint(int flags)

I think the commentary above this function would need a fair bit of
revising...

>       */
>      RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;
>  
> +    /*
> +     * Insert a special purpose CHECKPOINT_REDO record as the first record at
> +     * checkpoint redo lsn.  Although we have the checkpoint record that
> +     * contains the exact redo lsn, there might have been some other records
> +     * those got inserted between the redo lsn and the actual checkpoint
> +     * record.  So when processing the wal, we cannot rely on the checkpoint
> +     * record if we want to stop at the checkpoint-redo LSN.
> +     *
> +     * This special record, however, is not required when we doing a shutdown
> +     * checkpoint, as there will be no concurrent wal insertions during that
> +     * time.  So, the shutdown checkpoint LSN will be the same as
> +     * checkpoint-redo LSN.
> +     *
> +     * This record is guaranteed to be the first record at checkpoint redo lsn
> +     * because we are inserting this while holding the exclusive wal insertion
> +     * lock.
> +     */
> +    if (!shutdown)
> +    {
> +        int            dummy = 0;
> +
> +        XLogBeginInsert();
> +        XLogRegisterData((char *) &dummy, sizeof(dummy));
> +        recptr = XLogInsert(RM_XLOG_ID, XLOG_CHECKPOINT_REDO);
> +    }

It seems to me that we should be able to do better than this.

I suspect we might be able to get rid of the need for exclusive inserts
here. If we rid of that, we could determine the redoa location based on the
LSN determined by the XLogInsert().

Alternatively, I think we should split XLogInsertRecord() so that the part
with the insertion locks held is a separate function, that we could use here.

Greetings,

Andres Freund