Hi all, On Mon, Jan 12, 2026 at 3:57 PM Michael Banck <[email protected]> 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 <[email protected]> > > 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
From c2ed1eb81b38c36399b8fbec5bf09326b06fefa0 Mon Sep 17 00:00:00 2001 From: Soumya <[email protected]> Date: Tue, 13 Jan 2026 11:50:53 +0530 Subject: [PATCH] Expose checkpoint reason in completion log messages Signed-off-by: Soumya <[email protected]> --- src/backend/access/transam/xlog.c | 79 ++++++++++++++++------- src/test/recovery/t/019_replslot_limit.pl | 2 +- 2 files changed, 56 insertions(+), 25 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 22d0a2e8c3..8cdf2b23c3 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6714,6 +6714,50 @@ 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 (...)". + */ +static const char * +CheckpointReasonString(int flags) +{ + static char buf[128]; + bool first = true; + + buf[0] = '\0'; + +#define APPEND_REASON(str) \ + do \ + { \ + if (!first) \ + strcat(buf, " "); \ + strcat(buf, (str)); \ + first = false; \ + } while (0) + + 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; +} + /* * Log start of a checkpoint. */ @@ -6723,34 +6767,19 @@ LogCheckpointStart(int flags, bool restartpoint) if (restartpoint) ereport(LOG, /* translator: the placeholders show checkpoint options */ - (errmsg("restartpoint starting:%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" : ""))); + (errmsg("restartpoint starting: %s", + CheckpointReasonString(flags)))); else ereport(LOG, - /* translator: the placeholders show checkpoint options */ - (errmsg("checkpoint starting:%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" : ""))); + (errmsg("checkpoint starting: %s", + CheckpointReasonString(flags)))); } /* * Log end of a checkpoint. */ static void -LogCheckpointEnd(bool restartpoint) +LogCheckpointEnd(bool restartpoint, int flags) { long write_msecs, sync_msecs, @@ -6800,12 +6829,13 @@ LogCheckpointEnd(bool restartpoint) */ if (restartpoint) ereport(LOG, - (errmsg("restartpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("restartpoint complete (%s): wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointReasonString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -6824,12 +6854,13 @@ LogCheckpointEnd(bool restartpoint) LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo)))); else ereport(LOG, - (errmsg("checkpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("checkpoint complete (%s): wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointReasonString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -7418,7 +7449,7 @@ CreateCheckPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(false); + LogCheckpointEnd(false, flags); /* Reset the process title */ update_checkpoint_display(flags, false, true); @@ -7886,7 +7917,7 @@ CreateRestartPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(true); + LogCheckpointEnd(true, flags); /* Reset the process title */ update_checkpoint_display(flags, true, true); diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl index 6468784b83..ccc363157e 100644 --- a/src/test/recovery/t/019_replslot_limit.pl +++ b/src/test/recovery/t/019_replslot_limit.pl @@ -203,7 +203,7 @@ is($result, "rep1|f|t|lost|", my $checkpoint_ended = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if ($node_primary->log_contains("checkpoint complete: ", $logstart)) + if ($node_primary->log_contains(qr/checkpoint complete(?: \([^)]*\))?:/, $logstart)) { $checkpoint_ended = 1; last; -- 2.34.1
