> On Oct 8, 2026, at 05:20, Ayush Tiwari <ayushtiwari.slg01@gmail.com> 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.
Icannot 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
checkis redundant, but I feel it makes the code logic clearer. If you don’t want to add the check, adding a comment
explainingthat implication also works for me.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
