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

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.