Re: Generalize ereport_startup_progress infrastructure

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

+1 for the idea to generalize the infrastructure.

Given two options, option-1 is to use a single GUC across all kind of
log running operations and option-2 is to use multiple GUCs (one for
each kind of long running operations), I go with option-1 because if a
user is interested to see a log message after every 10s for startup
operations (or any other long running operations) then it is likely
that he is interested to see other long running operations after every
10s only. It does not make sense to use different intervals for each
kind of long running operation here. It also increases the number of
GUCs which makes things complex. So it is a good idea to use a single
GUC here. But I am worried about the on/off switch as Robert
mentioned. How about using a new GUC to indicate features on/off. Say
"log_long_running_operations" which contains a comma separated string
which indicates the features to be enabled. For example,
"log_long_running_operations = startup, postmaster" will enable
logging for startup and postmaster operations and disables logging of
other long running operations. With this the number of GUCs will be
limited to 2 and it is simple and easy for the user.

Thanks & Regards,
Nitin Jadhav

On Thu, Aug 4, 2022 at 9:58 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?
>
> --
> Bharath Rupireddy
> RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
>
>