> On Oct 8, 2026, at 05:20, Ayush Tiwari <[email protected]> wrote:
>
> Hi,
>
> During crash startup we sync the unlogged forks, only to remove them later.
> ISTM we could avoid that writeback by doing the cleanup first.
>
> The attached patch puts the cleanup after InitWalRecovery() has restored
> any tablespace links, but before SyncDataDirectory(). Init forks and
> surviving files are still synced before replay, and the end-of-recovery
> checkpoint stays where it is.
>
> I've kept the later cleanup for recovery after a clean shutdown. We need
> pg_control to record recovery first, otherwise a failed start could leave
> missing unlogged files without forcing recovery on the next normal start.
>
> AFAICS this should help recovery_init_sync_method=syncfs too, since it
> can't skip individual files.
>
> I got the following startup times on a Linux VM (8 vCPUs, 32 GB RAM) with
> ext4, using a 1.14 GiB unlogged table (four runs each):
>
> Method Unpatched median (range) Patched median
> fsync 26.34 s (21.83-27.34) 0.40 s
> syncfs 22.58 s (19.43-23.93) 0.30 s
>
> The empty-table fsync control had a 0.30 s median for both builds.
>
> Does this ordering look reasonable, or am I missing a reason the initial
> sync needs to happen before InitWalRecovery()?
>
> Regards,
> Ayush
>
> P.S. These runs were right after a load and pg_ctl stop -m immediate,
> without a manual cache flush, so there may still have been dirty data in
> the OS page cache at restart (which could explain such big gains)
> <v1-0001-Reset-unlogged-relations-before-syncing-the-data-directory.patch>
Hi Ayush,
I agree with the optimization, though the benefit will depend on how much
unlogged data remains dirty in the OS cache. I cannot think of any adverse
effects.
WRT the code change, ResetUnloggedRelations used to be called only if
InRecovery is true. Should we keep that explicit check, like:
```
if (didCrash)
{
if (InRecovery)
ResetUnloggedRelations(UNLOGGED_RELATION_CLEANUP);
SyncDataDirectory();
}
```
I understand that, in the current implementation, didCrash implies InRecovery
after InitWalRecovery() returns, so the check is redundant, but I feel it makes
the code logic clearer. If you don’t want to add the check, adding a comment
explaining that implication also works for me.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
