On 28/11/2025 20:17, Andrey Borodin wrote:
> latest_page_number looks great! though I'm slightly worried by this commend:
> /*
> * During XLOG replay, latest_page_number isn't necessarily set up
> * yet; insert a suitable value to bypass the sanity test in
> * SimpleLruTruncate.
> */
>
> and by
>
> /*
> * latest_page_number is the page number of the current end of the log;
> * this is __not critical data__, since we use it only to avoid swapping out
> * the latest page.
> */
>
> If we use it in startup process to prevent failure, now it is critical data.
>
> But I think the comments are off, latest_page_number is updated precisely.
Agreed. I looked at the commit history of how the first comment got
introduced, to see if it used to be true at some point, but AFAICS we've
always kept latest_page_number up-to-date. It is very clearly
initialized in StartupMultiXact(), before WAL replay. Furthermore, we
only set the "suitable value to bypass the sanity test" for the offsets
SLRU, but everywhere else where we truncate, extend or set
latest_page_number, we treat the members SLRU. So I don't see how the
offsets and members SLRUs could be different in that regard.
I think the second comment became outdated in commit bc7d37a525c0, which
introduced the safety check in (what became later) SimpleLruTruncate().
After that, it's been important that latest_page_number is set
correctly, although for the sanity check I guess you could be a little
sloppy with it.
> The page initialization dance is only needed in back branches. And
> we inevitable will have conflicts with SLRU refactoring in 18 and
> banking in 17. Conceptually v12 looks good to me, I can prepare
> backport versions.
Thanks!
Here's a new patch version. I went through the test changes now:
I didn't understand why the 'kill9' and 'poll_start' stuff is needed. We
have plenty of tests that kill the server with regular
"$node->stop('immediate')", and restart the server normally. The
checkpoint in the middle of the tests seems unnecessary too. I removed
all that, and the test still seems to work. Was there a particular
reason for them?
I moved the wraparound test to a separate test file and commit. More
test coverage is good, but it's quite separate from the bugfix and the
wraparound related test shares very little with the other test. The
wraparound test needs a little more cleanup: use plain perl instead of
'dd' and 'rm' for the file operations, for example. (I did that with the
tests in the 64-bit mxoff patches, so we could copy from there.)
- Heikki
