Обсуждение: [PATCH] Expose checkpoint reason to completion log messages.

Hi,

On 2025-12-01 16:48:56 +0530, Soumya S Murali wrote:
> This patch is an update after reworking the “checkpoint reason” changes as
> a standalone patch, separate from the pg_stat_checkpointer additions as
> suggested [1].

This seems to not pass the tests on any platform:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6306

Looks like you need to update 019_replslot_limit.pl for the changed log
format.

I suggest running the tests before submitting.

Greetings,

Andres

Hi Andres,


On Thu, Dec 18, 2025 at 5:49 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2025-12-01 16:48:56 +0530, Soumya S Murali wrote:
> > This patch is an update after reworking the “checkpoint reason” changes as
> > a standalone patch, separate from the pg_stat_checkpointer additions as
> > suggested [1].
>
> This seems to not pass the tests on any platform:
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6306
>
> Looks like you need to update 019_replslot_limit.pl for the changed log
> format.
>
> I suggest running the tests before submitting.
>
> Greetings,
>
> Andres


Thank you for checking and for pointing this out. I will fix the
expected log pattern, rerun the test, and will resend an updated patch
once all tests get passed.

Regards,
Soumya

Hi all,

On Thu, Dec 18, 2025 at 5:49 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2025-12-01 16:48:56 +0530, Soumya S Murali wrote:
> > This patch is an update after reworking the “checkpoint reason” changes as
> > a standalone patch, separate from the pg_stat_checkpointer additions as
> > suggested [1].
>
> This seems to not pass the tests on any platform:
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6306
>
> Looks like you need to update 019_replslot_limit.pl for the changed log
> format.
>
> I suggest running the tests before submitting.
>
> Greetings,
>
> Andres


I have updated my patch and have tested successfully. Also had rerun
the entire recovery TAP suite and every test got passed successfully.
I am attaching the updated patch herewith.


Regards,
Soumya

On Thu, Jan 1, 2026 at 5:09 PM Soumya S Murali
<soumyamurali.work@gmail.com> wrote:
>
> Hi all,
>
> On Thu, Dec 18, 2025 at 5:49 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2025-12-01 16:48:56 +0530, Soumya S Murali wrote:
> > > This patch is an update after reworking the “checkpoint reason” changes as
> > > a standalone patch, separate from the pg_stat_checkpointer additions as
> > > suggested [1].

This kind of information already seems to be included in the
checkpoint starting log message, for example:

    LOG:  checkpoint starting: fast force wait

Why do you want to include essentially the same information in the
checkpoint ending log message as well?

Regards,

--
Fujii Masao

Hi,

On Tue, Jan 06, 2026 at 02:41:16PM +0900, Fujii Masao wrote:
> On Thu, Jan 1, 2026 at 5:09 PM Soumya S Murali
> <soumyamurali.work@gmail.com> wrote:
> > On Thu, Dec 18, 2025 at 5:49 AM Andres Freund <andres@anarazel.de> wrote:
> > > On 2025-12-01 16:48:56 +0530, Soumya S Murali wrote:
> > > > This patch is an update after reworking the “checkpoint reason” changes as
> > > > a standalone patch, separate from the pg_stat_checkpointer additions as
> > > > suggested [1].
>
> This kind of information already seems to be included in the
> checkpoint starting log message, for example:
>
>     LOG:  checkpoint starting: fast force wait
>
> Why do you want to include essentially the same information in the
> checkpoint ending log message as well?

I do think it is useful; the checkpoint finished message has a lot more
information than the checkpoint starting meessage (which basically only
mentions the reason) and when you extract log messages you currently
need to backtrack from the checkpoint finished message to the
corresponding checkpoint starting message to find out the type. Seeing
how the checkpoint finished message is so much more verbose anyway, I
think adding the reason to it is reasonably and helpful.


Michael

On Tue, Jan 6, 2026 at 8:09 PM VASUKI M <vasukianand0119@gmail.com> wrote:
>
> Yeah,the proposed idea makes sense to me,as users will almost see at the end of log messages it will be easy to find
outwhat is the checkpoint reason. 
> +1 for it.

If we do this, it would be better to keep the wording consistent between
the checkpoint starting and completion messages. For example,
the patch represents CHECKPOINT_FAST as "immediate",
while the checkpoint starting message uses "fast"; those should match.

I'm also wondering why this information is currently included only in
the checkpoint starting message. I tried to find any past discussion on this,
but couldn’t find one.

Regards,

--
Fujii Masao

Hi all,

Thank you for the review and feedback for raising these points.
While the checkpoint starting message already includes the reason, in
practice the checkpoint completion message is what users most often
look at for performance analysis since it contains the detailed
timing, buffer, and WAL statistics to refer to. When extracting or
aggregating logs, it is currently necessary to correlate the
completion message back to the corresponding starting message to
determine the checkpoint type, which is not always convenient or
reliable in busy systems. Including the reason directly in the
completion message avoids that extra step.
I agree with your point that the checkpoint completion message should
use the same terminology as the starting message ( “fast” rather than
“immediate”) and I have updated the patch accordingly and have
attached herewith for review. This patch aligns the checkpoint
completion log wording with the existing checkpoint starting message
(using “fast” instead of “immediate”) for consistency.
And regarding why this information has historically only been present
in the starting message, I think the completion log originally focused
purely on reporting I/O and timing statistics, rather than contextual
metadata. With the increasing use of log analysis for performance
troubleshooting, it is reasonable to include the checkpoint reason
with those statistics for the detailed analysis.
Kindly review my patch and let me know the feedback.

Regards,
Soumya

Hi,

On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote:
> I agree with your point that the checkpoint completion message should
> use the same terminology as the starting message ( “fast” rather than
> “immediate”) and I have updated the patch accordingly and have
> attached herewith for review. This patch aligns the checkpoint
> completion log wording with the existing checkpoint starting message
> (using “fast” instead of “immediate”) for consistency.

First off, please send complete and not incremental patches.

From what I can tell, you currently also do not have "wal" as reason?
Also, you have "timed" vs "time" etc.

I think it would make most sense to factor out the logic for the
starting checkpoint reason from xlog.c ("(errmsg("checkpoint
starting:%s%s%s%s%s%s%s%s")") and use that both for starting and
stopping.


Michael

Hi all,

On Wed, Jan 7, 2026 at 1:37 PM Michael Banck <mbanck@gmx.net> wrote:
>
> Hi,
>
> On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote:
> > I agree with your point that the checkpoint completion message should
> > use the same terminology as the starting message ( “fast” rather than
> > “immediate”) and I have updated the patch accordingly and have
> > attached herewith for review. This patch aligns the checkpoint
> > completion log wording with the existing checkpoint starting message
> > (using “fast” instead of “immediate”) for consistency.
>
> First off, please send complete and not incremental patches.
>
> From what I can tell, you currently also do not have "wal" as reason?
> Also, you have "timed" vs "time" etc.
>
> I think it would make most sense to factor out the logic for the
> starting checkpoint reason from xlog.c ("(errmsg("checkpoint
> starting:%s%s%s%s%s%s%s%s")") and use that both for starting and
> stopping.
>
>
> Michael

Thank you for the review and feedback.
I have reworked the patch to factor out the checkpoint reason logic to
use the same wording for both checkpoint start and completion log
(including wal, time, fast, force, and wait). The format has been
aligned with existing conventions and has been verified by running
make check and recovery TAP tests including t/019_replslot_limit.pl
successfully.
Kindly review my patch and let me know the feedback.

Regards,
Soumya

> 
> Hi all,
> 
> On Wed, Jan 7, 2026 at 1:37 PM Michael Banck <mbanck@gmx.net> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote:
> > > I agree with your point that the checkpoint completion message
> > > should use the same terminology as the starting message ( “fast”
> > > rather than
> > > “immediate”) and I have updated the patch accordingly and have
> > > attached herewith for review. This patch aligns the checkpoint
> > > completion log wording with the existing checkpoint starting message
> > > (using “fast” instead of “immediate”) for consistency.
> >
> > First off, please send complete and not incremental patches.
> >
> > From what I can tell, you currently also do not have "wal" as reason?
> > Also, you have "timed" vs "time" etc.
> >
> > I think it would make most sense to factor out the logic for the
> > starting checkpoint reason from xlog.c ("(errmsg("checkpoint
> > starting:%s%s%s%s%s%s%s%s")") and use that both for starting and
> > stopping.
> >
> >
> > Michael
> 
> Thank you for the review and feedback.
> I have reworked the patch to factor out the checkpoint reason logic to use the
> same wording for both checkpoint start and completion log (including wal, time,
> fast, force, and wait). The format has been aligned with existing conventions
> and has been verified by running make check and recovery TAP tests including
> t/019_replslot_limit.pl successfully.
> Kindly review my patch and let me know the feedback.
> 
> Regards,
> Soumya

Hi Soumya,

Thanks for the updated patch. I agree that having the checkpoint reason in the completion log is very helpful for
performanceanalysis, as it avoids the need to manually correlate "starting" and "complete" messages in busy logs.
 
I’ve reviewed the latest version and have a few suggestions to make the refactoring more complete:
1. Consistency in LogCheckpointStart
Currently, the checkpoint branch uses the new CheckpointReasonString helper, but the restartpoint branch still uses the
oldinline triple-operator logic. It would be better to use the helper function in both places to ensure they stay in
sync.

2. Missing Reason for Restartpoints 
In LogCheckpointEnd, the checkpoint reason is added to the checkpoint complete message, but the restartpoint complete
messageremains unchanged. I think users would find the reason for restartpoints just as useful for debugging.
 

3. Global Variable vs. Passing Flags: Instead of using a file-level global checkpoint_reason_str to pass data between
thestart and end functions, would it make sense to simply pass the flags as a new argument to LogCheckpointEnd(bool
restartpoint,int flags)? 
 
This would allow you to call the helper function directly inside the ereport calls and avoid maintaining global state.

4. Whitespace Errors: I noticed a few whitespace warnings (space before tabs) when applying the patch. 
/home/carol/0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch:69: space before tab in indent.
            CheckpointReasonString(flags),
/home/carol/0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch:70: space before tab in indent.
            sizeof(checkpoint_reason_str));
/home/carol/0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch:88: space before tab in indent.
                (errmsg("checkpoint starting:%s", checkpoint_reason_str)));
warning: 3 lines add whitespace errors.

The patch can be compiled successfully and can pass make check -C src/test/recovery/

Regards,
Yuan Li(carol)

Hi,

On Thu, Jan 08, 2026 at 11:58:43AM +0530, Soumya S Murali wrote:
> On Wed, Jan 7, 2026 at 1:37 PM Michael Banck <mbanck@gmx.net> wrote:
> > On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote:
> I have reworked the patch to factor out the checkpoint reason logic to
> use the same wording for both checkpoint start and completion log
> (including wal, time, fast, force, and wait). The format has been
> aligned with existing conventions and has been verified by running
> make check and recovery TAP tests including t/019_replslot_limit.pl
> successfully.
> Kindly review my patch and let me know the feedback.

Thanks for the new patch.

> index 22d0a2e8c3..9f7439f9a2 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
[...]
> +static const char *
> +CheckpointReasonString(int flags)
> +{
> +    static char buf[128];
> +
> +    buf[0] = '\0';
> +
> +    if (flags & CHECKPOINT_IS_SHUTDOWN)
> +        strcat(buf, " shutdown");
> +    if (flags & CHECKPOINT_END_OF_RECOVERY)
> +        strcat(buf, " end-of-recovery");
> +    if (flags & CHECKPOINT_FAST)
> +        strcat(buf, " fast");
> +    if (flags & CHECKPOINT_FORCE)
> +        strcat(buf, " force");
> +    if (flags & CHECKPOINT_WAIT)
> +        strcat(buf, " wait");
> +    if (flags & CHECKPOINT_CAUSE_XLOG)
> +        strcat(buf, " wal");
> +    if (flags & CHECKPOINT_CAUSE_TIME)
> +        strcat(buf, " time");
> +    if (flags & CHECKPOINT_FLUSH_UNLOGGED)
> +        strcat(buf, " flush-unlogged");
> +
> +    return buf;
> +}

The way this is coded now prepends the final string with a space
character. This is fine for the checkpoint starting message, but makes
the checkpoint finished message look slightly weird (not the "( wal)"):

|2026-01-08 18:30:33.691 CET [1666416] LOG:  checkpoint complete ( wal):
|wrote 20 buffers (0.1%), wrote 0 SLRU buffers; 0 WAL file(s) added, 4
|removed, 33 recycled; write=2.221 s, sync=0.019 s, total=2.701 s; sync
|files=3, longest=0.018 s, average=0.007 s; distance=601885 kB,
|estimate=601885 kB; lsn=0/D2D59FC8, redo lsn=0/B492B500

Also I think the translators might now complain, but I am not sure how
to best approach this.


Michael

Hi all,

On Thu, Jan 8, 2026 at 11:06 PM Michael Banck <mbanck@gmx.net> wrote:
>
> Hi,
>
> On Thu, Jan 08, 2026 at 11:58:43AM +0530, Soumya S Murali wrote:
> > On Wed, Jan 7, 2026 at 1:37 PM Michael Banck <mbanck@gmx.net> wrote:
> > > On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote:
> > I have reworked the patch to factor out the checkpoint reason logic to
> > use the same wording for both checkpoint start and completion log
> > (including wal, time, fast, force, and wait). The format has been
> > aligned with existing conventions and has been verified by running
> > make check and recovery TAP tests including t/019_replslot_limit.pl
> > successfully.
> > Kindly review my patch and let me know the feedback.
>
> Thanks for the new patch.
>
> > index 22d0a2e8c3..9f7439f9a2 100644
> > --- a/src/backend/access/transam/xlog.c
> > +++ b/src/backend/access/transam/xlog.c
> [...]
> > +static const char *
> > +CheckpointReasonString(int flags)
> > +{
> > +     static char buf[128];
> > +
> > +     buf[0] = '\0';
> > +
> > +     if (flags & CHECKPOINT_IS_SHUTDOWN)
> > +             strcat(buf, " shutdown");
> > +     if (flags & CHECKPOINT_END_OF_RECOVERY)
> > +             strcat(buf, " end-of-recovery");
> > +     if (flags & CHECKPOINT_FAST)
> > +             strcat(buf, " fast");
> > +     if (flags & CHECKPOINT_FORCE)
> > +             strcat(buf, " force");
> > +     if (flags & CHECKPOINT_WAIT)
> > +             strcat(buf, " wait");
> > +     if (flags & CHECKPOINT_CAUSE_XLOG)
> > +             strcat(buf, " wal");
> > +     if (flags & CHECKPOINT_CAUSE_TIME)
> > +             strcat(buf, " time");
> > +     if (flags & CHECKPOINT_FLUSH_UNLOGGED)
> > +             strcat(buf, " flush-unlogged");
> > +
> > +     return buf;
> > +}
>
> The way this is coded now prepends the final string with a space
> character. This is fine for the checkpoint starting message, but makes
> the checkpoint finished message look slightly weird (not the "( wal)"):
>
> |2026-01-08 18:30:33.691 CET [1666416] LOG:  checkpoint complete ( wal):
> |wrote 20 buffers (0.1%), wrote 0 SLRU buffers; 0 WAL file(s) added, 4
> |removed, 33 recycled; write=2.221 s, sync=0.019 s, total=2.701 s; sync
> |files=3, longest=0.018 s, average=0.007 s; distance=601885 kB,
> |estimate=601885 kB; lsn=0/D2D59FC8, redo lsn=0/B492B500
>
> Also I think the translators might now complain, but I am not sure how
> to best approach this.

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

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.work@gmail.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.


Michael

Hi all,

On Mon, Jan 12, 2026 at 3:57 PM Michael Banck <mbanck@gmx.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.work@gmail.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

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

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