Re: when the startup process doesn't - Mailing list pgsql-hackers

> Should it show the rusage ?  It's shown at startup completion since 10a5b35a0,
> so it seems strange not to show it here.

> I don't know, that seems like it's going to make the messages awfully
> long, and I'm not sure of what use it is to see that for every report.

I have not changed anything wrt this. If it is really required to
change, then I will change.

> +                       log_startup_process_progress("Syncing data directory", path, false);

> I think the fsync vs syncfs paths should be distinguished: "Syncing data
> directory (fsync)" vs "Syncing data directory (syncfs)".

Fixed.

> +               {"log_min_duration_startup_process", PGC_SUSET, LOGGING_WHEN,
>
> I think it should be PGC_SIGHUP, to allow changing it during runtime.
> Obviously it has no effect except during startup, but the change will be
> effective if the current process crashes.
> See also: https://www.postgresql.org/message-id/20210526001359.GE3676@telsasoft.com

I did not get exactly how it will change behaviour. In my
understanding, when the server restarts after a crash, it fetches the
value from the config file. So if there is any change that gets
affected. Kindly correct me if I am wrong.

> +extern void log_startup_process_progress(char *operation, void *data,
> +                                         bool is_complete);
>
> I think this should take an enum operation, rather than using strcmp() on it
> later.  The enum values might be RECOVERY_START, RECOVERY_END, rather than
> having a bool is_complete.

Fixed.

> I don't like the name very much. log_min_duration_startup_process
> seems to have been chosen to correspond to log_min_duration_statement,
> but the semantics are different. That one is a threshold, whereas this
> one is an interval. Maybe something like
> log_startup_progress_interval?

Yes. This looks more appropriate. Fixed in the attached patch.

> As far as the patch itself goes, I think that the overhead of this
> approach is going to be unacceptably high. I was imagining having a
> timer running in the background that fires periodically, with the
> interval handler just setting a flag. Then in the foreground we just
> need to check whether the flag is set. I doubt that we can get away
> with a GetCurrentTimestamp() after applying every WAL record ... that
> seems like it will be slow.

Thanks for correcting me. This approach is far better than what I had
used earlier. I have done the code changes as per your approach in the
attached patch.

Kindly let me know if any changes are required.

Thanks & Regards,
Nitin Jadhav

On Mon, Jun 7, 2021 at 7:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Robert Haas <robertmhaas@gmail.com> writes:
> > ... I doubt that we can get away
> > with a GetCurrentTimestamp() after applying every WAL record ... that
> > seems like it will be slow.
>
> Yeah, that's going to be pretty awful even on machines with fast
> gettimeofday, never mind ones where it isn't.
>
> It should be possible to use utils/misc/timeout.c to manage the
> interrupt, I'd think.
>
>                         regards, tom lane