MIME-Version: 1.0
References: <CAMm1aWbuJLEnAjT4_1s8eArmZq=H7oi0Jv=J2GnRpBhkbC_PVw@mail.gmail.com>
 <CAMm1aWYWmqys2O7UekQ0=ZSWSpG9h2qoEW7w46yiWM0eO1Lu7g@mail.gmail.com>
 <CAOBaU_bb=7wN55WU91+ZbZdWChpuerS7fKqZVeiL4ZEWCa2zqQ@mail.gmail.com>
 <CAMm1aWb=0aWAMuvD-H60h2AhjCKPLfRqCG4zOURj2RPcyuLZmA@mail.gmail.com>
 <20220309141750.jirvp5o5v5zpr4lz@jrouhaud> <CAMm1aWZP8brtPB9MitarD_cB_HD_n2fkmH2rHijA-ivRWOfN=w@mail.gmail.com>
 <20220311100428.amj6hmqyaokgyxf2@jrouhaud> <CAMm1aWZX7+juGYfCc+1tbDrCmDPWDa+3-Yrw_bqAWy+=bihrng@mail.gmail.com>
 <20220311121300.2uh7k2zbrx2exdtq@jrouhaud> <CAMm1aWaiMantp=4aPOBbCx2bK8koHsRE3ZfzKDSy0V0nsbeFzw@mail.gmail.com>
 <20220314114555.pqj5pnuxz76vdscn@jrouhaud>
In-Reply-To: <20220314114555.pqj5pnuxz76vdscn@jrouhaud>
From: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Date: Fri, 18 Mar 2022 16:52:52 +0530
Message-ID: <CAMm1aWaa_-PduZPyV+O862DRZpA_HFSzore=vhjV_G1uChKnuA@mail.gmail.com>
Subject: Re: Report checkpoint progress with pg_stat_progress_checkpoint (was:
 Report checkpoint progress in server logs)
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 
	Ashutosh Sharma <ashu.coek88@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/CAMm1aWaa_-PduZPyV%2BO862DRZpA_HFSzore%3DvhjV_G1uChKnuA%40mail.gmail.com>
Precedence: bulk

> > > I don't get it.  The checkpoint flags and the view flags (set by
> > > pgstat_progrss_update*) are different, so why can't we add this flag to the
> > > view flags?  The fact that checkpointer.c doesn't update the passed flag and
> > > instead look in the shmem to see if CHECKPOINT_IMMEDIATE has been set since is
> > > an implementation detail, and the view shouldn't focus on which flags were
> > > initially passed to the checkpointer but instead which flags the checkpointer
> > > is actually enforcing, as that's what the user should be interested in.  If you
> > > want to store it in another field internally but display it in the view with
> > > the rest of the flags, I'm fine with it.
> >
> > Just to be in sync with the way code behaves, it is better not to
> > update the next checkpoint request's CHECKPOINT_IMMEDIATE with the
> > current checkpoint 'flags' field. Because the current checkpoint
> > starts with a different set of flags and when there is a new request
> > (with CHECKPOINT_IMMEDIATE), it just processes the pending operations
> > quickly to take up next requests. If we update this information in the
> > 'flags' field of the view, it says that the current checkpoint is
> > started with CHECKPOINT_IMMEDIATE which is not true.
>
> Which is why I suggested to only take into account CHECKPOINT_REQUESTED (to
> be able to display that a new checkpoint was requested)

I will take care in the next patch.

> > Hence I had
> > thought of adding a new field ('next flags' or 'upcoming flags') which
> > contain all the flag values of new checkpoint requests. This field
> > indicates whether the current checkpoint is throttled or not and also
> > it indicates there are new requests.
>
> I'm not opposed to having such a field, I'm opposed to having a view with "the
> current checkpoint is throttled but if there are some flags in the next
> checkpoint flags and those flags contain checkpoint immediate then the current
> checkpoint isn't actually throttled anymore" behavior.

I understand your point and I also agree that it becomes difficult for
the user to understand the context.

> and
> CHECKPOINT_IMMEDIATE, to be able to display that the current checkpoint isn't
> throttled anymore if it were.
>
> I still don't understand why you want so much to display "how the checkpoint
> was initially started" rather than "how the checkpoint is really behaving right
> now".  The whole point of having a progress view is to have something dynamic
> that reflects the current activity.

As of now I will not consider adding this information to the view. If
required and nobody opposes having this included in the 'flags' field
of the view, then I will consider adding.

Thanks & Regards,
Nitin Jadhav

On Mon, Mar 14, 2022 at 5:16 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Mon, Mar 14, 2022 at 03:16:50PM +0530, Nitin Jadhav wrote:
> > > > I am not suggesting
> > > > removing the existing 'flags' field of pg_stat_progress_checkpoint
> > > > view and adding a new field 'throttled'. The content of the 'flags'
> > > > field remains the same. I was suggesting replacing the 'next_flags'
> > > > field with 'throttled' field since the new request with
> > > > CHECKPOINT_IMMEDIATE flag enabled will affect the current checkpoint.
> > >
> > > Are you saying that this new throttled flag will only be set by the overloaded
> > > flags in ckpt_flags?
> >
> > Yes. you are right.
> >
> > > So you can have a checkpoint with a CHECKPOINT_IMMEDIATE
> > > flags that's throttled, and a checkpoint without the CHECKPOINT_IMMEDIATE flag
> > > that's not throttled?
> >
> > I think it's the reverse. A checkpoint with a CHECKPOINT_IMMEDIATE
> > flags that's not throttled (disables delays between writes) and  a
> > checkpoint without the CHECKPOINT_IMMEDIATE flag that's throttled
> > (enables delays between writes)
>
> Yes that's how it's supposed to work, but my point was that your suggested
> 'throttled' flag could say the opposite, which is bad.
>
> > > I don't get it.  The checkpoint flags and the view flags (set by
> > > pgstat_progrss_update*) are different, so why can't we add this flag to the
> > > view flags?  The fact that checkpointer.c doesn't update the passed flag and
> > > instead look in the shmem to see if CHECKPOINT_IMMEDIATE has been set since is
> > > an implementation detail, and the view shouldn't focus on which flags were
> > > initially passed to the checkpointer but instead which flags the checkpointer
> > > is actually enforcing, as that's what the user should be interested in.  If you
> > > want to store it in another field internally but display it in the view with
> > > the rest of the flags, I'm fine with it.
> >
> > Just to be in sync with the way code behaves, it is better not to
> > update the next checkpoint request's CHECKPOINT_IMMEDIATE with the
> > current checkpoint 'flags' field. Because the current checkpoint
> > starts with a different set of flags and when there is a new request
> > (with CHECKPOINT_IMMEDIATE), it just processes the pending operations
> > quickly to take up next requests. If we update this information in the
> > 'flags' field of the view, it says that the current checkpoint is
> > started with CHECKPOINT_IMMEDIATE which is not true.
>
> Which is why I suggested to only take into account CHECKPOINT_REQUESTED (to
> be able to display that a new checkpoint was requested) and
> CHECKPOINT_IMMEDIATE, to be able to display that the current checkpoint isn't
> throttled anymore if it were.
>
> I still don't understand why you want so much to display "how the checkpoint
> was initially started" rather than "how the checkpoint is really behaving right
> now".  The whole point of having a progress view is to have something dynamic
> that reflects the current activity.
>
> > Hence I had
> > thought of adding a new field ('next flags' or 'upcoming flags') which
> > contain all the flag values of new checkpoint requests. This field
> > indicates whether the current checkpoint is throttled or not and also
> > it indicates there are new requests.
>
> I'm not opposed to having such a field, I'm opposed to having a view with "the
> current checkpoint is throttled but if there are some flags in the next
> checkpoint flags and those flags contain checkpoint immediate then the current
> checkpoint isn't actually throttled anymore" behavior.