Hi all,
> 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.
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 ?
+ ControlFile->checkPointKind = 0;
It is worth a comment that 0 is unknown, as for instance in [2]
+ (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" ?
+ snprintf(ckpt_kind, 2 * MAXPGPATH, "%s%s%s%s%s%s%s%s%s",
+ (flags == 0) ? "unknown" : "",
+ (flags & CHECKPOINT_IS_SHUTDOWN) ? "shutdown " : "",
+ (flags & CHECKPOINT_END_OF_RECOVERY) ? "end-of-recovery " : "",
+ (flags & CHECKPOINT_IMMEDIATE) ? "immediate " : "",
+ (flags & CHECKPOINT_FORCE) ? "force " : "",
+ (flags & CHECKPOINT_WAIT) ? "wait " : "",
+ (flags & CHECKPOINT_CAUSE_XLOG) ? "wal " : "",
+ (flags & CHECKPOINT_CAUSE_TIME) ? "time " : "",
+ (flags & CHECKPOINT_FLUSH_ALL) ? "flush-all" : "");
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.
[1]
https://github.com/postgres/postgres/blob/410aa248e5a883fde4832999cc9b23c7ace0f2ff/src/include/pg_config_manual.h#L106
[2]
https://github.com/postgres/postgres/blob/410aa248e5a883fde4832999cc9b23c7ace0f2ff/src/interfaces/libpq/fe-exec.c#L1078
[3]
https://github.com/postgres/postgres/blob/27b77ecf9f4d5be211900eda54d8155ada50d696/src/backend/access/transam/xlog.c#L8851-L8859
Regards,
Sergey Dudoladov
