Re: Generalize ereport_startup_progress infrastructure

On Thu, Aug 4, 2022 at 9:57 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> 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?

Here's v2 patch, passing progress report interval as an input to
begin_progress_report_phase() so that the processes can use their own
intervals(hard-coded or GUC) if they wish to not use the generic GUC
log_progress_report_interval.

Thoughts?

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