On Wed, Sep 10, 2025 at 6:14 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
Thanks so much for the review! I've only included inline comments to
things that still might need discussion. Otherwise, I've incorporated
your suggested changes.
> 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
> 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'?
I've gone with * Prepare and write out 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
>
> + * 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.
Yes, good thought. Actually for BAS_BULKWRITE this cannot happen
because when a buffer is not reused we overwrite its place in the
buffers array with the shared buffer we then replace it with. It can
happen for BAS_BULKREAD. Since we are only concerned with writing, I
think we can terminate after we hit an InvalidBuffer in the ring.
While looking at this, I decided it didn't make sense to have sweep
variables in the strategy object, so I've actually changed the way
StrategySweepNextBuffer() works. There was also an issue with the
sweep -- it could run into and past the starting buffer. So, I had to
change it. Take a look at the new method and let me know what you
think.
> +/*
> + * 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.
Yes, I just removed this check.
> +extern bool strategy_supports_eager_flush(BufferAccessStrategy strategy);
>
> All the functions in the buf_internals.h are pascal case, should we
> make this too?
I thought maybe I'd go a different way because it's sort of
informational and not a function that does stuff -- but anyway you're
right. I've given up and made all my helpers pascal case.
- Melanie
