From: Bossart, Nathan <bossartn@amazon.com>
To: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: k.jamison@fujitsu.com <k.jamison@fujitsu.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: please update ps display for recovery checkpoint
Date: Thu, 3 Dec 2020 21:18:07 +0000
Message-ID: <9A1B9D98-4576-438A-943F-0AFB05946C7C@amazon.com> (raw)
In-Reply-To: <20201002091354.GH8476@telsasoft.com>
References: <20200818225238.GP17022@telsasoft.com>
<OSBPR01MB2341D2D3141FB3E4C97A0B4DEF5D0@OSBPR01MB2341.jpnprd01.prod.outlook.com>
<20200820080905.GA10353@paquier.xyz>
<20200831065244.GB6555@paquier.xyz>
<20200910020050.GB18552@telsasoft.com>
<20200910043710.GD2743@paquier.xyz>
<20200919160031.GC30557@telsasoft.com>
<20201002072814.GE1464@paquier.xyz>
<20201002091354.GH8476@telsasoft.com>
Hi,
I like the idea behind this patch. I wrote a new version with a
couple of changes:
1. Instead of using StringInfoData, just use a small buffer along
with snprintf(). This is similar to what the WAL receiver
process does.
2. If the process is the checkpointer, reset the display to "idle"
once the checkpoint or restartpoint is finished. It's easy
enough to discover the backend type, and I think adding a bit
more clarity to the checkpointer display is a nice touch.
3. Instead of "running," use "creating." IMO that's a bit more
accurate.
I considered also checking that update_process_title was enabled, but
I figured that these ps display updates should happen sparsely enough
that it wouldn't make much of an impact.
What do you think?
Nathan
Attachments:v4-0001-Add-checkpoint-restartpoint-status-to-ps-display.patch (application/octet-stream, 3.3K) download
From befcf8b844fc602b6e038a04a03949ccb52f8b98 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Thu, 3 Dec 2020 21:10:48 +0000
Subject: [PATCH v4 1/1] Add checkpoint/restartpoint status to ps display.
---
src/backend/access/transam/xlog.c | 31 +++++++++++++++++++++++++++++++
src/backend/postmaster/checkpointer.c | 6 ++++++
2 files changed, 37 insertions(+)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.cindex 13f1d8c3dc..b36a3bb7cc 100644--- a/src/backend/access/transam/xlog.c+++ b/src/backend/access/transam/xlog.c@@ -8694,6 +8694,7 @@ CreateCheckPoint(int flags)
XLogRecPtr last_important_lsn;
VirtualTransactionId *vxids;
int nvxids;
+ char activitymsg[64];
/*
* An end-of-recovery checkpoint is really a shutdown checkpoint, just
@@ -8869,6 +8870,11 @@ CreateCheckPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, false);
+ snprintf(activitymsg, sizeof(activitymsg), "creating %s%scheckpoint",+ (flags & CHECKPOINT_END_OF_RECOVERY) ? "end-of-recovery " : "",+ (flags & CHECKPOINT_IS_SHUTDOWN) ? "shutdown " : "");+ set_ps_display(activitymsg);+
TRACE_POSTGRESQL_CHECKPOINT_START(flags);
/*
@@ -9084,6 +9090,16 @@ CreateCheckPoint(int flags)
/* Real work is done, but log and update stats before releasing lock. */
LogCheckpointEnd(false);
+ /*+ * Reset the ps status display. We set the status to "idle" for the+ * checkpointer process, and we clear it for anything else that runs this+ * code.+ */+ if (MyBackendType == B_CHECKPOINTER)+ set_ps_display("idle");+ else+ set_ps_display("");+
TRACE_POSTGRESQL_CHECKPOINT_DONE(CheckpointStats.ckpt_bufs_written,
NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -9246,6 +9262,7 @@ CreateRestartPoint(int flags)
XLogRecPtr endptr;
XLogSegNo _logSegNo;
TimestampTz xtime;
+ char activitymsg[64];
/*
* Acquire CheckpointLock to ensure only one restartpoint or checkpoint
@@ -9338,6 +9355,10 @@ CreateRestartPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, true);
+ snprintf(activitymsg, sizeof(activitymsg), "creating %srestartpoint",+ (flags & CHECKPOINT_IS_SHUTDOWN) ? "shutdown " : "");+ set_ps_display(activitymsg);+
CheckPointGuts(lastCheckPoint.redo, flags);
/*
@@ -9456,6 +9477,16 @@ CreateRestartPoint(int flags)
/* Real work is done, but log and update before releasing lock. */
LogCheckpointEnd(true);
+ /*+ * Reset the ps status display. We set the status to "idle" for the+ * checkpointer process, and we clear it for anything else that runs this+ * code.+ */+ if (MyBackendType == B_CHECKPOINTER)+ set_ps_display("idle");+ else+ set_ps_display("");+
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.cindex 429c8010ef..a0334c3b78 100644--- a/src/backend/postmaster/checkpointer.c+++ b/src/backend/postmaster/checkpointer.c@@ -332,6 +332,12 @@ CheckpointerMain(void)
*/
ProcGlobal->checkpointerLatch = &MyProc->procLatch;
+ /*+ * Initialize ps display. CreateCheckPoint() and CreateRestartPoint()+ * handle updating it as needed.+ */+ set_ps_display("idle");+
/*
* Loop forever
*/
--
2.16.6
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bossartn@amazon.com, pryzby@telsasoft.com, michael@paquier.xyz, k.jamison@fujitsu.com
Subject: Re: please update ps display for recovery checkpoint
In-Reply-To: <9A1B9D98-4576-438A-943F-0AFB05946C7C@amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox