| From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
|---|---|
| To: | Ayush Tiwari <ayushtiwari(dot)slg01(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 00:21:00 |
| Message-ID: | CALj2ACU5gDxE=pMeYFK2egP5OWGHbGFrdqTCY5_-H_KU_VE7ug@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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());
>> 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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Sami Imseih | 2026-08-16 00:23:32 | Re: Disallow outer-level and WHERE-clause aggregates in GRAPH_TABLE |
| Previous Message | Peter Geoghegan | 2026-08-16 00:15:23 | Re: GiST wal_consistency_checking issue |