Re: [PATCH] Expose checkpoint reason to completion log messages.

On Tue, Jan 13, 2026 at 3:29 PM Soumya S Murali
<soumyamurali.work@gmail.com> 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