Re: when the startup process doesn't (logging startup delays) - Mailing list 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.

Fixed.

> > This hunk should be removed.
>
> I will remove it in the next patch.

Removed.

Please find the updated patch attached.

On Wed, Aug 18, 2021 at 12:23 PM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
>
> > 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