Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

On Wed, 23 Feb 2022 at 14:28, Nitin Jadhav
<nitinjadhavpostgres(at)gmail(dot)com> wrote:
>
> Sharing the v2 patch. Kindly have a look and share your comments.

Thanks for updating.

> diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml

With the new pg_stat_progress_checkpoint, you should also add a
backreference to this progress reporting in the CHECKPOINT sql command
documentation located in checkpoint.sgml, and maybe in wal.sgml and/or
backup.sgml too. See e.g. cluster.sgml around line 195 for an example.

> diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
> +ImmediateCheckpointRequested(int flags)
> if (cps->ckpt_flags & CHECKPOINT_IMMEDIATE)
> + {
> + updated_flags |= CHECKPOINT_IMMEDIATE;

I don't think that these changes are expected behaviour. Under in this
condition; the currently running checkpoint is still not 'immediate',
but it is going to hurry up for a new, actually immediate checkpoint.
Those are different kinds of checkpoint handling; and I don't think
you should modify the reported flags to show that we're going to do
stuff faster than usual. Maybe maintiain a seperate 'upcoming
checkpoint flags' field instead?

> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
> + ( SELECT '0/0'::pg_lsn +
> + ((CASE
> + WHEN stat.lsn_int64 < 0 THEN pow(2::numeric, 64::numeric)::numeric
> + ELSE 0::numeric
> + END) +
> + stat.lsn_int64::numeric)
> + FROM (SELECT s.param3::bigint) AS stat(lsn_int64)
> + ) AS start_lsn,

My LSN select statement was an example that could be run directly in
psql; the so you didn't have to embed the SELECT into the view query.
The following should be sufficient (and save the planner a few cycles
otherwise spent in inlining):

+ ('0/0'::pg_lsn +
+ ((CASE
+ WHEN s.param3 < 0 THEN pow(2::numeric,
64::numeric)::numeric
+ ELSE 0::numeric
+ END) +
+ s.param3::numeric)
+ ) AS start_lsn,

> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> +checkpoint_progress_start(int flags)
> [...]
> +checkpoint_progress_update_param(int index, int64 val)
> [...]
> +checkpoint_progress_end(void)
> +{
> + /* In bootstrap mode, we don't actually record anything. */
> + if (IsBootstrapProcessingMode())
> + return;

Disabling pgstat progress reporting when in bootstrap processing mode
/ startup/end-of-recovery makes very little sense (see upthread) and
should be removed, regardless of whether seperate functions stay.

> diff --git a/src/include/commands/progress.h b/src/include/commands/progress.h
> +#define PROGRESS_CHECKPOINT_PHASE_INIT 0

Generally, enum-like values in a stat_progress field are 1-indexed, to
differentiate between empty/uninitialized (0) and states that have
been set by the progress reporting infrastructure.

Kind regards,

Matthias van de Meent