On 08/02/2023 22:26, Andres Freund wrote:
> 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.
Agreed. I rebased this over my rebased patch set from the other thread
at
https://www.postgresql.org/message-id/02825393-615a-ac81-0f05-f3cc2e6f875f%40iki.fi.
Attached is a new patch with only the changes relative to that patch set.
This is still messy, but now I can see what the point is: make the
SLRUs, which are tracked in the main buffer pool thanks to the other
patches, use the standard page header.
I'm not sure if I like that or not. I think we should clean up and
finish the other patches that this builds on first, and then decide if
we want to use the standard page header for the SLRUs or not. And if we
decide that we want the SLRU pages to have a page header, clean this up
or rewrite it from scratch.
- Heikki