MIME-Version: 1.0
References: <CALj2ACUGWAC56yqhVOisNPXhewfRdeHG4NGTOK_sOStL=7yP+w@mail.gmail.com>
 <CAMm1aWY_wThAxmhC_7mkFsYHcNP2_JQLAKJEoY5EcHBi0Q42QQ@mail.gmail.com>
 <CAEze2WhfmEWDKcFx8jhcSFhbSoP7ettdYiikNSsVWCNpe5ME0g@mail.gmail.com>
 <CAMm1aWYsfgO91mXxy7kzrDcML8nuY3Ab4w9RnSjZ_mr6=Y-hwQ@mail.gmail.com>
 <20220217110515.zssjnez4lq2lmrdq@jrouhaud> <CAMm1aWZ1YY6n8T=0OkwQJJdua7fpkJdKr1fRqfTSrJb1ME9Q5g@mail.gmail.com>
 <20220217172724.uhs2v3mgio7y6ga5@jrouhaud> <CAMm1aWbjzP9g9ebkYqqJvNAW4jh6wP0maKW2Ozcv8PLVu76cFA@mail.gmail.com>
 <20220218074351.wgfosjt5lql2vjbr@jrouhaud> <CAMm1aWadyUhb89DEfgrs2j5i2u--O06x6oTp0EEhgGHtnw-SXA@mail.gmail.com>
 <20220219053215.r4wqatln5awklecf@jrouhaud> <CAMm1aWZzPVpLs7k5sQU4DObvh6YpOhjibcq2ykDrC_n25jeJNA@mail.gmail.com>
 <CAMm1aWZ07VYUPpvmV9Ck_VX-o+xH=hEBs-67scd_Us6U6FKbHA@mail.gmail.com> <CAEze2WhXEU+n=o0UjQSx8sJ3Bf7Ru3NN+t5Jzrbt9pFfDrLy0A@mail.gmail.com>
In-Reply-To: <CAEze2WhXEU+n=o0UjQSx8sJ3Bf7Ru3NN+t5Jzrbt9pFfDrLy0A@mail.gmail.com>
From: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Date: Fri, 25 Feb 2022 00:23:27 +0530
Message-ID: <CAMm1aWaL8S9AbeLQrSGNBW+d3165dmV=JnCfVyjo9UGKCz4vHg@mail.gmail.com>
Subject: Re: Report checkpoint progress with pg_stat_progress_checkpoint (was:
 Report checkpoint progress in server logs)
To: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: Julien Rouhaud <rjuju123@gmail.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Bruce Momjian <bruce@momjian.us>, 
	Tom Lane <tgl@sss.pgh.pa.us>, Magnus Hagander <magnus@hagander.net>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAMm1aWaL8S9AbeLQrSGNBW%2Bd3165dmV%3DJnCfVyjo9UGKCz4vHg%40mail.gmail.com>
Precedence: bulk

> I think the change to ImmediateCheckpointRequested() makes no sense.
> Before this patch, that function merely inquires whether there's an
> immediate checkpoint queued.  After this patch, it ... changes a
> progress-reporting flag?  I think it would make more sense to make the
> progress-report flag change in whatever is the place that *requests* an
> immediate checkpoint rather than here.
>
> > 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?

Thank you Alvaro and Matthias for your views. I understand your point
of not updating the progress-report flag here as it just checks
whether the CHECKPOINT_IMMEDIATE is set or not and takes an action
based on that but it doesn't change the checkpoint flags. I will
modify the code but I am a bit confused here. As per Alvaro, we need
to make the progress-report flag change in whatever is the place that
*requests* an immediate checkpoint. I feel this gives information
about the upcoming checkpoint not the current one. So updating here
provides wrong details in the view. The flags available during
CreateCheckPoint() will remain same for the entire checkpoint
operation and we should show the same information in the view till it
completes. So just removing the above piece of code (modified in
ImmediateCheckpointRequested()) in the patch will make it correct. My
opinion about maintaining a separate field to show upcoming checkpoint
flags is it makes the view complex. Please share your thoughts.

Thanks & Regards,

On Thu, Feb 24, 2022 at 10:45 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
>
> 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