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 14:28, Nitin Jadhav
<nitinjadhavpostgres@gmail.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