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

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

> > > CHECKPOINT_REQUESTED will always be set by RequestCheckpoint(), and can be used
> > > to detect that someone wants a new checkpoint afterwards, whatever it's and
> > > whether or not the current checkpoint to be finished quickly. For this flag I
> > > think it's better to not report it in the view flags but with a new field, as
> > > discussed before, as it's really what it means.
> >
> > I understand your suggestion of adding a new field to indicate whether
> > any of the new requests have been made or not. You just want this
> > field to represent only a new request or does it also represent the
> > current checkpoint to finish quickly.
>
> Only represent what it means: a new checkpoint is requested. An additional
> CHECKPOINT_IMMEDIATE flag is orthogonal to this flag and this information.

Thanks for the confirmation.

> > > CHECKPOINT_IMMEDIATE is the only new flag that can be used in an already in
> > > progress checkpoint, so it can be simply added to the view flags.
> >
> > As discussed upthread this is not advisable to do so. The content of
> > 'flags' remains the same through the checkpoint. We cannot add a new
> > checkpoint's flag (CHECKPOINT_IMMEDIATE ) to the current one even
> > though it affects current checkpoint behaviour. Only thing we can do
> > is to add a new field to show that the current checkpoint is affected
> > with new requests.
>
> 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. 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. Please share your thoughts. More
thoughts are welcomed.

Thanks & Regards,
Nitin Jadhav

On Fri, Mar 11, 2022 at 5:43 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
>
> On Fri, Mar 11, 2022 at 04:59:11PM +0530, Nitin Jadhav wrote:
> > > That "throttled" flag should be the same as having or not a "force" in the
> > > flags. We should be consistent and report information the same way, so either
> > > a lot of flags (is_throttled, is_force...) or as now a single field containing
> > > the set flags, so the current approach seems better. Also, it wouldn't be much
> > > better to show the checkpoint as not having the force flags and still not being
> > > throttled.
> >
> > I think your understanding is wrong here. The flag which affects
> > throttling behaviour is CHECKPOINT_IMMEDIATE.
>
> Yes sorry, that's what I meant and later used in the flags.
>
> > 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? 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?
>
> > > CHECKPOINT_REQUESTED will always be set by RequestCheckpoint(), and can be used
> > > to detect that someone wants a new checkpoint afterwards, whatever it's and
> > > whether or not the current checkpoint to be finished quickly. For this flag I
> > > think it's better to not report it in the view flags but with a new field, as
> > > discussed before, as it's really what it means.
> >
> > I understand your suggestion of adding a new field to indicate whether
> > any of the new requests have been made or not. You just want this
> > field to represent only a new request or does it also represent the
> > current checkpoint to finish quickly.
>
> Only represent what it means: a new checkpoint is requested. An additional
> CHECKPOINT_IMMEDIATE flag is orthogonal to this flag and this information.
>
> > > CHECKPOINT_IMMEDIATE is the only new flag that can be used in an already in
> > > progress checkpoint, so it can be simply added to the view flags.
> >
> > As discussed upthread this is not advisable to do so. The content of
> > 'flags' remains the same through the checkpoint. We cannot add a new
> > checkpoint's flag (CHECKPOINT_IMMEDIATE ) to the current one even
> > though it affects current checkpoint behaviour. Only thing we can do
> > is to add a new field to show that the current checkpoint is affected
> > with new requests.
>
> 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.
>
> > > Why not just reporting (ckpt_flags & (CHECKPOINT_REQUESTED |
> > > CHECKPOINT_IMMEDIATE)) in the path(s) that can update the new flags for the
> > > view?
> >
> > Where do you want to add this in the path?
>
> Same as in your current patch I guess.