Date: Fri, 25 Feb 2022 15:03:52 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@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>
Subject: Re: Report checkpoint progress with pg_stat_progress_checkpoint
 (was: Report checkpoint progress in server logs)
Message-ID: <20220225070352.fhfwnl5ksfsa4eew@jrouhaud>
References: <CAMm1aWZ1YY6n8T=0OkwQJJdua7fpkJdKr1fRqfTSrJb1ME9Q5g@mail.gmail.com>
 <20220217172724.uhs2v3mgio7y6ga5@jrouhaud>
 <CAMm1aWbjzP9g9ebkYqqJvNAW4jh6wP0maKW2Ozcv8PLVu76cFA@mail.gmail.com>
 <20220218074351.wgfosjt5lql2vjbr@jrouhaud>
 <CAMm1aWadyUhb89DEfgrs2j5i2u--O06x6oTp0EEhgGHtnw-SXA@mail.gmail.com>
 <20220219053215.r4wqatln5awklecf@jrouhaud>
 <CAMm1aWZzPVpLs7k5sQU4DObvh6YpOhjibcq2ykDrC_n25jeJNA@mail.gmail.com>
 <CAMm1aWZ07VYUPpvmV9Ck_VX-o+xH=hEBs-67scd_Us6U6FKbHA@mail.gmail.com>
 <CAEze2WhXEU+n=o0UjQSx8sJ3Bf7Ru3NN+t5Jzrbt9pFfDrLy0A@mail.gmail.com>
 <CAMm1aWaL8S9AbeLQrSGNBW+d3165dmV=JnCfVyjo9UGKCz4vHg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAMm1aWaL8S9AbeLQrSGNBW+d3165dmV=JnCfVyjo9UGKCz4vHg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20220225070352.fhfwnl5ksfsa4eew%40jrouhaud>
Precedence: bulk

Hi,

On Fri, Feb 25, 2022 at 12:23:27AM +0530, Nitin Jadhav wrote:
> > I think the change to ImmediateCheckpointRequested() makes no sense.
> > Before this patch, that function merely inquires whether there's an
> > immediate checkpoint queued.  After this patch, it ... changes a
> > progress-reporting flag?  I think it would make more sense to make the
> > progress-report flag change in whatever is the place that *requests* an
> > immediate checkpoint rather than here.
> >
> > > diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
> > > +ImmediateCheckpointRequested(int flags)
> > >      if (cps->ckpt_flags & CHECKPOINT_IMMEDIATE)
> > > +    {
> > > +        updated_flags |= CHECKPOINT_IMMEDIATE;
> >
> > I don't think that these changes are expected behaviour. Under in this
> > condition; the currently running checkpoint is still not 'immediate',
> > but it is going to hurry up for a new, actually immediate checkpoint.
> > Those are different kinds of checkpoint handling; and I don't think
> > you should modify the reported flags to show that we're going to do
> > stuff faster than usual. Maybe maintiain a seperate 'upcoming
> > checkpoint flags' field instead?
> 
> Thank you Alvaro and Matthias for your views. I understand your point
> of not updating the progress-report flag here as it just checks
> whether the CHECKPOINT_IMMEDIATE is set or not and takes an action
> based on that but it doesn't change the checkpoint flags. I will
> modify the code but I am a bit confused here. As per Alvaro, we need
> to make the progress-report flag change in whatever is the place that
> *requests* an immediate checkpoint. I feel this gives information
> about the upcoming checkpoint not the current one. So updating here
> provides wrong details in the view. The flags available during
> CreateCheckPoint() will remain same for the entire checkpoint
> operation and we should show the same information in the view till it
> completes.

I'm not sure what Matthias meant, but as far as I know there's no fundamental
difference between checkpoint with and without the CHECKPOINT_IMMEDIATE flag,
and there's also no scheduling for multiple checkpoints.

Yes, the flags will remain the same but checkpoint.c will test both the passed
flags and the shmem flags to see whether a delay should be added or not, which
is the only difference in checkpoint processing for this flag.  See the call to
ImmediateCheckpointRequested() which will look at the value in shmem:

	/*
	 * Perform the usual duties and take a nap, unless we're behind schedule,
	 * in which case we just try to catch up as quickly as possible.
	 */
	if (!(flags & CHECKPOINT_IMMEDIATE) &&
		!ShutdownRequestPending &&
		!ImmediateCheckpointRequested() &&
		IsCheckpointOnSchedule(progress))
[...]