Horton

Thread

  1. Re: Checkpointer write combining

    Soumya S Murali <soumyamurali.work@gmail.com> — 2025-11-25T11:28:49Z

    Hi all,
I applied the v11 patchset on my 19devel source code without any
modifications and ran some rounds of pgbench for testing. I ran
multiple workloads with different client counts and it executed
successfully without any backend crashes. I am including the
observations herewith for further review.

1. pgbench -c 4 -j 4 -T 10 -p 55432 postgres
pgbench (19devel)
starting vacuum...end.
transaction type: <builtin: TPC-B (sort of)>
scaling factor: 50
query mode: simple
number of clients: 4
number of threads: 4
maximum number of tries: 1
duration: 10 s
number of transactions actually processed: 10103
number of failed transactions: 0 (0.000%)
latency average = 3.958 ms
initial connection time = 4.970 ms
tps = 1010.552537 (without initial connection time)

2. pgbench -c 8 -j 8 -T 120 -p 55432 postgres
pgbench (19devel)
starting vacuum...end.
transaction type: <builtin: TPC-B (sort of)>
scaling factor: 50
query mode: simple
number of clients: 8
number of threads: 8
maximum number of tries: 1
duration: 120 s
number of transactions actually processed: 193468
number of failed transactions: 0 (0.000%)
latency average = 4.962 ms
initial connection time = 8.135 ms
tps = 1612.271773 (without initial connection time)

3. pgbench -c 16 -j 8 -T 180 -p 55432 postgres
pgbench (19devel)
starting vacuum...end.
transaction type: <builtin: TPC-B (sort of)>
scaling factor: 50
query mode: simple
number of clients: 16
number of threads: 8
maximum number of tries: 1
duration: 180 s
number of transactions actually processed: 598737
number of failed transactions: 0 (0.000%)
latency average = 4.810 ms
initial connection time = 13.793 ms
tps = 3326.398071 (without initial connection time)

I will continue testing with more workloads including forced
checkpoints to see if it can trigger any edge cases or WAL flush
decisions. But for now, the v11 series appears stable on my system.
Looking forward to more feedback.

Regards,
Soumya

  2. Re: Checkpointer write combining

    Soumya S Murali <soumyamurali.work@gmail.com> — 2025-12-08T07:14:33Z

    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.

I am now focusing on the PageSetBatchChecksumInplace() as mentioned in [1]
and the write combining logic. I will update my implementations soon once I
complete it.
I hope these corrected versions of functions will be helpful in further
implementation. Looking forward to more feedback.

Regards,
Soumya

Reference:
Discussion [1]
https://www.postgresql.org/message-id/CAAKRu_ZiEpE_EHww3S3-E3iznybdnX8mXSO7Wsuru7=P9Y=czQ@mail.gmail.com

  3. Re: Checkpointer write combining

    Soumya S Murali <soumyamurali.work@gmail.com> — 2025-12-15T09:37:00Z

    Hi all,

With reference to the last patches (v11) I received [1] and while reviewing
Melanie’s latest feedback, I understood that PageSetBatchChecksumInplace()
is currently WIP and depends on upcoming changes to hint-bit locking. It
will be contrary to the flow if I propose new functional changes to
checksum batching at this time. So for now I will focus on preparatory or
documentation improvements until I get the updates on dependencies.
Regarding my patch attached, the patch introduces write-combining during
checkpoints by batching contiguous buffers and allowing them to be written
using vectorized I/O. My patch includes write-combining for checkpoint
buffer flushes, contiguous buffer batching, Preserved WAL ordering,
locking, and buffer state invariants. The change is currently limited to
the checkpointer path (BufferSync()). So far I tested my implementation and
found that all the regression (233 tests) and isolation tests (121 tests)
got passed, the manual pgbench validation completed successfully and also
verified pg_stat_bgwriter counters before and after checkpoints. So far the
implementation is stable in my system. And currently, I am focussing on the
implementation of check pointer write combining for bgwriter behavior and
will update it soon after validating my implementation.
I hope my findings and approach will be helpful in further implementations
and I would appreciate feedback on my approach.

Regards,
Soumya

Reference:
[1]
https://www.postgresql.org/message-id/flat/CAAKRu_ZiEpE_EHww3S3-E3iznybdnX8mXSO7Wsuru7%3DP9Y%3DczQ%40mail.gmail.com#52e4f67645f26609427d5b1fc31fdf08

  4. Re: Checkpointer write combining

    Melanie Plageman <melanieplageman@gmail.com> — 2025-12-15T21:48:40Z

    On Mon, Dec 15, 2025 at 4:36 AM Soumya S Murali
<soumyamurali.work@gmail.com> wrote:
>
> With reference to the last patches (v11) I received [1] and while reviewing Melanie’s latest feedback, I understood that PageSetBatchChecksumInplace() is currently WIP and depends on upcoming changes to hint-bit locking. It will be contrary to the flow if I propose new functional changes to checksum batching at this time. So for now I will focus on preparatory or documentation improvements until I get the updates on dependencies.
> Regarding my patch attached, the patch introduces write-combining during checkpoints by batching contiguous buffers and allowing them to be written using vectorized I/O. My patch includes write-combining for checkpoint buffer flushes, contiguous buffer batching, Preserved WAL ordering, locking, and buffer state invariants. The change is currently limited to the checkpointer path (BufferSync()). So far I tested my implementation and found that all the regression (233 tests) and isolation tests (121 tests) got passed, the manual pgbench validation completed successfully and also verified pg_stat_bgwriter counters before and after checkpoints. So far the implementation is stable in my system.

Can you explain how your implementation differs from what was posted
in v11 0006 [1]? That implements checkpointer write combining. I'm
open to ideas for improving the code, but I don't understand how your
patch is supposed to fit into the ongoing work on this thread.

- Melanie

[1] https://www.postgresql.org/message-id/CAAKRu_ZiEpE_EHww3S3-E3iznybdnX8mXSO7Wsuru7%3DP9Y%3DczQ%40mail.gmail.com

  5. Re: Checkpointer write combining

    Melanie Plageman <melanieplageman@gmail.com> — 2025-12-15T21:50:31Z

    On Mon, Dec 8, 2025 at 2:14 AM Soumya S Murali
<soumyamurali.work@gmail.com> wrote:
>
> 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.

Thanks for the suggestions. It would be best if you attached a patch
instead of pasting it in the email like this with no formatting (and
no diff). It is very hard to tell what you've changed.

- Melanie

  6. Re: Checkpointer write combining

    Soumya S Murali <soumyamurali.work@gmail.com> — 2025-12-17T04:46:48Z

    Hi all,

On Tue, Dec 16, 2025 at 3:20 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Mon, Dec 8, 2025 at 2:14 AM Soumya S Murali
> <soumyamurali.work@gmail.com> wrote:
> >
> > 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.
>
> Thanks for the suggestions. It would be best if you attached a patch
> instead of pasting it in the email like this with no formatting (and
> no diff). It is very hard to tell what you've changed.
>
> - Melanie

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.


Regards
Soumya

  7. Re: Checkpointer write combining

    Soumya S Murali <soumyamurali.work@gmail.com> — 2025-12-17T05:53:08Z

    Hi all,

On Tue, Dec 16, 2025 at 3:18 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Mon, Dec 15, 2025 at 4:36 AM Soumya S Murali
> <soumyamurali.work@gmail.com> wrote:
> >
> > With reference to the last patches (v11) I received [1] and while reviewing Melanie’s latest feedback, I understood that PageSetBatchChecksumInplace() is currently WIP and depends on upcoming changes to hint-bit locking. It will be contrary to the flow if I propose new functional changes to checksum batching at this time. So for now I will focus on preparatory or documentation improvements until I get the updates on dependencies.
> > Regarding my patch attached, the patch introduces write-combining during checkpoints by batching contiguous buffers and allowing them to be written using vectorized I/O. My patch includes write-combining for checkpoint buffer flushes, contiguous buffer batching, Preserved WAL ordering, locking, and buffer state invariants. The change is currently limited to the checkpointer path (BufferSync()). So far I tested my implementation and found that all the regression (233 tests) and isolation tests (121 tests) got passed, the manual pgbench validation completed successfully and also verified pg_stat_bgwriter counters before and after checkpoints. So far the implementation is stable in my system.
>
> Can you explain how your implementation differs from what was posted
> in v11 0006 [1]? That implements checkpointer write combining. I'm
> open to ideas for improving the code, but I don't understand how your
> patch is supposed to fit into the ongoing work on this thread.
>
> - Melanie
>
> [1] https://www.postgresql.org/message-id/CAAKRu_ZiEpE_EHww3S3-E3iznybdnX8mXSO7Wsuru7%3DP9Y%3DczQ%40mail.gmail.com


Thank you for the question.
My patch is not intended to replace or redesign v11-0006. I am fully
aligned with that patch and treated it as the baseline for my work.
The work I sent is intentionally incremental, rather than introducing
a new batching logic.
v11-0006 already implements the core checkpointer write-combining
logic (batch formation, contiguity checks, WAL ordering, pin limits,
and IO issuance). I did not change that structure.
My changes focus on correctness around existing CleanVictimBuffer()
ensuring content locks are always released on early exit paths and
making the shared exclusive lock transitions explicit. This directly
addresses the lock-handling issue you pointed out earlier in the
thread. And the BufferNeedsWALFlush() clarifying semantics so that an
LSN is only returned when the buffer is logged (BM_PERMANENT),
otherwise explicitly setting it to InvalidXLogRecPtr. This matches the
direction you mentioned about avoiding confusing or unsafe LSN
propagation.
I intentionally did not modify PageSetBatchChecksumInplace(), since it
is clearly marked WIP and depended on the hint-bit locking work as you
mentioned.
I just validated the v11-0006 design on a fresh tree, done a few small
correctness cleanups that do not alter behavior and done the testing
like make check, isolation tests and manual checkpoint validation for
confirmation. I hope you find this useful.
Thank you for the guidance and patience. Looking forward to more feedback.


Regards,
Soumya

  8. Re: Checkpointer write combining

    Melanie Plageman <melanieplageman@gmail.com> — 2025-12-17T15:39:05Z

    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

  9. Re: Checkpointer write combining

    Soumya S Murali <soumyamurali.work@gmail.com> — 2025-12-29T10:07:40Z

    Hi all,

On Wed, Dec 17, 2025 at 9:09 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> 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


Thank you for reviewing the patch and for the clarification.
It seems I was working on the v11 patch version that I had received
from November, and I did not have the newer fixes that already
addressed the XLogRecPtrIsValid() check and the shared-lock early
return case in CleanVictimBuffer(). That's why the first part of my
changes duplicated the work that has already been fixed.
Regarding the latter part of the diff, the reason I attempted that
change was that in my local tree the code was still performing both
WAL–LSN read + dirty check inside the exclusive lock followed by
marking/queuing the buffer for write. And I initially interpreted that
section as functionally similar to the description of
PrepareBufferForCheckpoint() from the early discussion in the thread.
Because I didn't have the newer version of the patchset where this
function was removed or renamed, I refactored it into a second
PrepareFlushBuffer() call to ensure correctness on re-checking after
lock upgrade and to keep failure paths symmetrical (especially unlock
and early return). I now understand that this was based on an outdated
context and why it conflicts with the current patch flow. So I will
rebase my work on the latest version of the patch and will drop that
section to avoid misalignment.
If there are any specific areas where I can help align with the
ongoing work, please let me know I’m happy to continue contributing in
the direction you are taking the patchset.


Regards,
Soumya