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

Hi,

Here is the update patch which fixes the previous comments discussed
in this thread. I am sorry for the long gap in the discussion. Kindly
let me know if I have missed any of the comments or anything new.

Thanks & Regards,
Nitin Jadhav

On Fri, Mar 18, 2022 at 4:52 PM Nitin Jadhav
<nitinjadhavpostgres(at)gmail(dot)com> wrote:
>
> > > > 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(at)gmail(dot)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.