On Tue, Jan 13, 2026 at 3:29 PM Soumya S Murali
<[email protected]> wrote:
> Thank you for the detailed review and suggestions.
> I have reworked the patch on a fresh tree, addressed the formatting
> and indentation issues, and aligned the helper implementation with
> PostgreSQL coding conventions. The updated patch has been pgindented
> and validated with make check and the full recovery TAP test suite. I
> am attaching the revised patch for further review.
Thanks for updating the patch!
With this patch, the checkpoint log messages look like this:
LOG: checkpoint starting: fast force wait
LOG: checkpoint complete (fast force wait): ...
The formatting of the checkpoint flags differs between the starting and
complete messages: the complete message wraps them in parentheses,
while the starting message does not. I think it would be better to log
the flags in a consistent way in both messages. For example:
LOG: checkpoint starting: fast force wait
LOG: checkpoint complete: fast force wait: ...
- if ($node_primary->log_contains("checkpoint complete: ", $logstart))
+ if ($node_primary->log_contains(qr/checkpoint complete(?:
\([^)]*\))?:/, $logstart))
If we adopt a format like the above example, this test change would
no longer be needed.
+#define APPEND_REASON(str) \
+ do \
+ { \
+ if (!first) \
Wouldn't it be simpler to just build the string with snprintf() based on
the flags? For example:
---------------------------------------------------
static char buf[128];
snprintf(buf, sizeof(buf), "%s%s%s%s%s%s%s%s",
(flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "",
(flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "",
(flags & CHECKPOINT_FAST) ? " fast" : "",
(flags & CHECKPOINT_FORCE) ? " force" : "",
(flags & CHECKPOINT_WAIT) ? " wait" : "",
(flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "",
(flags & CHECKPOINT_CAUSE_TIME) ? " time" : "",
(flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : "");
return buf;
---------------------------------------------------
+ * Format checkpoint reason flags consistently for log messages.
+ * The returned string is suitable for inclusion after
+ * "checkpoint starting:" or inside "checkpoint complete (...)".
+ */
+static const char *
+CheckpointReasonString(int flags)
These flags include more than just the reason a checkpoint was triggered,
so using "reason" here seems misleading. Wouldn't just "flags" be a better term?
Regards,
--
Fujii Masao
