Re: when the startup process doesn't (logging startup delays)

Justin Pryzby <pryzby@telsasoft.com>

From: Justin Pryzby <pryzby@telsasoft.com>
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>, Stephen Frost <sfrost@snowman.net>, Magnus Hagander <magnus@hagander.net>, Thomas Munro <thomas.munro@gmail.com>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2021-07-25T17:56:54Z
Lists: pgsql-hackers
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



Commits

  1. Un-revert "Disable STARTUP_PROGRESS_TIMEOUT in standby mode."

  2. Revert "Disable STARTUP_PROGRESS_TIMEOUT in standby mode."

  3. Disable STARTUP_PROGRESS_TIMEOUT in standby mode.

  4. Fix race condition in startup progress reporting.

  5. Report progress of startup operations that take a long time.

  6. Add enable_timeout_every() to fire the same timeout repeatedly.