Re: Eagerly evict bulkwrite strategy ring - Mailing list pgsql-hackers

Hi,

On Tue, 9 Sept 2025 at 20:37, Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Mon, Aug 25, 2025 at 3:03 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > I just finished a draft of a patch set to do write combining for COPY
> > FROM using this same heuristic as this patch for deciding to eagerly
> > flush but then combining multiple buffers into a single IO. That has
> > larger performance gains, so one could argue to wait to do that.
>
> Attached v3 uses the same code structure as in the checkpointer write
> combining thread [1]. I need the refactoring of FlushBuffer() now
> included in this set to do the write combining in checkpointer. Upon
> testing, I did notice that write combining seemed to have little
> effect on COPY FROM beyond what eagerly flushing the buffers in the
> ring has. The bottleneck is WAL IO and CPU. While we will need the
> COPY FROM write combining to use direct IO, perhaps it is not worth
> committing it just yet. For now, this thread remains limited to
> eagerly flushing buffers in the BAS_BULKWRITE strategy ring.

Reviewing patches here instead of the checkpointer write combining thread.

From dae7c82146c2d73729fc12a742d84b660e6db2ad Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Tue, 2 Sep 2025 11:00:44 -0400
Subject: [PATCH v3 1/4] Refactor goto into for loop in GetVictimBuffer()

LGTM.

From 2c8aafe30fb58516654e7d0cfdbfbb15a6a00498 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Tue, 2 Sep 2025 11:32:24 -0400
Subject: [PATCH v3 2/4] Split FlushBuffer() into two parts

Codewise LGTM. Two minor points:

1- Commit message only mentions PrepareFlushBuffer() and
DoFlushBuffer(), I did not expect to see the CleanVictimBuffer().
Maybe it is worth mentioning it in the commit message.

2-
+/*
+ * Prepare to write and write a dirty victim buffer.

Although this comment is correct, it is a bit complicated for me. How
about 'Prepare to write and then write a dirty victim buffer'?

From 32f8dbe2c885ce45ef7b217c2693333525fb9b89 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Tue, 2 Sep 2025 12:43:24 -0400
Subject: [PATCH v3 3/4] Eagerly flush bulkwrite strategy ring

+/*
+ * Returns the buffer descriptor of the buffer containing the next block we
+ * should eagerly flush or or NULL when there are no further buffers to

s/or or/ or/.

+ * consider writing out.
+ */
+static BufferDesc *
+next_strat_buf_to_flush(BufferAccessStrategy strategy,
+                        XLogRecPtr *lsn)
+{
+    Buffer        bufnum;
+    BufferDesc *bufdesc;
+
+    while ((bufnum = StrategySweepNextBuffer(strategy)) != InvalidBuffer)
+    {

StrategySweepNextBuffer() returns InvalidBuffer when we reach the
start but can strategy->buffers[strategy->sweep_current] be an
InvalidBuffer? I mean, is the following case possible:
strategy->buffers[strategy->sweep_current] is an InvalidBuffer but
strategy->buffers[strategy->sweep_current + 1] is not. So, we exit
early from the next_strat_buf_to_flush() although there are more
buffers to consider writing out.

+/*
+ * Start a sweep of the strategy ring.
+ */
+void
+StartStrategySweep(BufferAccessStrategy strategy)
+{
+    if (!strategy)
+        return;

I think we will always use this function together with
strategy_supports_eager_flush(), right? If yes, then we do not need to
check if the strategy is NULL. If not, then I think this function
should return boolean to make it explicit that we can not do sweep.

+extern bool strategy_supports_eager_flush(BufferAccessStrategy strategy);

All the functions in the buf_internals.h are pascal case, should we
make this too?

+    /* Must do this before taking the buffer header spinlock. */
+    /* Try to start an I/O operation. */

These one line comments end with a dot.

--
Regards,
Nazir Bilal Yavuz
Microsoft