public inbox for [email protected]
help / color / mirror / Atom feedFrom: Julien Rouhaud <[email protected]>
To: Nitin Jadhav <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Bruce Momjian <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Magnus Hagander <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)
Date: Fri, 25 Feb 2022 15:03:52 +0800
Message-ID: <20220225070352.fhfwnl5ksfsa4eew@jrouhaud> (raw)
In-Reply-To: <CAMm1aWaL8S9AbeLQrSGNBW+d3165dmV=JnCfVyjo9UGKCz4vHg@mail.gmail.com>
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>
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))
[...]
view thread (74+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)
In-Reply-To: <20220225070352.fhfwnl5ksfsa4eew@jrouhaud>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox