Re: [PATCH] Expose checkpoint reason to completion log messages. - Mailing list pgsql-hackers

Hi all,

On Tue, Jan 13, 2026 at 6:40 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> 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?
>


Thank you for the detailed review and suggestions.
Based on the feedback, I have updated the patch to make the checkpoint
and restart point log formatting fully consistent between start and
completion log messages. The completion messages now use the same
format as the start messages without parentheses. I have also replaced
the term "reason" with the term "flags". The patch has been rebuilt
and validated with make check and the full recovery TAP test suite,
and I have manually verified the resulting log output to confirm the
expected formatting. I am attaching the updated patch for further
review. Please let me know if any additional adjustments are needed.


Regards,
Soumya