Thread: Re: Elimination of the repetitive code at the SLRU bootstrap functions.

Hi Evgeny,

> The functions, bootstrapping SLRU pages, such as BootStrapMultiXact,
> BootStrapCLOG, ActivateCommitTs, multixact_redo and others, have a lot
> of repetitive code.
>
> A new proposed function BootStrapSlruPage moves a duplicating code into
> the single place. Additionally, a new member ZeroFunc is implemented in
> the SlruCtlData structure. The ZeroFunc keeps a pointer to a function
> proper for nullifying SLRU pages of a type defined by a corresponding
> SlruCtlData object.
>
> Applying proposed modifications can simplify maintenance and future
> development of Postgres code in the realm of bootstrapping SLRU.

Thanks for the patch.

```
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -102,14 +102,6 @@ TransactionIdToPage(TransactionId xid)
  */
 #define THRESHOLD_SUBTRANS_CLOG_OPT    5

-/*
- * Link to shared-memory data structures for CLOG control
- */
-static SlruCtlData XactCtlData;
-
-#define XactCtl (&XactCtlData)
-
-
 static int    ZeroCLOGPage(int64 pageno, bool writeXlog);
 static bool CLOGPagePrecedes(int64 page1, int64 page2);
 static void WriteZeroPageXlogRec(int64 pageno);
@@ -130,6 +122,14 @@ static void
TransactionIdSetPageStatusInternal(TransactionId xid, int nsubxids,
                                                XLogRecPtr lsn, int64 pageno);

+/*
+ * Link to shared-memory data structures for CLOG control
+ */
+static SlruCtlData XactCtlData = { .ZeroPage = ZeroCLOGPage };
+
+#define XactCtl (&XactCtlData)
+
+
```

Since BootStrapSlruPage() is the only caller of ZeroPage() it seems to
me that it merely wastes space in SlruCtlData. On top of that I'm not
100% sure if all the supported platforms have C99 compilers with
designated initializers support. Wouldn't it be simpler to pass the
callback straight to BootStrapSlruPage()?

After changing the patch accordingly it shouldn't move XactCtl and
others. This is just an irrelevant change.

-- 
Best regards,
Aleksander Alekseev

On 2025-Feb-13, Aleksander Alekseev wrote:

> Since BootStrapSlruPage() is the only caller of ZeroPage() it seems to
> me that it merely wastes space in SlruCtlData. On top of that I'm not
> 100% sure if all the supported platforms have C99 compilers with
> designated initializers support.

They do -- we use them quite extensively nowadays.

> Wouldn't it be simpler to pass the callback straight to
> BootStrapSlruPage()?

Yeah, maybe this would be easier.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Just treat us the way you want to be treated + some extra allowance
 for ignorance."                                    (Michael Brusser)

Hi Alvaro,

> > Since BootStrapSlruPage() is the only caller of ZeroPage() it seems to
> > me that it merely wastes space in SlruCtlData. On top of that I'm not
> > 100% sure if all the supported platforms have C99 compilers with
> > designated initializers support.
>
> They do -- we use them quite extensively nowadays.

Got it, thanks. I was a bit worried about Visual Studio in particular.

-- 
Best regards,
Aleksander Alekseev

> On 14 Feb 2025, at 11:54, Evgeny Voropaev <evgeny.voropaev@tantorlabs.com> wrote:
>
> <v2-0001-Elimination-of-the-repetitive-code-at-the-SLRU-bo.patch>

Hi! Nice patch!

BootStrapSlruPage() always calls zerofunc(pageno, false) with second argument false.
In case of every possible argument (ZeroCLOGPage, ZeroCommitTsPage, ZeroMultiXactOffsetPage, ZeroMultiXactMemberPage,
ZeroSUBTRANSPage)it means just a call to SimpleLruZeroPage(). 
I think we can safely replace

+ slotno = (*zerofunc)(pageno, false);

with

+ slotno = SimpleLruZeroPage(pageno);

Thus we will not need zerofunc argument at all.

Thanks!


Best regards, Andrey Borodin.

On 2025-Feb-17, Andrey Borodin wrote:

> BootStrapSlruPage() always calls zerofunc(pageno, false) with second argument false.
> In case of every possible argument (ZeroCLOGPage, ZeroCommitTsPage,
> ZeroMultiXactOffsetPage, ZeroMultiXactMemberPage, ZeroSUBTRANSPage) it
> means just a call to SimpleLruZeroPage().
> I think we can safely replace
> 
> + slotno = (*zerofunc)(pageno, false);
> 
> with
> 
> + slotno = SimpleLruZeroPage(pageno);
> 
> Thus we will not need zerofunc argument at all.

Good observation.  This also suggests another change: because this new
function is used not only for bootstrapping but also during WAL replay,
we can call the new function SimpleLruUnloggedZeroPage() and place it
immediately after SimpleLruZeroPage, instead of at the end of the file.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La persona que no quería pecar / estaba obligada a sentarse
 en duras y empinadas sillas    / desprovistas, por cierto
 de blandos atenuantes"                          (Patricio Vogel)