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.
> - proallargtypes => '{pg_lsn,pg_lsn,text,int4,int4,bool,text,oid,xid,xid,xid,oid,xid,xid,oid,xid,xid,timestamptz}',
> + proallargtypes =>
'{pg_lsn,pg_lsn,text,int4,int4,bool,text,oid,xid,xid,xid,oid,xid,xid,oid,xid,xid,timestamptz,text}',
>
> I think the additional column should be text[] instead of text, but
> not sure.
We are preparing a single string of all the checkpoint kinds and
outputting as a text column, so we don't need text[].
Regards,
Bharath Rupireddy.
