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

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

Fixed.

> 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.

Fixed.

> 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.

As suggested, I have changed it to GUC_UNIT_MS and kept the default
value to 10s. I would like to know the reason why it can't be
GUC_UNIT_S as we are expressing the values in terms of seconds.

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

Fixed.

> 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?

I had added this to support if no variable argument are passed to the
macro. For example, in the previous patches we used to log the
progress at the end of the operation like
"ereport_startup_progress(true, "data directory sync (syncfs) complete
after %ld.%02d s");". Since these calls are removed now, ## is not
required. Hence removed in the attached patch.

> 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 had not added extern since those function were not used in the other
files. Now added to match the 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.

Yes begin_startup_progress_phase() looks more appropriate. Instead of
startup_process_phase_start_time, startup_progress_phase_start_time
looks more appropriate. Changed these in the attached patch.

> 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().

Fixed.

> 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.

Fixed in the function comments of
startup_progress_timeout_has_expired(). Please let me now if this is
not the one you wanted me to correct.

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

Sorry. I missed this. I have added the documentation in the attached patch.
On Tue, Aug 10, 2021 at 8:56 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> 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