Feedback-ID: id4a34324:Fastmail
Date: Wed, 8 Feb 2023 12:26:21 -0800
From: Andres Freund <andres@anarazel.de>
To: "Bagga, Rishu" <bagrishu@amazon.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Thomas Munro <thomas.munro@gmail.com>,
	"Debnath, Shawn" <sdn@amazon.com>, vignesh C <vignesh21@gmail.com>
Subject: Re: SLRUs in the main buffer pool - Page Header definitions
Message-ID: <20230208202621.53mmhammbfu5mes6@awork3.anarazel.de>
References: <EFAAC0BE-27E9-4186-B925-79B7C696D5AC@amazon.com>
 <7C4D53F1-A156-4ABC-9B89-B2C253D5FF74@amazon.com>
 <CAB8KJ=iwph4My_kGek_gANymZSU3UegdDYkF+gWEH+5JxE4txg@mail.gmail.com>
 <A45DC000-C7F5-43C1-9E9E-57FB18E2FD0D@amazon.com>
 <D663D8DD-6B13-4DF4-9A8D-045D5F0F2A7E@amazon.com>
 <CALDaNm299u5YcMUXztCXaQeRks+mqJ-KrJ5MkAGWjhjVnJv4Pg@mail.gmail.com>
 <A09EAE0D-0D3F-4A34-ADE9-8AC1DCBE7D57@amazon.com>
 <7BAF384A-654C-4877-BB4D-EDE4E44A9326@amazon.com>
 <20230207202019.mhovw7aopjy3bile@awork3.anarazel.de>
 <BBA4E674-ABCC-4788-AE1C-8EB295B217FE@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <BBA4E674-ABCC-4788-AE1C-8EB295B217FE@amazon.com>
Archived-At: <https://www.postgresql.org/message-id/20230208202621.53mmhammbfu5mes6%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2023-02-08 20:04:52 +0000, Bagga, Rishu wrote:
> To summarize, our underlying effort is to move the SLRUs to the buffer
> cache. We were working with Thomas Munro off a patch he introduced here
> [1]. Munro’s patch moves SLRUs to the buffer cache by introducing the
> pseudo db id 9 to denote SLRU pages, but maintains the current “raw”
> data format of SLRU pages. The addition of page headers in our patch
> resolves this issue [2] Munro mentions in this email [3].
>
> Heikki Linnakangas then introduced  patch on top of Munro’s patch that
> modularizes the storage manager, allowing SLRUs to use it [4]. Instead
> of using db id 9, SLRUs use spcOid 9, and each SLRU is its own relation.
> Here, Heikki simplifies the storage manager by having each struct be
> responsible for just one fork of a relation; thus increasing
> extensibility of the smgr API, including for SLRUs. [5] We integrated
> our changes introducing page headers for SLRU pages, and upgrade logic
> to Heikki’s latest patch.

That doesn't explain the bulk of the changes here.  Why e.g. does any of the
above require RelationGetSmgr() to handle the fork as well? Why do we need
smgrtruncate_multi()? And why does all of this happens as one patch?

As is, with a lot of changes mushed together, without distinct explanations
for why is what done, this patch is essentially unreviewable. It'll not make
progress in this form.

It doesn't help that much to reference prior discussions in the email I'm
responding to - the patches need to be mostly understandable on their own,
without reading several threads.  And there needs to be explanations in the
code as well, otherwise we'll have no chance to understand any of this in a
few years.


> > > Rebased patch as per latest community changes since last email.
>
> > This version doesn't actually build.
> > https://cirrus-ci.com/task/4512310190931968
> > [19:43:20.131] FAILED:
> > src/test/modules/test_slru/test_slru.so.p/test_slru.c.o
>
>
> As for the build failures, I was using make install to build, and make
> check for regression tests.

There's a *lot* more tests than the main regression tests. You need to make
sure that they all continue to work. Enable tap tests and se check-world.


> The build failures in this link are from unit tests dependent on custom
> SLRUs, which would no longer apply. I’ve attached another patch that removes
> these tests.

I think you'd need to fix those tests, rather than just removing them.

I suspect we're going to continue to want SLRU specific stats, but your patch
also seems to remove those.


Greetings,

Andres Freund