Hi Dilip! Thank you for the review!
On Tue, Jan 10, 2023 at 9:58 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Jan 9, 2023 at 9:49 AM Andrey Borodin <amborodin86@gmail.com> wrote:
> >
> > On Tue, Jan 3, 2023 at 5:02 AM vignesh C <vignesh21@gmail.com> wrote:
> > > does not apply on top of HEAD as in [1], please post a rebased patch:
> > >
> > Thanks! Here's the rebase.
>
> I was looking into this patch, it seems like three different
> optimizations are squeezed in a single patch
> 1) dividing buffer space in banks to reduce the seq search cost 2) guc
> parameter for buffer size scale 3) some of the buffer size values are
> modified compared to what it is on the head. I think these are 3
> patches which should be independently committable.
There's no point in dividing SLRU buffers in parts unless the buffer's
size is configurable.
And it's only possible to enlarge default buffers size if SLRU buffers
are divided into banks.
So the features can be viewed as independent commits, but make no
sense separately.
But, probably, it's a good idea to split the patch back anyway, for
easier review.
>
> While looking into the first idea of dividing the buffer space in
> banks, I see that it will speed up finding the buffers but OTOH while
> searching the victim buffer it will actually can hurt the performance
> the slru pages which are frequently accessed are not evenly
> distributed across the banks. So imagine the cases where we have some
> banks with a lot of empty slots and other banks from which we
> frequently have to evict out the pages in order to get the new pages
> in.
>
Yes. Despite the extremely low probability of such a case, this
pattern when a user accesses pages assigned to only one bank may
happen.
This case is equivalent to having just one bank, which means small
buffers. Just as we have now.
Thank you!
Best regards, Andrey Borodin.