| From: | Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> |
|---|---|
| To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
| Cc: | Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>, solai v <solai(dot)cdac(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Ian Lawrence Barwick <barwick(at)gmail(dot)com> |
| Subject: | Re: Add a pg_wal_preallocate() SQL function to eagerly create future WAL segments |
| Date: | 2026-08-16 15:08:53 |
| Message-ID: | CAJTYsWUSvu2LSFbfag4yaqt+5NwaTAdAz_jqB0FqKT8mRhTQbA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
On Sun, 16 Aug 2026 at 05:51, Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> Hi,
>
> On Fri, Aug 14, 2026 at 7:16 AM Ayush Tiwari
> <ayushtiwari(dot)slg01(at)gmail(dot)com> 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
| Attachment | Content-Type | Size |
|---|---|---|
| v6-0001-pg_wal_preallocate.patch | application/octet-stream | 14.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | ZizhuanLiu X-MAN | 2026-08-16 15:42:15 | Re: examine_variable ignored CollateExpr |
| Previous Message | Salma El-Sayed | 2026-08-16 14:47:00 | Re: [GSoC 2026] - B-tree Index Bloat Reduction - Approach & Questions |