Hi all,
Based on the ongoing discussions on Checkpointer Write Combining [1] and the last patch v11 discussed in forum, I have re-implemented the logic for CleanVictimBuffer() and BufferNeedsWALFlush() with a few changes:
static void
CleanVictimBuffer(BufferDesc *bufdesc, bool from_ring, IOContext io_context)
{
XLogRecPtr max_lsn = InvalidXLogRecPtr;
LWLock *content_lock;
content_lock = BufHdrGetContentLock(bufdesc);
LWLockAcquire(content_lock, LW_SHARED);
if (!PrepareFlushBuffer(bufdesc, &max_lsn))
{
LWLockRelease(content_lock);
return;
}
LWLockRelease(content_lock);
LWLockAcquire(content_lock, LW_EXCLUSIVE);
if (!PrepareFlushBuffer(bufdesc, &max_lsn))
{
LWLockRelease(content_lock);
return;
}
BufferSetStateWriteInProgress(bufdesc);
LWLockRelease(content_lock);
RecordBufferForBatch(bufdesc, max_lsn);
}
Here I added some extra safety checks and early-exit handling, making sure the function returns cleanly when a buffer no longer needs writing and ensuring locks are handled correctly.
static bool
BufferNeedsWALFlush(BufferDesc *bufdesc, XLogRecPtr *lsn)
{
uint32 buf_state;
buf_state = LockBufHdr(bufdesc);
if (buf_state & BM_PERMANENT)
*lsn = BufferGetLSN(bufdesc);
else
*lsn = InvalidXLogRecPtr;
UnlockBufHdr(bufdesc);
if (!(*lsn != InvalidXLogRecPtr))
return false;
return XLogNeedsFlush(*lsn);
}
Here I made some changes in the logic of the function that only returns a real LSN when the buffer actually needs a WAL flush. If the buffer is not permanent or doesn’t need WAL, it returns false and set the LSN to InvalidXLogRecPtr, by which it can avoid confusions or unsafe downstream behaviors.
