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

From: Nitin Jadhav <nitinjadhavpostgres(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)anarazel(dot)de>, Stephen Frost <sfrost(at)snowman(dot)net>, Magnus Hagander <magnus(at)hagander(dot)net>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: when the startup process doesn't (logging startup delays)
Date: 2021-08-18 06:53:55
Message-ID: CAMm1aWZrnYp1WocvoXcR2W2CGuqhgO+UjA9yz29kT6j1Axe=-w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
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(at)gmail(dot)com> wrote:
>
> On Sat, Aug 14, 2021 at 5:47 PM Justin Pryzby <pryzby(at)telsasoft(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message houzj.fnst@fujitsu.com 2021-08-18 07:19:07 RE: Skipping logical replication transactions on subscriber side
Previous Message Masahiko Sawada 2021-08-18 06:41:27 Re: Skipping logical replication transactions on subscriber side