Re: New WAL record to detect the checkpoint redo location - Mailing list pgsql-hackers
From | Andres Freund |
---|---|
Subject | Re: New WAL record to detect the checkpoint redo location |
Date | |
Msg-id | 20230714151626.rhgae7taigk2xrq7@awork3.anarazel.de Whole thread Raw |
In response to | New WAL record to detect the checkpoint redo location (Dilip Kumar <dilipbalaut@gmail.com>) |
Responses |
Re: New WAL record to detect the checkpoint redo location
Re: New WAL record to detect the checkpoint redo location |
List | pgsql-hackers |
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
pgsql-hackers by date: