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

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.