Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock - Mailing list pgsql-hackers

From Alvaro Herrera
Subject Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock
Date
Msg-id 202402061053.smxzqyip6m5l@alvherre.pgsql
Whole thread Raw
In response to Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock  ("Andrey M. Borodin" <x4mmm@yandex-team.ru>)
Responses Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock
List pgsql-hackers
On 2024-Feb-04, Andrey M. Borodin wrote:

> This patch uses wording "banks" in comments before banks start to
> exist. But as far as I understand, it is expected to be committed
> before "banks" patch.

True -- changed that to use ControlLock.

> Besides this patch looks good to me.

Many thanks for reviewing.

On 2024-Feb-05, Dilip Kumar wrote:

> > (We also have SimpleLruTruncate, but I think it's not as critical to
> > have a barrier there anyhow: accessing a slightly outdated page number
> > could only be a problem if a bug elsewhere causes us to try to truncate
> > in the current page.  I think we only have this code there because we
> > did have such bugs in the past, but IIUC this shouldn't happen anymore.)
> 
> +1, I agree with this theory in general.  But the below comment in
> SimpleLruTrucate in your v3 patch doesn't seem correct, because here
> we are checking if the latest_page_number is smaller than the cutoff
> if so we log it as wraparound and skip the whole thing and that is
> fine even if we are reading with atomic variable and slightly outdated
> value should not be a problem but the comment claim that this safe
> because we have the same bank lock as SimpleLruZeroPage(), but that's
> not true here we will be acquiring different bank locks one by one
> based on which slotno we are checking.  Am I missing something?

I think you're correct.  I reworded this comment, so now it says this:

    /*
     * An important safety check: the current endpoint page must not be
     * eligible for removal.  This check is just a backstop against wraparound
     * bugs elsewhere in SLRU handling, so we don't care if we read a slightly
     * outdated value; therefore we don't add a memory barrier.
     */

Pushed with those changes.  Thank you!

Now I'll go rebase the rest of the patch on top.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Having your biases confirmed independently is how scientific progress is
made, and hence made our great society what it is today" (Mary Gardiner)



pgsql-hackers by date:

Previous
From: Ashutosh Bapat
Date:
Subject: Re: RFC: Logging plan of the running query
Next
From: Peter Eisentraut
Date:
Subject: Allow passing extra options to initdb for tests