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

Hello Aleksander!
Hello Álvaro!
Thank you for your attention to the patch.

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()?

I agree with your proposals. Moreover, similar solution is also implemented and exploited at
Tantor's versions of the xid64 patch (/messages/by-id/5ca56aed-dc69-4c3e-968f-a822ae0937b5@gmail.com).

The corrected patch version implementing these proposals is attached.

Best regards,
Evgeny Voropaev,
Tantor Labs LLC.

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)

On 17.02.2025 21:00, Álvaro Herrera wrote:

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.

Created functions BootStrapSlruPage,SimpleLruZeroAndLogPage,
WriteSlruZeroPageXlogRec. Using of these functions allows to delete
ZeroXYZPage functions, WriteXYZZeroPageXlogRec functions and eliminate
code repetitions.

The conception is:
/*
* BootStrapSlruPage,
* SimpleLruZeroAndLogPage,
* SimpleLruZeroPage
* - functions nullifying SLRU pages.
*
* BootStrapSlruPage is the most holistic. It performs:
* 1. locking,
* 2. nullifying,
* 3. logging (when writeXlog is true),
* 4. writing out,
* 5. releasing the lock.
*
* SimpleLruZeroAndLogPage performs:
* 2. nullifying,
* 3. logging (when writeXlog is true),
* 4. writing out.
*
* If the writeXlog is true, BootStrapSlruPage and SimpleLruZeroAndLogPage
* emit an XLOG record saying we did this.
* If the writeXlog is false, the rmid and info parameters are unused.
*
* SimpleLruZeroPage performs:
* 2. nullifying.
*/

On 2025-Feb-18, Evgeny Voropaev wrote:

Created functions BootStrapSlruPage,SimpleLruZeroAndLogPage,
WriteSlruZeroPageXlogRec. Using of these functions allows to delete
ZeroXYZPage functions, WriteXYZZeroPageXlogRec functions and eliminate code
repetitions.

I think the overall idea here is good, but I didn't like the new
WriteSlruZeroPageXlogRec helper function; it looks too much like a
modularity violation (same for the fact that you have to pass the rmid
and info from each caller into slru.c). I would not do that in slru.c
but instead in every individual caller, and have this helper function in
xloginsert.c where it can be caller XLogSimpleInsert or something like
that.

Also, please do not document a bunch of functions together in a single
comment. Instead, have one comment for each function. I mean this
here:

diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 9ce628e62a5..cc069da19c6 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -363,6 +364,65 @@ check_slru_buffers(const char *name, int *newval)
return false;
}

+/*
+ * BootStrapSlruPage, 
+ * SimpleLruZeroAndLogPage, 
+ * SimpleLruZeroPage
+ * 		- functions nullifying SLRU pages.
+ *
+ * BootStrapSlruPage is the most holistic. It performs:
+ * 		1. locking,
+ * 		2. nullifying,
+ * 		3. logging (when writeXlog is true),
+ * 		4. writing out,
+ * 		5. releasing the lock.
+ *
+ * SimpleLruZeroAndLogPage performs:
+ * 		2. nullifying,
+ * 		3. logging (when writeXlog is true),
+ * 		4. writing out.
+ * 		
+ * If the writeXlog is true, BootStrapSlruPage and SimpleLruZeroAndLogPage 
+ * emit an XLOG record saying we did this.
+ * If the writeXlog is false, the rmid and info parameters are unused.
+ * 
+ * SimpleLruZeroPage performs:
+ * 		2. nullifying.
+ */
+void
+BootStrapSlruPage(SlruCtl ctl, int64 pageno,
+				  bool writeXlog, RmgrId rmid, uint8 info)
+{

This is not our style, and I don't think it's very ergonomical. Most
people don't read source files from top to bottom normally (heck,
sometimes I read source from bottom to top), so somebody looking at
SimpleLruZeroAndLogPage (the second function) might just not realize
that it's documented a few pagefuls above itself.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)

Hello hackers!

I think the overall idea here is good, but I didn't like the new
WriteSlruZeroPageXlogRec helper function; it looks too much like a
modularity violation (same for the fact that you have to pass the rmid
and info from each caller into slru.c). I would not do that in slru.c
but instead in every individual caller, and have this helper function in
xloginsert.c where it can be caller XLogSimpleInsert or something like
that.

Thank you Alvaro! I got your ideas and have tried to implement them.

In the v4 patch version:
1) WriteSlruZeroPageXlogRec has been renamed into XLogSimpleInsert and
moved to the xloginset.c.

2) With the purpose of excluding passing any resource manager’s
information into the slru.c module I propose to pass into the
BootStrapSlruPage the XLogInsertFunc parameter pointing to a function
accomplishing insertion of an XLog record. Implementations of these
functions are enclosed in a corresponding slru-page module (commit_ts.c,
multixaxt.c and so on). It preserves original modularity - the
BootStrapSlruPage and the slru.c still do not know anything about
resource managers. If the XLogInsertFunc parameter equals zero,
BootStrapSlruPage will not try to perform XLog recording.

3) In addition, let’s also implement in the BootStrapSlruPage a new
parameter writePage. It commands whether to write a page onto a disk or
not. As a result, the BootStrapSlruPage become apt to be used in a
larger number of places of code.

Also, please do not document a bunch of functions together in a single
comment. Instead, have one comment for each function.
This is not our style, and I don't think it's very ergonomical.

I got it. And since
4) The SimpleLruZeroAndLogPage is subject to deletion now.
comments are formatted as well.

sometimes I read source from bottom to top

Probably, we all should profess this approach!

Best regards,
Evgeny Voropaev,
Tantor Labs LLC.

Hello Hackers!

Álvaro, Andrey, Aleksander! We have not been discussing anything in the
thread for the past ten days. Can we mark this thread as "Ready for
Merge" or should I improve something in the patch. If I should, I am
ready to do it.

Looking forward to your feedback.
Evgeny Voropaev.

Hello Hackers!

On 11 March 2025, the CI job failed
(https://cirrus-ci.com/task/5453963400577024).

The issue occurred in the test ‘pg_amcheck/t/002_nonesuch.pl .

Log:

https://api.cirrus-ci.com/v1/artifact/task/5453963400577024/testrun/build/testrun/pg_amcheck/002_nonesuch/log/regress_log_002_nonesuch

Log output:
--------------------------------------------------------------------------------------
[11:40:20.527](0.000s) not ok 18 - checking with a non-existent user
stderr /(?^:role "no_such_user" does not exist)/
[11:40:20.527](0.000s) # Failed test 'checking with a non-existent
user stderr /(?^:role "no_such_user" does not exist)/'
# at C:/cirrus/src/bin/pg_amcheck/t/002_nonesuch.pl line 86.
[11:40:20.527](0.000s) # 'pg_amcheck: error:
connection to server on socket
"C:/Windows/TEMP/AtJUArHHFu/.s.PGSQL.17536" failed: server closed the
connection unexpectedly
# This probably means the server terminated abnormally
# before or while processing the request.
# '
# doesn't match '(?^:role "no_such_user" does not exist)'
# Running: pg_amcheck template1
----------------------------------------------------------------------------------

It appears to be a flickering issue that regularly occurs on the Windows
platform and is not related to the patch.

For the purpose of restarting the CI job, I have attached the 5th
version of the patch. It is identical to v4, except for the version
number increment, which helps prevent duplicate attachment names in this
thread.

Best regards,
Evgeny Voropaev.

On 11 Mar 2025, at 14:12, Evgeny <evorop@gmail.com> wrote:

Hi!

Some nits:

Patch adds whitespace errors
.git/rebase-apply/patch:64: trailing whitespace.
* Nullify the page (pageno = 0), don't insert an XLog record, .git/rebase-apply/patch:212: trailing whitespace.
/* .git/rebase-apply/patch:213: trailing whitespace.
* Zero the page; .git/rebase-apply/patch:250: trailing whitespace.
.git/rebase-apply/patch:349: trailing whitespace.
* Nullify the page (pageno = 0), don't insert an XLog record, warning: squelched 10 whitespace errors
warning: 15 lines add whitespace errors.

if (writePage != 0) should be if (writePage)

XLogSimpleInsert(int64 simpledata, RmgrId rmid, uint8 info)
I’d rename function XLogSimpleInsert() to something more descriptive and changed arguments order from generic to specific. Probably, committer has broader view on XLog routines and can decide if this function would better belong to SLRU than common XLog stuff.

Besides this patch seems ready to me.

Thanks!

Best regards, Andrey Borodin.

Hello Hackers!

Andrey, thank you for your review and remarks.

Patch adds whitespace errors

Cleaned.

if (writePage != 0) should be if (writePage)

Done.

XLogSimpleInsert(int64 simpledata, RmgrId rmid, uint8 info)
I’d rename function XLogSimpleInsert() to something more descriptive
and changed arguments order from generic to specific.

The function has been renamed and the parameters have been reordered.
Now we have:
XLogInsertInt64(RmgrId rmid, uint8 info, int64 simpledata)

Probably, committer has broader view on XLog routines and can decide
if this function would better belong to SLRU than common XLog stuff.

In accordance with Álvaro's proposal, we want to enclose this function
in the "xloginsert.c" module.

Best regards,
Evgeny Voropaev.

On 12 Mar 2025, at 20:02, Evgeny Voropaev <evorop.wiki@gmail.com> wrote:

v6 looks good to me. I'll flip the CF entry.

Thanks!

Best regards, Andrey Borodin.

Hello

I think this is going too far. The new function BootStrapSlruPage() now
is being used for things other than bootstrapping, and that doesn't seem
appropriate to me. I think we should leave the things that aren't
bootstrap out of this patch. For instance, ExtendCLOG was more
understandable before this patch than after. Same with
ExtendMultiXact{Offset,Member}, ExtendSUBTRANS, etc.

By removing these changes, you can remove the third argument to
BootStrapSlruPage (the function pointer), since you don't need it.

I'm also very suspicious of the use of the new function in the redo
routines also, because those are not bootstrapping anything. I'd rather
have another routine, say SimpleLruRedoZeroPage() which only handles the
zeroing of a page from a WAL record. It would be very similar to
BootstrapSlruPage, and maybe they can share an internal "workhorse"
function for the bits that are common.

I don't have faith in the function name XLogInsertInt64(). The datatype
has no role there. I liked my proposal better.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Hello Hackers!

I think this is going too far. The new function BootStrapSlruPage() now
is being used for things other than bootstrapping, and that doesn't seem
appropriate to me. I think we should leave the things that aren't
bootstrap out of this patch. For instance, ExtendCLOG was more
understandable before this patch than after. Same with
ExtendMultiXact{Offset,Member}, ExtendSUBTRANS, etc.

I have excluded the uses of BootStrapSlruPage in Extend* functions. The
code of these functions is remaining still modified a bit on account of
Zero* functions having been deleted. It now includes such fragments:

/* Zero the page and make an XLOG entry about it */
SimpleLruZeroPage(MultiXactMemberCtl, pageno);
XLogSimpleInsert(RM_MULTIXACT_ID, XLOG_MULTIXACT_ZERO_MEM_PAGE, pageno);

as a substitution of:
/* Zero the page and make an XLOG entry about it */
ZeroMultiXactOffsetPage(pageno, true);

But, probably, it even makes a code more readable showing both actions
clearly.

By removing these changes, you can remove the third argument to
BootStrapSlruPage (the function pointer), since you don't need it.

Indeed, it has helped to remove an input parameter from the
BootStrapSlruPage function. Honestly, even two arguments are removed,
since code has not opted whether to write a page on a disk or not. It
saves a page every time.

I'm also very suspicious of the use of the new function in the redo
routines also, because those are not bootstrapping anything. I'd rather
have another routine, say SimpleLruRedoZeroPage() which only handles the
zeroing of a page from a WAL record. It would be very similar to
BootstrapSlruPage, and maybe they can share an internal "workhorse"
function for the bits that are common.

Now the logic zeroing page on the "do" side is fully equal to one on the
"redo" side. As a result, creation of a new distinct function for "redo"
is not needed. In order for the function to conform to the both sides
("do" and "redo") I have renamed it into SimpleLruZeroPageExt. If this
name looks not appropriate, we can change it, please, propose new one.

I don't have faith in the function name XLogInsertInt64(). The datatype
has no role there. I liked my proposal better.

The name has been reverted to one Álvaro has proposed at first
(XLogSimpleInsert).

Patch looks even better - it reduce code by 180 lines.

Best regards,
Evgeny Voropaev.

Álvaro, Andrey, Alexander, hello!

Since the master branch became the PG19dev and PG18 is now stable, I
have directed this patch into PG19. Could we continue with it now?

Álvaro, should I rename the SimpleLruZeroPageExt function?

Best regards,
Evgeny Voropaev.

On 2025-Jul-02, Evgeny wrote:

Álvaro, Andrey, Alexander, hello!

Since the master branch became the PG19dev and PG18 is now stable, I have
directed this patch into PG19. Could we continue with it now?

Sure, I pushed your patch now.

Álvaro, should I rename the SimpleLruZeroPageExt function?

Well, I didn't like that name -- normally, names ending in Ext represent
a version of the routine named without the "Ext" that has some additions
to its argument list, so the Ext is an extended version or something
like that. That pattern does not fit this case. I used the name
"SimpleLruZeroAndWritePage" instead, and rewrote the comment to explain
that it's a simple wrapper that does exactly what the name says.
Because of this I also removed some comments in the callsites, because
those would have been redundant with the new name. So, no you don't
need to do anything, because I already did it.

I also went back and accepted Andrey's suggestion to have Int64 in the
name of the XLogSimpleInsert routine, because it's not totally
unthinkable that we'll have some other simple wrapper in the future.
I made it return the LSN, because while no current caller needs it, some
external caller might want to have that.

I also moved each routine to a more natural place, namely just below the
function they wrap. The pattern of adding stuff at the end of the file
just results in messy code, so don't do that.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)

Álvaro, thank you for pushing, attention and final editing of the patch!
I got your recommendations and requirements. I will take them into
account at a next patch.

Andrey, Alexander thank you for your proposals, attention, advice, and
review!

Best regards,
Evgeny Voropaev,
Tantor Labs, LLC.