Subject: Re: Add checkpoint and redo LSN to LogCheckpointEnd log message

From: Greg Stark <[email-masked]>
Date: Mon, 28 Mar 2022 15:31:16 -0400

This patch is currently showing up with a test failure in the CFBot
however I do *not* believe this is a bug in the patch. I think it's a
bug in that test which is being discussed elsewhere.

It's also a very short and straightforward patch that a committer
could probably make a decision about whether it's a good idea or not
and then apply it quickly if so.

Just to give people a leg up and an idea how short the patch is...
Here's the entire patch:

diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index ed16f279b1..b85c76d8f8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6121,7 +6121,8 @@ LogCheckpointEnd(bool restartpoint)
  "%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",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
  CheckpointStats.ckpt_bufs_written,
  (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
  CheckpointStats.ckpt_segs_added,
@@ -6134,14 +6135,21 @@ LogCheckpointEnd(bool restartpoint)
  longest_msecs / 1000, (int) (longest_msecs % 1000),
  average_msecs / 1000, (int) (average_msecs % 1000),
  (int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /*
+ * ControlFileLock is not required as we are the only
+ * updator of these variables.
+ */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
  else
  ereport(LOG,
  (errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
  "%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",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
  CheckpointStats.ckpt_bufs_written,
  (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
  CheckpointStats.ckpt_segs_added,
@@ -6154,7 +6162,13 @@ LogCheckpointEnd(bool restartpoint)
  longest_msecs / 1000, (int) (longest_msecs % 1000),
  average_msecs / 1000, (int) (average_msecs % 1000),
  (int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /*
+ * ControlFileLock is not required as we are the only
+ * updator of these variables.
+ */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
 }

 /*
-- 
2.27.0

---

From: Fujii Masao <[email-masked]>
Date: Thu, 7 Jul 2022 01:11:33 +0900

On 2022/03/16 10:29, Kyotaro Horiguchi wrote:
> At Wed, 16 Mar 2022 09:19:13 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in
>> In short, I split out the two topics other than checkpoint log to
>> other threads.
> 
> So, this is about the main topic of this thread, adding LSNs to
> checkpint log.  Other topics have moved to other treads [1], [2] ,
> [3].
> 
> I think this is no longer controversial alone.  So this patch is now
> really Read-for-Commiter and is waiting to be picked up.

+1

Isn't it better to add "at this time" or something at the end of the comment because only we're not always updator of them? No?

Barring any objection, I'm thinking to apply the above small change and commit the patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 07 Jul 2022 16:26:59 +0900 (JST)

At Thu, 7 Jul 2022 01:11:33 +0900, Fujii Masao <[email-masked]> wrote in 
> Isn't it better to add "at this time" or something at the end of the
> comment because only we're not always updator of them? No?

Excluding initialization, (I believe) checkpointer is really the only
updator of the variables/members.  But I'm fine with the addition.

> Barring any objection, I'm thinking to apply the above small change

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Fujii Masao <[email-masked]>
Date: Thu, 7 Jul 2022 22:45:39 +0900

> updator of the variables/members.  But I'm fine with the addition.

Ok, so I modified the patch slightly and pushed it. Thanks!

Regards,