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)
