Re: Eagerly evict bulkwrite strategy ring

On Mon, Aug 18, 2025 at 2:54 AM Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
> Hi!

Thanks for the review!

> 1) In EvictStrategyRing we find io context for strategy:
>
> > + io_context = IOContextForStrategy(strategy);
>
> but the caller of this function (GetVictimBuffer) already has one.
> Should we reuse its context, pass it as function param to
> EvictStrategyRing?

Done in attached v2.

> 2) QuickCleanBuffer function has a return value which is never
> checked. Should we change the signature to `void QuickCleanBuffer
> (...)` ?

Done in attached v2.

> 3) In QuickCleanBuffer, we have `buffer =
> BufferDescriptorGetBuffer(bufdesc);`, while in QuickCleanBuffer we do
> the opposite right before QuickCleanBuffer call. Should we pass
> `bufnum` as a parameter?

Done in attached v2.

v2 also changes instances of the word "evict" to simply "flush"
because we don't actually invalidate the contents of the buffer -- we
just flush it so that it can be cheaply evicted when it is needed.

Also, I noticed that v1 had an issue -- it goes through the buffers
from 0 to nbuffers and flushes them instead of starting at the buffer
just after the one we flushed and doing a single sweep. I've fixed
that in the attached. It makes it more likely we'll flush a buffer
we're about to use.

> > A 15% improvement can be noticed with the same benchmark but 4 workers.
> >
> > - Melanie
> In only get 5-10% improvements
>
> I did this benchmark also. For 16 source data files that are 150MB
> each I get 5-10 % speedup (5% with small shared_buffers and 10 % with
> big shared_buffers).

Yes, one possible reason is that with smaller source files (150 MB vs
1 GB) is that the times around the ring for each COPY are less vs. the
setup and one time costs. And there are fewer sustained periods of
time where the different COPYies are ongoing doing the same
operations.

However, the more likely reason you see a smaller improvement is just
variance from one SSD and CPU to the next. I tried on three models of
SSD and two of CPU (all local [not cloud]) and saw variation in the
improvement -- on some models the improvement was less. For all cases,
I had to turn off CPU turbo boosting and idling to see consistent
results.

It's possible that this performance improvement of this patch by
itself isn't large enough to merit committing it.

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.

- Melanie