Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs) - Mailing list pgsql-hackers

On Wed, 23 Feb 2022 at 15:24, Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
>
> > At least for pg_stat_progress_checkpoint, storing only a timestamp in
> > the pg_stat storage (instead of repeatedly updating the field as a
> > duration) seems to provide much more precise measures of 'time
> > elapsed' for other sessions if one step of the checkpoint is taking a
> > long time.
>
> I am storing the checkpoint start timestamp in the st_progress_param[]
> and this gets set only once during the checkpoint (at the start of the
> checkpoint). I have added function
> pg_stat_get_progress_checkpoint_elapsed() which calculates the elapsed
> time and returns a string. This function gets called whenever
> pg_stat_progress_checkpoint view is queried. Kindly refer v2 patch and
> share your thoughts.

I dislike the lack of access to the actual value of the checkpoint
start / checkpoint elapsed field.

As a user, if I query the pg_stat_progress_* views, my terminal or
application can easily interpret an `interval` value and cast it to
string, but the opposite is not true: the current implementation for
pg_stat_get_progress_checkpoint_elapsed loses precision. This is why
we use typed numeric fields in effectively all other places instead of
stringified versions of the values: oid fields, counters, etc are all
rendered as bigint in the view, so that no information is lost and
interpretation is trivial.

> > I understand the want to integrate the log-based reporting in the same
> > API, but I don't think that is necessarily the right approach:
> > pg_stat_progress_* has low-overhead infrastructure specifically to
> > ensure that most tasks will not run much slower while reporting, never
> > waiting for locks. Logging, however, needs to take locks (if only to
> > prevent concurrent writes to the output file at a kernel level) and
> > thus has a not insignificant overhead and thus is not very useful for
> > precise and very frequent statistics updates.
>
> I understand that the log based reporting is very costly and very
> frequent updates are not advisable.  I am planning to use the existing
> infrastructure of 'log_startup_progress_interval' which provides an
> option for the user to configure the interval between each progress
> update. Hence it avoids frequent updates to server logs. This approach
> is used only during shutdown and end-of-recovery cases because we
> cannot access pg_stat_progress_checkpoint view during those scenarios.

I see; but log_startup_progress_interval seems to be exclusively
consumed through the ereport_startup_progress macro. Why put
startup/shutdown logging on the same path as the happy flow of normal
checkpoints?

> > So, although similar in nature, I don't think it is smart to use the
> > exact same infrastructure between pgstat_progress*-based reporting and
> > log-based progress reporting, especially if your logging-based
> > progress reporting is not intended to be a debugging-only
> > configuration option similar to log_min_messages=DEBUG[1..5].
>
> Yes. I agree that we cannot use the same infrastructure for both.
> Progress views and servers logs have different APIs to report the
> progress information. But since both of this are required for the same
> purpose, I am planning to use a common function which increases the
> code readability than calling it separately in all the scenarios. I am
> planning to include log based reporting in the next patch. Even after
> that if using the same function is not recommended, I am happy to
> change.

I don't think that checkpoint_progress_update_param(int, uint64) fits
well with the construction of progress log messages, requiring
special-casing / matching the offset numbers to actual fields inside
that single function, which adds unnecessary overhead when compared
against normal and direct calls to the related infrastructure.

I think that, instead of looking to what might at some point be added,
it is better to use the currently available functions instead, and
move to new functions if and when the log-based reporting requires it.


- Matthias