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

Hi all,

On Mon, Jan 12, 2026 at 3:57 PM Michael Banck <mbanck(at)gmx(dot)net> wrote:
>
> Hi,
>
> On Mon, Jan 12, 2026 at 12:53:54PM +0530, Soumya S Murali wrote:
> > Based on the feedback received, I have reworked and implemented a
> > helper function so that both checkpoint start and completion messages
> > use the same wording and would remain in sync, extending the same
> > reason reporting to restart points for completeness and better
> > observability, ensuring consistent terminology with existing log
> > output, had removed the previous use of global state and instead
> > passed checkpoint flags explicitly and have fixed formatting and
> > whitespace issues noted during review.
> > Also, I have tested and validated the logic successfully. I have
> > attached the updated patch for further review.
> > Kindly review the patch and let me know the feedback.
> >
> > Regards,
> > Soumya
>
> > From 30e3c62695a643daf96ffca3b504a79d2a761f77 Mon Sep 17 00:00:00 2001
> > From: Soumya <soumyamurali(dot)work(at)gmail(dot)com>
> > Date: Mon, 12 Jan 2026 12:36:29 +0530
> > Subject: [PATCH] Expose checkpoint reason in completion log messages
> >
> > ---
> > src/backend/access/transam/xlog.c | 78 ++++++++++++++++-------
> > src/test/recovery/t/019_replslot_limit.pl | 2 +-
> > 2 files changed, 55 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> > index 22d0a2e8c3..63d1e56ca1 100644
> > --- a/src/backend/access/transam/xlog.c
> > +++ b/src/backend/access/transam/xlog.c
> > @@ -6714,6 +6714,49 @@ ShutdownXLOG(int code, Datum arg)
> > }
> > }
> >
> > +/*
> > +* Format checkpoint reason flags consistently for log messages.
> > +* The returned string is suitable for inclusion after
> > +* "checkpoint starting:" or inside "checkpoint complete (...)".
> > +*/
>
> Indentation of the comment is wrong here, the asterisks should be all on
> the same column.
> > +static const char *
> > +CheckpointReasonString(int flags)
> > +{
> > + static char buf[128];
> > + bool first = true;
>
> pgindent complains about the above line as well:
>
> | {
> | static char buf[128];
> |- bool first = true;
> |+ bool first = true;
> |
> | buf[0] = '\0';
>
> > + buf[0] = '\0';
> > +
> > +#define APPEND_REASON(str) \
> > + do { \
> > + if (!first) \
> > + strcat(buf, " "); \
> > + strcat(buf, (str)); \
> > + first = false; \
> > + } while (0)
>
> Those ending backslashes also don't seem to line up when 4-space tabs
> are set. But pgindent doesn't change them so maybe it is correct as-is?
>
> I am also not sure whether the above is current best-practise for
> Postgres code, I'd have to look deeper (or maybe somebody else can
> review this).
>
> > + if (flags & CHECKPOINT_IS_SHUTDOWN)
> > + APPEND_REASON("shutdown");
> > + if (flags & CHECKPOINT_END_OF_RECOVERY)
> > + APPEND_REASON("end-of-recovery");
> > + if (flags & CHECKPOINT_FAST)
> > + APPEND_REASON("fast");
> > + if (flags & CHECKPOINT_FORCE)
> > + APPEND_REASON("force");
> > + if (flags & CHECKPOINT_WAIT)
> > + APPEND_REASON("wait");
> > + if (flags & CHECKPOINT_CAUSE_XLOG)
> > + APPEND_REASON("wal");
> > + if (flags & CHECKPOINT_CAUSE_TIME)
> > + APPEND_REASON("time");
> > + if (flags & CHECKPOINT_FLUSH_UNLOGGED)
> > + APPEND_REASON("flush-unlogged");
> > +
> > +#undef APPEND_REASON
> > +
> > + return buf;
>
> I am still not sure whether this will be a regression for translation of
> those strings.
>
> Otherwise, the patch looks sane to me.

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.

Regards,
Soumya