From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andrey M. Borodin <x4mmm@yandex-team.ru>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: tender wang <tndrwang@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock
Date: Tue, 6 Feb 2024 11:53:12 +0100
Message-ID: <202402061053.smxzqyip6m5l@alvherre.pgsql> (raw)
In-Reply-To: <CAFiTN-v8r07XALFzUTmMysq23Sa6xshd3cQuqjDv23Hnz-4u-w@mail.gmail.com> <DF01C331-6151-48E0-93E2-553B030835FA@yandex-team.ru>
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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, x4mmm@yandex-team.ru, dilipbalaut@gmail.com, tndrwang@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock
In-Reply-To: <202402061053.smxzqyip6m5l@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox