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.
Attachment
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.
Attachment
> 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 SLRUthan 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.
Attachment
> 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.