Re: Is there a way (except from server logs) to know the kind of on-going/last checkpoint? - Mailing list pgsql-hackers

Hi,

On Fri, Jan 28, 2022 at 01:49:19PM +0530, Bharath Rupireddy wrote:
> On Thu, Jan 27, 2022 at 3:23 PM Sergey Dudoladov
> <sergey.dudoladov@gmail.com> wrote:
> > > Here's v2, rebased onto the latest master.
> >
> > I've reviewed this patch. The patch builds against the master (commit
> > e9d4001ec592bcc9a3332547cb1b0211e8794f38) and passes all the tests.
> > The patch does what it intends to do, namely store the kind of the
> > last checkpoint in the control file and display it in the output of
> > the pg_control_checkpoint() function and pg_controldata utility.
> > I did not test it with restartpoints though. Speaking of the torn
> > writes, the size of the control file with this patch applied does not
> > exceed 8Kb.
> 
> Thanks for the review.
> 
> > A few code comments:
> >
> > + char ckpt_kind[2 * MAXPGPATH];
> >
> > I don't completely understand why 2 * MAXPGPATH is used here for the
> > buffer size.
> > [1] defines MAXPGPATH to be standard size of a pathname buffer. How
> > does it relate to ckpt_kind ?
> 
> I was using it loosely. Changed in the v3 patch.
> 
> > + ControlFile->checkPointKind = 0;
> >
> > It is worth a comment that 0 is unknown, as for instance in [2]
> 
> We don't even need ControlFile->checkPointKind = 0; because
> InitControlFile will memset(ControlFile, 0, sizeof(ControlFileData));,
> hence removed this.
> 
> > + (flags == 0) ? "unknown" : "",
> >
> > That reads as if this patch would introduce a new "unknown" checkpoint state.
> > Why have it here at all if after for example initdb the kind is "shutdown" ?
> 
> Yeah, even LogCheckpointStart doesn't have anything "unknown" so removed it.
> 
> > The space at the strings' end (as in "wait " or "immediate ")
> > introduces extra whitespace in the output of pg_control_checkpoint().
> > A similar check at [3] places whitespace differently; that arrangement
> > of whitespace should remove the issue.
> 
> Changed.
> 
> > >       Datum           values[18];
> > >       bool            nulls[18];
> >
> > You forgot to expand these arrays.
> 
> Not sure what you meant here. The size of the array is already 19 in v2.
> 
> > This breaks checkpoint file format. Need to bump PG_CONTROL_VERSION,
> > and pg_upgrade need to treat the change.
> 
> I added a note in the commit message to bump cat version so that the
> committer will take care of it.

PG_CONTROL_VERSION is different from catversion.  You should update it in this
patch.

But Horiguchi-san was also mentioning that pg_upgrade/controldata.c needs some
modifications if you change the format (thus the requirement to bump
PG_CONTROL_VERSION).

Why are you defining CHECKPOINT_KIND_TEXT_LENGTH twice?  You
should just define it in some sensible header used by both files, or better
have a new function to take care of that rather than having the code
duplicated.

Also, you still didn't fix the possible flag upgrade issue.