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

From: Justin Pryzby <pryzby(at)telsasoft(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Nitin Jadhav <nitinjadhavpostgres(at)gmail(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
Subject: Re: when the startup process doesn't (logging startup delays)
Date: 2021-07-26 15:30:23
Message-ID: 20210726153023.GC23997@telsasoft.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-Shows-the-progress-of-the-operations-performed-durin.patch text/x-diff 14.6 KB
0002-justins-changes.patch text/x-diff 9.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexander Pyhalov 2021-07-26 16:03:54 Re: Case expression pushdown
Previous Message Tom Lane 2021-07-26 15:18:29 Re: Case expression pushdown