Re: Generalize ereport_startup_progress infrastructure

On Wed, Aug 3, 2022 at 12:11 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Aug 2, 2022 at 3:25 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > ereport_startup_progress infrastructure added by commit 9ce346e [1]
> > will be super-useful for reporting progress of any long-running server
> > operations, not just the startup process operations. For instance,
> > postmaster can use it for reporting progress of temp file and temp
> > relation file removals [2], checkpointer can use it for reporting
> > progress of snapshot or mapping file processing or even WAL file
> > processing and so on. And I'm sure there can be many places in the
> > code where we have while or for loops which can, at times, take a long
> > time to finish and having a log message there would definitely help.
> >
> > Here's an attempt to generalize the ereport_startup_progress
> > infrastructure. The attached v1 patch places the code in elog.c/.h,
> > renames associated functions and variables, something like
> > ereport_startup_progress to ereport_progress,
> > log_startup_progress_interval to log_progress_report_interval and so
> > on.
>
> I'm not averse to reusing this infrastructure in other places, but I
> doubt we'd want all of those places to be controlled by a single GUC,
> especially because that GUC is also the on/off switch for the feature.

Thanks Robert! How about we tweak the function a bit -
begin_progress_report_phase(int timeout), so that each process can use
their own timeout interval? In this case, do we want to retain
log_startup_progress_interval as-is specific to the startup process?
If yes, other processes might come up with their own GUCs (if they
don't want to use hard-coded timeouts) similar to
log_startup_progress_interval, which isn't the right way IMO.

I think the notion of ereport_progress feature being disabled when the
timeout is 0, makes sense to me at least.

On the flip side, what if we just have a single GUC
log_progress_report_interval (as proposed in the v1 patch)? Do we ever
want different processes to emit progress report messages at different
frequencies? Well, I can think of the startup process during standby
recovery needing to emit recovery progress report messages at a much
lower frequency than the startup process during the crash recovery.
Again, controlling the frequencies with different GUCs isn't the way
forward. But we can do something like: process 1 emits messages with a
frequency of 2*log_progress_report_interval, process 2 with a
frequency 4*log_progress_report_interval and so on without needing
additional GUCs.

Thoughts?

--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/