While doing some benchmarking of some fast-to-execute queries, I see
that set_ps_display() popping up on the profiles. Looking a little
deeper, there are some inefficiencies in there that we could fix.
For example, the following is pretty poor:
strlcpy(ps_buffer + ps_buffer_fixed_size, activity,
ps_buffer_size - ps_buffer_fixed_size);
ps_buffer_cur_len = strlen(ps_buffer);
We already know the strlen of the fixed-sized part, so why bother
doing strlen on the entire thing? Also, if we did just do
strlen(activity), we could just memcpy, which would be much faster
than strlcpy's byte-at-a-time method of copying.
Adjusting that lead me to notice that we often just pass string
constants to set_ps_display(), so we already know the strlen for this
at compile time. So maybe we can just have set_ps_display_with_len()
and then make a static inline wrapper that does strlen() so that when
the compiler can figure out the length, it just hard codes it.
After doing that, I went over all usages of set_ps_display() to see if
any of those call sites knew the length already in a way that the
compiler wouldn't be able to deduce. There were a few cases to adjust
when setting the process title to contain the command tag.
After fixing up the set_ps_display()s to use set_ps_display_with_len()
where possible, I discovered some not so nice code which appends "
waiting" onto the process title. Basically, there's a bunch of code
that looks like this:
const char *old_status;
int len;
old_status = get_ps_display(&len);
new_status = (char *) palloc(len + 8 + 1);
memcpy(new_status, old_status, len);
strcpy(new_status + len, " waiting");
set_ps_display(new_status);
new_status[len] = '\0'; /* truncate off " waiting" */
Seeing that made me wonder if we shouldn't just have something more
generic for setting a suffix on the process title. I came up with
set_ps_display_suffix() and set_ps_display_remove_suffix(). The above
code can just become:
set_ps_display_suffix("waiting");
then to remove the "waiting" suffix, just:
set_ps_display_remove_suffix();
I considered adding a format version to append the suffix as there's
one case that could make use of it, but in the end, decided it might
be overkill, so I left that code like:
char buffer[32];
sprintf(buffer, "waiting for %X/%X", LSN_FORMAT_ARGS(lsn));
set_ps_display_suffix(buffer);
I don't think that's terrible enough to warrant making a va_args
version of set_ps_display_suffix(), especially for just 1 instance of
it.
I also resisted making set_ps_display_suffix_with_len(). The new code
should be quite a bit
faster already without troubling over that additional function.
I've attached the patch.
David
diff --git a/src/backend/replication/syncrep.c
b/src/backend/replication/syncrep.c
index 80d681b71c..889e20b5dd 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -148,8 +148,6 @@ static bool SyncRepQueueIsOrderedByLSN(int mode);
void
SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
{
- char *new_status = NULL;
- const char *old_status;
int mode;
/*
@@ -216,15 +214,10 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
/* Alter ps display to show waiting for sync rep. */
if (update_process_title)
{
- int len;
-
- old_status = get_ps_display(&len);
- new_status = (char *) palloc(len + 32 + 1);
- memcpy(new_status, old_status, len);
- sprintf(new_status + len, " waiting for %X/%X",
- LSN_FORMAT_ARGS(lsn));
- set_ps_display(new_status);
- new_status[len] = '\0'; /* truncate off " waiting ..." */
+ char buffer[32];
+
+ sprintf(buffer, "waiting for %X/%X", LSN_FORMAT_ARGS(lsn));
+ set_ps_display_suffix(buffer);
}
/*
@@ -322,12 +315,9 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
MyProc->syncRepState = SYNC_REP_NOT_WAITING;
MyProc->waitLSN = 0;
- if (new_status)
- {
- /* Reset ps display */
- set_ps_display(new_status);
- pfree(new_status);
- }
+ /* reset ps display to remove the suffix */
+ if (update_process_title)
+ set_ps_display_remove_suffix();
}
/*
diff --git a/src/backend/storage/buffer/bufmgr.c
b/src/backend/storage/buffer/bufmgr.c
index 2d6dbc6561..98904a7c05 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4302,8 +4302,8 @@ void
LockBufferForCleanup(Buffer buffer)
{
BufferDesc *bufHdr;
- char *new_status = NULL;
TimestampTz waitStart = 0;
+ bool waiting = false;
bool logged_recovery_conflict = false;
Assert(BufferIsPinned(buffer));
@@ -4350,11 +4350,11 @@ LockBufferForCleanup(Buffer buffer)
waitStart, GetCurrentTimestamp(),
NULL,
false);
- /* Report change to non-waiting status */
- if (new_status)
+ if (waiting)
{
- set_ps_display(new_status);
- pfree(new_status);
+ /* reset ps display to remove the suffix if we
added one */
+ set_ps_display_remove_suffix();
+ waiting = false;
}
return;
}
@@ -4374,18 +4374,11 @@ LockBufferForCleanup(Buffer buffer)
/* Wait to be signaled by UnpinBuffer() */
if (InHotStandby)
{
- /* Report change to waiting status */
- if (update_process_title && new_status == NULL)
+ if (!waiting)
{
- const char *old_status;
- int len;
-
- old_status = get_ps_display(&len);
- new_status = (char *) palloc(len + 8 + 1);
- memcpy(new_status, old_status, len);
- strcpy(new_status + len, " waiting");
- set_ps_display(new_status);
- new_status[len] = '\0'; /* truncate off "
waiting" */
+ /* adjust the process title to indicate that
it's waiting */
+ set_ps_display_suffix("waiting");
+ waiting = true;
}
/*
diff --git a/src/backend/storage/ipc/standby.c
b/src/backend/storage/ipc/standby.c
index 94cc860f5f..9a73ae67d0 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -362,7 +362,7 @@ ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId
*waitlist,
bool
report_waiting)
{
TimestampTz waitStart = 0;
- char *new_status = NULL;
+ bool waiting = false;
bool logged_recovery_conflict = false;
/* Fast exit, to avoid a kernel call if there's no work to be done. */
@@ -400,14 +400,14 @@
ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist,
pg_usleep(5000L);
}
- if (waitStart != 0 && (!logged_recovery_conflict ||
new_status == NULL))
+ if (waitStart != 0 && (!logged_recovery_conflict ||
!waiting))
{
TimestampTz now = 0;
bool maybe_log_conflict;
bool maybe_update_title;
maybe_log_conflict =
(log_recovery_conflict_waits && !logged_recovery_conflict);
- maybe_update_title = (update_process_title &&
new_status == NULL);
+ maybe_update_title = (update_process_title &&
!waiting);
/* Get the current timestamp if not report yet
*/
if (maybe_log_conflict || maybe_update_title)
@@ -420,15 +420,8 @@
ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist,
if (maybe_update_title &&
TimestampDifferenceExceeds(waitStart,
now, 500))
{
- const char *old_status;
- int len;
-
- old_status = get_ps_display(&len);
- new_status = (char *) palloc(len + 8 +
1);
- memcpy(new_status, old_status, len);
- strcpy(new_status + len, " waiting");
- set_ps_display(new_status);
- new_status[len] = '\0'; /* truncate off
" waiting" */
+ set_ps_display_suffix("waiting");
+ waiting = true;
}
/*
@@ -456,12 +449,10 @@
ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist,
LogRecoveryConflict(reason, waitStart, GetCurrentTimestamp(),
NULL, false);
- /* Reset ps display if we changed it */
- if (new_status)
- {
- set_ps_display(new_status);
- pfree(new_status);
- }
+ /* reset ps display to remove the suffix if we added one */
+ if (waiting)
+ set_ps_display_remove_suffix();
+
}
/*
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index a87372f33f..af1b0146a9 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -1810,24 +1810,12 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
{
LOCKMETHODID lockmethodid = LOCALLOCK_LOCKMETHOD(*locallock);
LockMethod lockMethodTable = LockMethods[lockmethodid];
- char *volatile new_status = NULL;
LOCK_PRINT("WaitOnLock: sleeping on lock",
locallock->lock, locallock->tag.mode);
- /* Report change to waiting status */
- if (update_process_title)
- {
- const char *old_status;
- int len;
-
- old_status = get_ps_display(&len);
- new_status = (char *) palloc(len + 8 + 1);
- memcpy(new_status, old_status, len);
- strcpy(new_status + len, " waiting");
- set_ps_display(new_status);
- new_status[len] = '\0'; /* truncate off " waiting" */
- }
+ /* adjust the process title to indicate that it's waiting */
+ set_ps_display_suffix("waiting");
awaitedLock = locallock;
awaitedOwner = owner;
@@ -1874,12 +1862,8 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
{
/* In this path, awaitedLock remains set until LockErrorCleanup
*/
- /* Report change to non-waiting status */
- if (update_process_title)
- {
- set_ps_display(new_status);
- pfree(new_status);
- }
+ /* reset ps display to remove the suffix if we added one */
+ set_ps_display_remove_suffix();
/* and propagate the error */
PG_RE_THROW();
@@ -1888,12 +1872,8 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
awaitedLock = NULL;
- /* Report change to non-waiting status */
- if (update_process_title)
- {
- set_ps_display(new_status);
- pfree(new_status);
- }
+ /* reset ps display to remove the suffix if we added one */
+ set_ps_display_remove_suffix();
LOCK_PRINT("WaitOnLock: wakeup on lock",
locallock->lock, locallock->tag.mode);
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 5d439f2710..cab709b07b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1071,6 +1071,8 @@ exec_simple_query(const char *query_string)
Portal portal;
DestReceiver *receiver;
int16 format;
+ const char *cmdtagname;
+ size_t cmdtaglen;
pgstat_report_query_id(0, true);
@@ -1081,8 +1083,9 @@ exec_simple_query(const char *query_string)
* destination.
*/
commandTag = CreateCommandTag(parsetree->stmt);
+ cmdtagname = GetCommandTagNameAndLen(commandTag, &cmdtaglen);
- set_ps_display(GetCommandTagName(commandTag));
+ set_ps_display_with_len(cmdtagname, cmdtaglen);
BeginCommand(commandTag, dest);
@@ -2064,6 +2067,8 @@ exec_execute_message(const char *portal_name, long
max_rows)
char msec_str[32];
ParamsErrorCbData params_data;
ErrorContextCallback params_errcxt;
+ const char *cmdtagname;
+ size_t cmdtaglen;
/* Adjust destination to tell printtup.c what to do */
dest = whereToSendOutput;
@@ -2110,7 +2115,9 @@ exec_execute_message(const char *portal_name, long
max_rows)
pgstat_report_activity(STATE_RUNNING, sourceText);
- set_ps_display(GetCommandTagName(portal->commandTag));
+ cmdtagname = GetCommandTagNameAndLen(portal->commandTag, &cmdtaglen);
+
+ set_ps_display_with_len(cmdtagname, cmdtaglen);
if (save_log_statement_stats)
ResetUsage();
diff --git a/src/backend/utils/misc/ps_status.c
b/src/backend/utils/misc/ps_status.c
index 06bdc2afd1..45cf76b0cd 100644
--- a/src/backend/utils/misc/ps_status.c
+++ b/src/backend/utils/misc/ps_status.c
@@ -100,12 +100,19 @@ static size_t ps_buffer_cur_len; /* nominal
strlen(ps_buffer) */
static size_t ps_buffer_fixed_size; /* size of the constant prefix */
+/*
+ * Length of ps_buffer before the suffix was appeneded to the end, or 0 if we
+ * didn't set a suffix.
+ */
+static size_t ps_buffer_nosuffix_len;
+
#endif /* not PS_USE_NONE */
/* save the original argv[] location here */
static int save_argc;
static char **save_argv;
+static void set_ps_display_internal(void);
/*
* Call this early in startup to save the original argc/argv values.
@@ -332,15 +339,117 @@ init_ps_display(const char *fixed_part)
#endif /* not PS_USE_NONE */
}
+/*
+ * set_ps_display_suffix
+ * Adjust the process title to append 'suffix' onto the end with a
space
+ * between it and the current process title.
+ */
+void
+set_ps_display_suffix(const char *suffix)
+{
+ size_t len;
+
+#ifndef PS_USE_NONE
+ /* update_process_title=off disables updates */
+ if (!update_process_title)
+ return;
+
+ /* no ps display for stand-alone backend */
+ if (!IsUnderPostmaster)
+ return;
+
+#ifdef PS_USE_CLOBBER_ARGV
+ /* If ps_buffer is a pointer, it might still be null */
+ if (!ps_buffer)
+ return;
+#endif
+
+ if (ps_buffer_nosuffix_len > 0)
+ ps_buffer_cur_len = ps_buffer_nosuffix_len;
+ else
+ ps_buffer_nosuffix_len = ps_buffer_cur_len;
+
+ len = strlen(suffix);
+
+ /* check if we have enough space to append the suffix */
+ if (ps_buffer_cur_len + len + 1 >= ps_buffer_size)
+ {
+ /* not enough space. Check the buffer isn't full already */
+ if (ps_buffer_cur_len < ps_buffer_size - 1)
+ {
+ /* append a space before the suffix */
+ ps_buffer[ps_buffer_cur_len++] = ' ';
+
+ /* Not enough space. Just add what we can */
+ memcpy(ps_buffer + ps_buffer_cur_len, suffix,
+ ps_buffer_size - ps_buffer_cur_len - 1);
+ ps_buffer[ps_buffer_size - 1] = '\0';
+ ps_buffer_cur_len = ps_buffer_size - 1;
+ }
+ }
+ else
+ {
+ ps_buffer[ps_buffer_cur_len++] = ' ';
+ memcpy(ps_buffer + ps_buffer_cur_len, suffix, len + 1);
+ ps_buffer_cur_len = ps_buffer_cur_len + len;
+ }
+
+ Assert(strlen(ps_buffer) == ps_buffer_cur_len);
+
+ /* and set the new title */
+ set_ps_display_internal();
+#endif /* not PS_USE_NONE */
+}
+
+/*
+ * set_ps_display_remove_suffix
+ * Remove the process display suffix added by set_ps_display_suffix
+ */
+void
+set_ps_display_remove_suffix(void)
+{
+#ifndef PS_USE_NONE
+ /* update_process_title=off disables updates */
+ if (!update_process_title)
+ return;
+
+ /* no ps display for stand-alone backend */
+ if (!IsUnderPostmaster)
+ return;
+
+#ifdef PS_USE_CLOBBER_ARGV
+ /* If ps_buffer is a pointer, it might still be null */
+ if (!ps_buffer)
+ return;
+#endif
+
+ /* check we added a suffix */
+ if (ps_buffer_nosuffix_len == 0)
+ return; /* no suffix */
+ /* remove the suffix from ps_buffer */
+ ps_buffer[ps_buffer_nosuffix_len] = '\0';
+ ps_buffer_cur_len = ps_buffer_nosuffix_len;
+ ps_buffer_nosuffix_len = 0;
+
+ Assert(ps_buffer_cur_len == strlen(ps_buffer));
+
+ /* and set the new title */
+ set_ps_display_internal();
+#endif /* not PS_USE_NONE */
+}
/*
* Call this to update the ps status display to a fixed prefix plus an
* indication of what you're currently doing passed in the argument.
+ *
+ * 'len' must be the same as strlen(activity)
*/
void
-set_ps_display(const char *activity)
+set_ps_display_with_len(const char *activity, size_t len)
{
+ Assert(strlen(activity) == len);
+
#ifndef PS_USE_NONE
/* update_process_title=off disables updates */
if (!update_process_title)
@@ -356,13 +465,34 @@ set_ps_display(const char *activity)
return;
#endif
+ /* wipe out the suffix when the title is completely changed */
+ ps_buffer_nosuffix_len = 0;
+
/* Update ps_buffer to contain both fixed part and activity */
- strlcpy(ps_buffer + ps_buffer_fixed_size, activity,
- ps_buffer_size - ps_buffer_fixed_size);
- ps_buffer_cur_len = strlen(ps_buffer);
+ if (ps_buffer_fixed_size + len >= ps_buffer_size)
+ {
+ /* handle the case where ps_buffer doesn't have enough space */
+ memcpy(ps_buffer + ps_buffer_fixed_size, activity,
+ ps_buffer_size - ps_buffer_fixed_size - 1);
+ ps_buffer[ps_buffer_size - 1] = '\0';
+ ps_buffer_cur_len = ps_buffer_size - 1;
+ }
+ else
+ {
+ memcpy(ps_buffer + ps_buffer_fixed_size, activity, len + 1);
+ ps_buffer_cur_len = ps_buffer_fixed_size + len;
+ }
+ Assert(strlen(ps_buffer) == ps_buffer_cur_len);
/* Transmit new setting to kernel, if necessary */
+ set_ps_display_internal();
+#endif /* not PS_USE_NONE */
+}
+#ifndef PS_USE_NONE
+static void
+set_ps_display_internal(void)
+{
#ifdef PS_USE_SETPROCTITLE
setproctitle("%s", ps_buffer);
#elif defined(PS_USE_SETPROCTITLE_FAST)
@@ -400,9 +530,8 @@ set_ps_display(const char *activity)
ident_handle = CreateEvent(NULL, TRUE, FALSE, name);
}
#endif /* PS_USE_WIN32 */
-#endif /* not PS_USE_NONE */
}
-
+#endif
/*
* Returns what's currently in the ps display, in case someone needs
diff --git a/src/include/utils/ps_status.h b/src/include/utils/ps_status.h
index 6953a326f1..ff5a2b2b8a 100644
--- a/src/include/utils/ps_status.h
+++ b/src/include/utils/ps_status.h
@@ -25,7 +25,22 @@ extern char **save_ps_display_args(int argc, char **argv);
extern void init_ps_display(const char *fixed_part);
-extern void set_ps_display(const char *activity);
+extern void set_ps_display_suffix(const char *suffix);
+
+extern void set_ps_display_remove_suffix(void);
+
+extern void set_ps_display_with_len(const char *activity, size_t len);
+
+/*
+ * set_ps_display
+ * inlined to allow strlen to be evaluated during compilation when
+ * passing string constants.
+ */
+static inline void
+set_ps_display(const char *activity)
+{
+ set_ps_display_with_len(activity, strlen(activity));
+}
extern const char *get_ps_display(int *displen);
