On Wed, Dec 09, 2020 at 06:37:22PM +0000, Bossart, Nathan wrote: > On 12/8/20, 10:16 PM, "Michael Paquier" <mich...@paquier.xyz> wrote: >> Yeah, I'd rather leave out the part of the patch where we have the >> checkpointer say "idle". So I still think that what v3 did upthread, >> by just resetting the ps display in all cases, feels more natural. So >> we should remove the parts of v5 from checkpointer.c. > > That seems fine to me. I think it is most important that the end-of- > recovery and shutdown checkpoints are shown. I'm not sure there's > much value in updating the process title for automatic checkpoints and > checkpoints triggered via the CHECKPOINT command, so IMO we could skip > those, too. I made these changes in the new version of the patch.
It would be possible to use pg_stat_activity in most cases here, so I
agree to settle down to the minimum we can agree on for now, and maybe
discuss separately if this should be extended in some or another in
the future if there is a use-case for that. So I'd say that what you
have here is logically fine.
> > + snprintf(activitymsg, sizeof(activitymsg), "creating %s%scheckpoint",
> > [...]
> > + snprintf(activitymsg, sizeof(activitymsg), "creating %srestartpoint",
> > FWIW, I still fing "running" to sound better than "creating" here. An
> > extra term I can think of that could be adapted is "performing".
>
> I think I prefer "performing" over "running" because that's what the
> docs use [0]. I've changed it to "performing" in the new version of
> the patch.
That's also used in the code comments, FWIW.
@@ -9282,6 +9296,7 @@ CreateRestartPoint(int flags)
XLogRecPtr endptr;
XLogSegNo _logSegNo;
TimestampTz xtime;
+ bool shutdown = (flags & CHECKPOINT_IS_SHUTDOWN);
You are right that CHECKPOINT_END_OF_RECOVERY should not be called for
a restart point, so that's correct.
However, I think that it would be better to have all those four code
paths controlled by a single routine, where we pass down the
checkpoint flags and fill in the ps string directly depending on what
the caller wants to do. This way, we will avoid any inconsistent
updates if this stuff gets extended in the future, and there will be
all the information at hand to extend the logic. So I have simplified
your patch as per the attached. Thoughts?
--
Michael
From 98440b74b10c522bd485e072c43631ce78d115a7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 11 Dec 2020 12:53:36 +0900
Subject: [PATCH v7] Add checkpoint/restartpoint status to ps display.
---
src/backend/access/transam/xlog.c | 44 +++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7e81ce4f17..8d0a5e05fc 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8687,6 +8687,38 @@ UpdateCheckPointDistanceEstimate(uint64 nbytes)
(0.90 * CheckPointDistanceEstimate + 0.10 * (double) nbytes);
}
+/*
+ * Update the ps display for a process running a checkpoint. Note that
+ * this routine should not do any allocations so as it can be called
+ * from a critical section.
+ */
+static void
+update_checkpoint_display(int flags, bool restartpoint, bool reset)
+{
+ /*
+ * The status is reported only for end-of-recovery and shutdown checkpoints
+ * or shutdown restartpoints. Updating the ps display is useful in those
+ * situations as it may not be possible to rely on pg_stat_activity to see
+ * the status of the checkpointer or the startup process.
+ */
+ if ((flags & (CHECKPOINT_END_OF_RECOVERY | CHECKPOINT_IS_SHUTDOWN)) == 0)
+ return;
+
+ if (reset)
+ {
+ char activitymsg[128];
+
+ snprintf(activitymsg, sizeof(activitymsg), "performing %s%s%s",
+ (flags & CHECKPOINT_END_OF_RECOVERY) ? "end-of-recovery " : "",
+ (flags & CHECKPOINT_IS_SHUTDOWN) ? "shutdown " : "",
+ restartpoint ? "restartpoint" : "checkpoint");
+ set_ps_display(activitymsg);
+ }
+ else
+ set_ps_display("");
+}
+
+
/*
* Perform a checkpoint --- either during shutdown, or on-the-fly
*
@@ -8905,6 +8937,9 @@ CreateCheckPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, false);
+ /* Update the process title */
+ update_checkpoint_display(flags, false, false);
+
TRACE_POSTGRESQL_CHECKPOINT_START(flags);
/*
@@ -9120,6 +9155,9 @@ CreateCheckPoint(int flags)
/* Real work is done, but log and update stats before releasing lock. */
LogCheckpointEnd(false);
+ /* Reset the process title */
+ update_checkpoint_display(flags, false, true);
+
TRACE_POSTGRESQL_CHECKPOINT_DONE(CheckpointStats.ckpt_bufs_written,
NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -9374,6 +9412,9 @@ CreateRestartPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, true);
+ /* Update the process title */
+ update_checkpoint_display(flags, true, false);
+
CheckPointGuts(lastCheckPoint.redo, flags);
/*
@@ -9492,6 +9533,9 @@ CreateRestartPoint(int flags)
/* Real work is done, but log and update before releasing lock. */
LogCheckpointEnd(true);
+ /* Reset the process title */
+ update_checkpoint_display(flags, true, true);
+
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
--
2.29.2
signature.asc
Description: PGP signature
