| From: | Michael Banck <mbanck(at)gmx(dot)net> |
|---|---|
| To: | Soumya S Murali <soumyamurali(dot)work(at)gmail(dot)com> |
| Cc: | Álvaro Herrera <alvherre(at)kurilemu(dot)de>, pgsql-hackers(at)lists(dot)postgresql(dot)org, melanieplageman(at)gmail(dot)com, juanjo(dot)santamaria(at)gmail(dot)com |
| Subject: | Re: [PATCH] Expose checkpoint timestamp and duration in pg_stat_checkpointer |
| Date: | 2025-11-28 08:08:50 |
| Message-ID: | 20251128080849.GA13635@p46.dedyn.io;lightning.p46.dedyn.io |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
On Fri, Nov 28, 2025 at 10:23:54AM +0530, Soumya S Murali wrote:
> I have updated the code based on the feedback received to my earlier
> mails and prepared a patch for further review.
I think the logging change and the pg_stat_checkpointer changes are
different enough that they should be separate patches. If not just
because the logging change seems to not have had any non-positive
feedback.
> In this patch, I have renamed the checkpoint_total_time to
> last_checkpoint_duration, stats_reset has been kept as the last column
> following the usual pattern, last_checkpoint_duration and
> last_checkpoint_time will now be overwritten per checkpoint and also
> have removed unnecessary lines as per the usual format. I had
> successfully verified the checkpointer duration with different write
> loads and I am attaching the observations for further reference.
I am still not convinced of the usefulness of those changes to
pg_stat_checkpointer, but some feedback on the patch:
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 9217508917..4a45f4f708 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6778,7 +6778,7 @@ LogCheckpointEnd(bool restartpoint, int flags)
/* Store in PendingCheckpointerStats */
- PendingCheckpointerStats.checkpoint_total_time += (double) total_msecs;
+ PendingCheckpointerStats.last_checkpoint_duration = (double) total_msecs;
PendingCheckpointerStats.last_checkpoint_time = CheckpointStats.ckpt_end_t;
[...]
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index a8eb1f8add..73688041c8 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -263,7 +263,7 @@ typedef struct PgStat_CheckpointerStats
PgStat_Counter sync_time;
PgStat_Counter buffers_written;
PgStat_Counter slru_written;
- PgStat_Counter checkpoint_total_time; /* new: total ms of last checkpoint */
+ PgStat_Counter last_checkpoint_duration; /* new: total ms of last checkpoint */
TimestampTz last_checkpoint_time; /* new: end time of last checkpoint */
TimestampTz stat_reset_timestamp;
} PgStat_CheckpointerStats;
This looks like an incremental patch based on your original one? It is
customary to send the full, updated, patch again.
Michael
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Eisentraut | 2025-11-28 08:11:16 | Re: Remove useless casting to the same type |
| Previous Message | Peter Eisentraut | 2025-11-28 07:40:50 | Re: more C99 cleanup |