Hi, On Fri, Aug 14, 2026 at 7:16 AM Ayush Tiwari <[email protected]> wrote: > >> > Checkpoint: RemoveOldXlogFiles does one ReadDir, and each future segment >> > costs a single strcmp. Should be marginal(?) >> >> I didn't measure this, but could you give some numbers for the record >> here to ensure we don't leave that cost unmeasured. > > I tried measuring this on an Azure Standard_D8as_v5 VM with ext4 on a > Premium_LRS > disk. I used separate clusters with 1, 641, and 6401 WAL files, and ran > three order-rotated rounds of 500 checkpoints per condition. > > With fsync enabled, median checkpoint times were 14.890 ms, 14.652 ms, > and 17.141 ms respectively. Adding 640 WAL files caused no measurable > slowdown on this setup. Adding 6400 files increased the median from > 14.89 ms to 17.14 ms, about 2.25 ms. > > With fsync disabled to isolate directory scanning, the medians were 0.470 ms, > 0.758 ms, and 3.271 ms, or roughly 0.44 to 0.45 microseconds per additional > file.
I think a few milliseconds per 6K+ WAL files should be okay. I'm okay with this. Thanks for measuring this. >> We expect the PreallocXlogSegments() caller to ensure the database is >> not in recovery. I think the startup process disabling segment >> installation is also as important as the recovery-in-progress check >> and the caller must error out in such cases. I suggest moving these >> into the caller and having them as asserts at the beginning of >> PreallocXlogSegments(). > > That makes sense to me. My thought is to move both checks to the SQL caller, > use the locked accessor for the installation state, and leave assertions in > the internal helper. If that's what you had in mind too, I can add this in > v6. > Thoughts? >> >> 2/ Also, do we ever hit the case where the startup process disables >> segment installation in the WAL files allocation loop in >> PreallocXlogSegments()? AFAICS, maybe not. During crash recovery when >> the startup process sets this flag in StartupXLOG(), at that point >> RecoveryInProgress() is still true. > > I couldn't find a normal SQL-callable state where installation is disabled > independently of recovery. That seems to support your point that this should > be a caller precondition rather than a second runtime path in the helper. I don't think taking the control file lock (which IsInstallXLogFileSegmentActive() acquires) in this SQL function path is a good idea. It can easily increase lock contention on the control file lock, which is used internally to update important control file fields. This boils down to the following: when the database is not in recovery (that is, RecoveryInProgress() is false), can the startup process ever block new WAL file creation? It turns out it cannot, so the invariant is Assert(RecoveryInProgress() || IsInstallXLogFileSegmentActive()). I would just keep this in pg_wal_preallocate(): if (RecoveryInProgress()) ereport(ERROR .....); /* * When idatabase is not in recovery, new WAL file creation is not * blocked by the startup process, so assert that invariant here. */ Assert(IsInstallXLogFileSegmentActive()); >> 4/ >> >> Docs: >> + space occupied for a long time. On file systems where >> recycling a WAL file >> + is not cheaper than creating a new one (for example copy-on-write >> file >> + systems, see <xref linkend="guc-wal-recycle"/>), preallocation >> provides >> + little benefit. This function cannot be executed during recovery. >> >> My experimentation says otherwise. CoW still gains about 11% with >> zero-fill on, and shows no gain with it off. Can we be more specific >> in the docs and commit message? > > Hmm, you are right about this, and results do show that. I think it > should describe the wal_init_zero distinction directly: CoW can still benefit > when zero-fill is enabled, but little benefit should be expected when it is > disabled. Will edit it. Sounds good, but use the GUC name directly instead of "zero-fill" and avoid saying "little benefit should be expected." >> 5/ Can we think of deduplicating the for loop in >> PreallocXlogSegments() with PreallocXlogFiles()? Also the naming of >> the new function and the existing function looks similar (WAL segment >> is an internal term used for WAL file). > > I think a small helper for the common one-file initialization and close > sequence would help. I'll also try names that distinguish on-demand work > from checkpoint preallocation more clearly. Thinking about it more, I'm fine to keep a separate function and leave PreallocXlogFiles() as-is to avoid any backpatching issues. Just name the new function something like PreallocNXlogFiles() and keep it closer to PreallocXlogFiles() in the code. >> 6/ >> + allows_streaming => 1, extra => ['--wal-segsize=16']); >> >> Why not use wal segment size 1MB for testing and use min and max >> wal_size in multiples of 1MB? This can make the tests a bit faster. >> >> 7/ Tests look too many. I don't think we need to cover all the cases. >> One positive case and one negative case should be enough. No need to >> cover for recovery-in-progress errors and all, because that just >> works. This keeps the number of tests to 1 or 2 and you can even think >> of adding them to an existing closely related TAP test without the >> need to start and stop another server for this (I'm aware of the fact >> that our testing infrastructure isn't free). > > You're right that the test can be smaller, but I'd keep the exact insertion > boundary case because it caught a real bug. A one-node prototype with 1 MB > segments and eight assertions passed and cut runtime to about half. > Does that seem like a reasonable balance? Sounds good. Just keep the test comments and descriptions minimal with key details. > I'm still inclined to keep force. Raising max_wal_size changes global > checkpoint scheduling, whereas force scopes the choice to one explicit > superuser call. The concern is legitimate, though, and I'd like to hear if > others prefer removing the escape hatch entirely. Increasing max_wal_size alone doesn't block checkpoints, timeout-based checkpoints can still occur. So, getting rid of the force option seems the right choice to address accidentally or intentionally preallocating too many files, eating up storage, and affecting checkpoints (even though only a little). A simple note in the docs would suffice: the number of WAL files preallocated by this function is limited by max_wal_size, and if a benchmark or bulk load operation requires more WAL files to be preallocated, increase max_wal_size accordingly. I know this sounds a bit complex, but it addresses the too-many-files eating up storage issue. And that should be okay IMO. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
