Re: when the startup process doesn't (logging startup delays) - Mailing list pgsql-hackers

> Apparently not, but here's a v2 anyway. In this version I made
> enable_timeout_every() a three-argument function, so that the caller
> can specify both the first time at which the timeout routine should be
> called and the interval between them, instead of only the latter. That
> seems to be more convenient for this use case, and is more powerful in
> general.

Thanks for sharing the patch. Overall approach looks good to me. But
just one concern about using enable_timeout_every() functionality. As
per my understanding the caller should calculate the first scheduled
timeout (now + interval) and pass it as the second argument but this
is not the same in 'v2-0002-Quick-testing-hack.patch'. Anyways I have
done the changes as I have mentioned (like now + interval). Kindly
correct me if I am wrong. I am attaching 2 patches here.
'v19-0001-Add-enable_timeout_every-to-fire-the-same-timeout.patch' is
the same as Robert's v2 patch. I have rebased my patch on top of this
and it is 'v19-0002-startup-progress.patch'.

> I just noticed something else which I realize is the indirect result
> of my own suggestion but which doesn't actually look all that nice.
> You've now got a call to RegisterTimeout(STARTUP_PROGRESS_TIMEOUT,
> ...) in InitPostgres, guarded by ! IsPostmasterEnvironment, and then
> another one in StartupProcessMain(). I think that's so that the
> feature works in single-user mode, but that means that technically,
> we're not reporting on the progress of the startup process. We're
> reporting progress on the startup operations that are normally
> performed by the startup process. But that means that the
> documentation isn't quite accurate (because it mentions the startup
> process specifically) and that the placement of the code in startup.c
> is suspect (because that's specifically for the startup process) and
> that basically every instance of the word "process" in the patch is
> technically a little bit wrong. I'm not sure if that's a big enough
> problem to be worth worrying about or exactly what we ought to do
> about it, but it doesn't seem fantastic. At a minimum, I think we
> should probably try to eliminate as many references to the startup
> process as we can, and talk about startup progress or startup
> operations or something like that.

Yeah right. I have modified the comments accordingly and also fixed
the other review comments related to the code comments.

I have modified the code to include a call to RegisterTimeout() in
only one place than the two calls previously. Initially I thought to
call this in begin_startup_progress_phase(). I feel this is not a
better choice since begin_startup_progress_phase function is getting
called in many places. So it calls RegisterTimeout() many times which
is not required. I feel StartupXLOG() is a better place for this since
StartupXLOG() gets called during the startup process, bootstrap
process or standalone backend. As per earlier discussion we need
support for this in the case of startup process and standalone
backend. Hence guarded this with '!IsBootstrapProcessingMode()'. Also
verified the behaviour in both of the cases. Please correct me if I am
wrong.

Thanks & Regards,
Nitin Jadhav

On Mon, Oct 18, 2021 at 9:15 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Sep 30, 2021 at 5:08 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > Any thoughts on the patch I attached?
>
> Apparently not, but here's a v2 anyway. In this version I made
> enable_timeout_every() a three-argument function, so that the caller
> can specify both the first time at which the timeout routine should be
> called and the interval between them, instead of only the latter. That
> seems to be more convenient for this use case, and is more powerful in
> general.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com