Re: when the startup process doesn't (logging startup delays)
Justin Pryzby <pryzby@telsasoft.com>
From: Justin Pryzby <pryzby@telsasoft.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Nitin Jadhav <nitinjadhavpostgres@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>, Stephen Frost <sfrost@snowman.net>, Magnus Hagander <magnus@hagander.net>, Thomas Munro <thomas.munro@gmail.com>, pgsql-hackers@postgresql.org
Date: 2021-07-26T15:30:23Z
Lists: pgsql-hackers
Attachments
On Mon, Jul 26, 2021 at 10:13:09AM -0400, Robert Haas wrote: > I don't think walkdir() has any business calling LogStartupProgress() > at all. It's supposed to be a generic function, not one that is only > available to be called from the startup process, or has different > behavior there. From my point of view, the right thing is to put the > logging calls into the particular callbacks that SyncDataDirectory() > uses. You're right - this is better. 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. > > Maybe I'm missing something here, but I don't understand the purpose > of this. You can always combine two functions into one, but it's only > worth doing if you end up with less code, which doesn't seem to be the > case here. 4 files changed, 39 insertions(+), 71 deletions(-) > ... but I'm not exactly sure how to get there from here. Having only > LogStartupProgress() but having it do a giant if-statement to figure > out whether we're mid-phase or end-of-phase does not seem like the > right approach. I used a bool arg and negation to handle within a single switch. Maybe it's cleaner to use a separate enum value for each DONE, and set a local done flag. startup[29675] LOG: database system was interrupted; last known up at 2021-07-26 10:23:04 CDT startup[29675] LOG: syncing data directory (fsync), elapsed time: 1.38 s, current path: ./pg_ident.conf startup[29675] LOG: data directory sync (fsync) complete after 1.72 s startup[29675] LOG: database system was not properly shut down; automatic recovery in progress startup[29675] LOG: resetting unlogged relations (cleanup) complete after 0.00 s startup[29675] LOG: redo starts at 0/17BE500 startup[29675] LOG: redo in progress, elapsed time: 1.00 s, current LSN: 0/35D7CB8 startup[29675] LOG: redo in progress, elapsed time: 2.00 s, current LSN: 0/54A6918 startup[29675] LOG: redo in progress, elapsed time: 3.00 s, current LSN: 0/7370570 startup[29675] LOG: redo in progress, elapsed time: 4.00 s, current LSN: 0/924D8A0 startup[29675] LOG: redo done at 0/9FFFFB8 system usage: CPU: user: 4.28 s, system: 0.15 s, elapsed: 4.44 s startup[29675] LOG: resetting unlogged relations (init) complete after 0.03 s startup[29675] LOG: checkpoint starting: end-of-recovery immediate startup[29675] LOG: checkpoint complete: wrote 9872 buffers (60.3%); 0 WAL file(s) added, 0 removed, 8 recycled; write=0.136 s, sync=0.801 s, total=1.260 s; sync files=21, longest=0.774 s, average=B
Commits
-
Un-revert "Disable STARTUP_PROGRESS_TIMEOUT in standby mode."
- ecb01e6ebb5a 15.3 landed
-
Revert "Disable STARTUP_PROGRESS_TIMEOUT in standby mode."
- 1eadfbdd7eb0 15.2 landed
-
Disable STARTUP_PROGRESS_TIMEOUT in standby mode.
- 98e7234242a6 15.2 landed
- 8a2f783cc489 16.0 landed
-
Fix race condition in startup progress reporting.
- 5ccceb2946d4 15.0 landed
-
Report progress of startup operations that take a long time.
- 9ce346eabf35 15.0 landed
-
Add enable_timeout_every() to fire the same timeout repeatedly.
- 732e6677a667 15.0 landed