Re: Reset unlogged relations before syncing the data directory?

Hi,

On Sat, 10 Oct 2026 at 07:36, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> > On Oct 8, 2026, at 05:20, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)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.
> >
> > 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>

Thanks for looking at this.

> I agree with the optimization, though the benefit will depend on how much
> unlogged data remains dirty in the OS cache.

Agreed. These runs were right after a load and immediate shutdown; I haven't
measured dirty pages per relation.

> WRT the code change, ResetUnloggedRelations used to be called only if
> InRecovery is true. Should we keep that explicit check, like:

I've added the explicit InRecovery check in the attached v2. It's redundant
today, as you noted, but makes it clear that cleanup is only for recovery.
The rest of the patch is unchanged.

Regards,
Ayush