Re: when the startup process doesn't (logging startup delays)
Nitin Jadhav <nitinjadhavpostgres@gmail.com>
From: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.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" <pgsql-hackers@postgresql.org>
Date: 2021-08-18T06:53:55Z
Lists: pgsql-hackers
> Anything that's not used in other files should be declared static in > the file itself, and not present in the header. Once you fix this for > reset_startup_progress_timeout, the header won't need to include > datatype/timestamp.h any more, which is good, because we don't want > header files to depend on more other header files than necessary. Thanks for identifying this. I will take care in the next patch. > Looking over this version, I realized something I (or you) should have > noticed sooner: you've added the RegisterTimeout call to > InitPostgres(), but that's for things that are used by all backends, > and this is only used by the startup process. So it seems to me that > the right place is StartupProcessMain. That would have the further > advantage of allowing startup_progress_timeout_handler to be made > static. begin_startup_progress_phase() and > startup_progress_timeout_has_expired() are the actual API functions > though so they will need to remain extern. Yes. I had noticed this earlier and the RegisterTimeout() call was only present in StartupProcessMain() and not in InitPostgres() in the earlier versions (v7) of the patch. Since StartupXLOG() gets called in the 2 places, I had restricted the InitPostgres() flow by checking for the !AmStartupProcess() in the newly added functions. But later we had discussion and concluded to add the RegisterTimeout() call even in case of InitPostgres(). Kindly refer to the discussion just after the v7 patch in this thread and let me know your thoughts. > This hunk should be removed. I will remove it in the next patch. On Tue, Aug 17, 2021 at 1:08 AM Robert Haas <robertmhaas@gmail.com> wrote: > > On Sat, Aug 14, 2021 at 5:47 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > Should this feature distinguish between crash recovery and archive recovery on > > a hot standby ? Otherwise the standby will display this all the time. > > > > 2021-08-14 16:13:33.139 CDT startup[11741] LOG: redo in progress, elapsed time: 124.42 s, current LSN: 0/EEE2100 > > > > If so, I think maybe you'd check !InArchiveRecovery (but until Robert finishes > > cleanup of xlog.c variables, I can't say that with much confidence). > > Hmm. My inclination is to think that on an actual standby, you > wouldn't want to get messages like this, but if you were doing a > point-in-time-recovery, then you would. So I think maybe > InArchiveRecovery is not the right thing. Perhaps StandbyMode? > > -- > Robert Haas > EDB: http://www.enterprisedb.com
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