Hi, On Sun, 16 Aug 2026 at 05:51, Bharath Rupireddy < [email protected]> wrote:
> 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()); > Thanks, done this way. > >> 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." > I've edited the commit message and docs, to sound better. > >> 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. > Sounds good. >> 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. > Done. > > 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. > Upon thinking more, I've removed the force option for now. We can revisit/add it if and when need be, adding something like that is easier than removing it later. v6 attached. Thoughts? Regards, Ayush
v6-0001-pg_wal_preallocate.patch
Description: Binary data
