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

On Tue, Aug 10, 2021 at 9:28 AM Nitin Jadhav
<nitinjadhavpostgres(at)gmail(dot)com> wrote:
> Please find the updated patch attached.

I think this is getting close. The output looks nice. However, I still
see a bunch of issues.

You mentioned previously that you would add documentation, but I do
not see it here.

startup_progress_timer_expired should be declared as sig_atomic_t like
we do in other cases (see interrupt.c).

The default value of the new GUC is 10s in postgresql.conf.sample, but
-1 in guc.c. They should both be 10s, and the one in
postgresql.conf.sample should be commented out.

I suggest making the GUC GUC_UNIT_MS rather than GUC_UNIT_S, but
expressing the default in postgresl.conf.sample as 10s rather than
10000ms. I tried values measured in milliseconds just for testing
purposes and didn't initially understand why it wasn't working. I
don't think there's any reason it can't work.

I would prefer to see log_startup_progress_interval declared and
defined in startup.c/startup.h rather than guc.c/guc.h.

There's no precedent in the tree for the use of ##__VA_ARGS__. On my
system it seems to work fine if I just leave out the ##. Any reason
not to do that?

Two of the declarations in startup.h forgot the leading "extern",
while the other two that are right next to them have it, matching
project style.

I'm reasonably happy with most of the identifier names now, but I
think init_startup_progress() is confusing. The reason I think that is
that we call it more than once, which is not really what people think
about when they think of an "init" function, I think. It's not
initializing the startup progress facility in general; it's preparing
for the next phase of startup progress reporting. How about renaming
it to begin_startup_progress_phase()? And then
startup_process_op_start_time could be
startup_process_phase_start_time to match.

SyncDataDirectory() potentially walks over the data directory three
times: once to call do_syncfs(), once to call pre_sync_fname(), and
once to call datadir_fsync_fname(). With this patch, the first and
third will emit progress messages if the operation runs long, but the
second will not. I think they should all be treated the same. Also,
the locations where you've inserted calls to init_startup_progress()
don't really make it clear with what code that's associated. I'd put
them *immediately* before the call to do_syncfs() or walkdir().

Remember that PostgreSQL comments are typically written "telegraph
style," so function comments should say "Does whatever" not "It does
whatever". Most of them are correct, but there's one sentence you need
to fix.

--
Robert Haas
EDB: http://www.enterprisedb.com