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

> > > 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.