On Fri, Jul 23, 2021 at 04:09:47PM +0530, Nitin Jadhav wrote: > > I think walkdir() should only call LogStartupProgress(FSYNC_IN_PROGRESS, > > path); > > when action == datadir_fsync_fname. > > I agree and fixed it.
I saw that you fixed it by calling InitStartupProgress() after the walkdir() calls which do pre_sync_fname. So then walkdir is calling LogStartupProgress(STARTUP_PROCESS_OP_FSYNC) even when it's not doing fsync, and then LogStartupProgress() is returning because !AmStartupProcess(). That seems indirect, fragile, and confusing. I suggest that walkdir() should take an argument for which operation to pass to LogStartupProgress(). You can pass a special enum for cases where nothing should be logged, like STARTUP_PROCESS_OP_NONE. On Wed, Jul 21, 2021 at 04:47:32PM +0530, Bharath Rupireddy wrote: > 1) I still don't see the need for two functions LogStartupProgress and > LogStartupProgressComplete. Most of the code is duplicate. I think we > can just do it with a single function something like [1]: I agree that one function can do this more succinctly. I think it's best to use a separate enum value for START operations and END operations. switch(operation) { case STARTUP_PROCESS_OP_SYNCFS_START: ereport(...); break; case STARTUP_PROCESS_OP_SYNCFS_END: ereport(...); break; case STARTUP_PROCESS_OP_FSYNC_START: ereport(...); break; case STARTUP_PROCESS_OP_FSYNC_END: ereport(...); break; ... -- Justin