Re: Checkpointer write combining - Mailing list pgsql-hackers

On Tue, Dec 16, 2025 at 11:46 PM Soumya S Murali
<soumyamurali.work@gmail.com> wrote:
>
> Thank you for the clarification.
> I understand the concern, and apologies for the earlier confusion. My
> intention is to stay aligned with the direction of your v11 series, so
> this patch contains only small, incremental changes intended to be
> easy to review and not conflict with ongoing work.
> My previous message was only meant to summarize logic I was
> experimenting with locally, not to propose it as a final patch. Based
> on the feedback, I revisited the implementation, made the necessary
> modifications, and verified the updated logic.
> I am attaching the patch for review. It has been tested with make
> check and make -C src/test/isolation check.
> Thank you again for the guidance. I hope this is helpful for the
> ongoing work, and I am looking forward to further feedback.

I took a look at your patch and the first two parts were already fixed
in a previous version

--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4405,7 +4405,7 @@ BufferNeedsWALFlush(BufferDesc *bufdesc, XLogRecPtr *lsn)
     UnlockBufHdr(bufdesc);

     /* Skip all WAL flush logic if relation is not logged */
-    if (!(*lsn != InvalidXLogRecPtr))
+    if (!XLogRecPtrIsValid(*lsn))
         return false;


@@ -4433,6 +4433,12 @@ CleanVictimBuffer(BufferDesc *bufdesc, bool
from_ring, IOContext io_context)
     content_lock = BufHdrGetContentLock(bufdesc);
     LWLockAcquire(content_lock, LW_SHARED);

+    if (!PrepareFlushBuffer(bufdesc, &max_lsn))
+    {
+        LWLockRelease(content_lock);
+        return;
+    }
+

And I don't understand the last parts of the diff.

@@ -4440,19 +4446,14 @@ CleanVictimBuffer(BufferDesc *bufdesc, bool
from_ring, IOContext io_context)
     LWLockRelease(content_lock);
     LWLockAcquire(content_lock, LW_EXCLUSIVE);

-    /*
-     * Mark the buffer ready for checksum and write.
-     */
-    PrepareBufferForCheckpoint(bufdesc, &max_lsn);
+    if (!PrepareFlushBuffer(bufdesc, &max_lsn))
+    {
+        LWLockRelease(content_lock);
+        return;
+    }

     /* Release exclusive lock; the batch will write the page later */
     LWLockRelease(content_lock);
-
-    /*
-     * Add LSN to caller's batch tracking.
-     * Caller handles XLogFlush() using highest LSN.
-     */
-    PrepareBufferForCheckpoint(bufdesc, max_lsn);
 }

AFAIK, there has never been a function called
PrepareBufferForCheckpoint() and my current patchset doesn't have it
either.

- Melanie