public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Andres Freund <[email protected]>
Cc: PostgreSQL Developers <[email protected]>
Subject: Re: Make set_ps_display faster and easier to use
Date: Fri, 17 Feb 2023 21:44:06 +1300
Message-ID: <CAApHDvrx-kwtWFaO8_hxS5fBYeAe-Rp0YWATZSwBJx8UVu9bJA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAApHDvocBvvk-0gWNA2Gohe+sv9fMcv+fK_G+siBKJrgDG4O7g@mail.gmail.com>
<[email protected]>
Thank you for having a look at this.
On Fri, 17 Feb 2023 at 14:01, Andres Freund <[email protected]> wrote:
> > +set_ps_display_suffix(const char *suffix)
> > +{
> > + size_t len;
>
> Think this will give you an unused-variable warning in the PS_USE_NONE case.
Fixed
> > +#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
>
> This bit is now repeated three times. How about putting it into a helper?
Good idea. Done.
> > +set_ps_display_internal(void)
>
> Very very minor nit: Perhaps this should be update_ps_display() or
> flush_ps_display() instead?
I called the precheck helper update_ps_display_precheck(), so went
with flush_ps_display() for updating the display so they both didn't
start with "update".
Updated patch attached.
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 c99a4f10fa..720482e581 100644
--- a/src/backend/utils/misc/ps_status.c
+++ b/src/backend/utils/misc/ps_status.c
@@ -86,12 +86,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 flush_ps_display(void);
/*
* Call this early in startup to save the original argc/argv values.
@@ -300,37 +307,152 @@ init_ps_display(const char *fixed_part)
#endif /* not PS_USE_NONE */
}
+#ifndef PS_USE_NONE
+static bool
+update_ps_display_precheck(void)
+{
+ /* update_process_title=off disables updates */
+ if (!update_process_title)
+ return false;
+ /* no ps display for stand-alone backend */
+ if (!IsUnderPostmaster)
+ return false;
+
+#ifdef PS_USE_CLOBBER_ARGV
+ /* If ps_buffer is a pointer, it might still be null */
+ if (!ps_buffer)
+ return false;
+#endif
+
+ return true;
+}
+#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.
+ * 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(const char *activity)
+set_ps_display_suffix(const char *suffix)
{
#ifndef PS_USE_NONE
- /* update_process_title=off disables updates */
- if (!update_process_title)
+ size_t len;
+
+ /* first, check if we need to update the process title */
+ if (!update_ps_display_precheck())
return;
- /* no ps display for stand-alone backend */
- if (!IsUnderPostmaster)
+ 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 */
+ flush_ps_display();
+#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
+ /* first, check if we need to update the process title */
+ if (!update_ps_display_precheck())
return;
-#ifdef PS_USE_CLOBBER_ARGV
- /* If ps_buffer is a pointer, it might still be null */
- if (!ps_buffer)
+ /* 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 */
+ flush_ps_display();
+#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_with_len(const char *activity, size_t len)
+{
+ Assert(strlen(activity) == len);
+
+#ifndef PS_USE_NONE
+ /* first, check if we need to update the process title */
+ if (!update_ps_display_precheck())
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 */
+ flush_ps_display();
+#endif /* not PS_USE_NONE */
+}
+#ifndef PS_USE_NONE
+static void
+flush_ps_display(void)
+{
#ifdef PS_USE_SETPROCTITLE
setproctitle("%s", ps_buffer);
#elif defined(PS_USE_SETPROCTITLE_FAST)
@@ -363,9 +485,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);
Attachments:
[text/plain] optimize_and_make_set_ps_display_better_v2.patch (14.7K, ../CAApHDvrx-kwtWFaO8_hxS5fBYeAe-Rp0YWATZSwBJx8UVu9bJA@mail.gmail.com/2-optimize_and_make_set_ps_display_better_v2.patch)
download | inline diff:
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 c99a4f10fa..720482e581 100644
--- a/src/backend/utils/misc/ps_status.c
+++ b/src/backend/utils/misc/ps_status.c
@@ -86,12 +86,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 flush_ps_display(void);
/*
* Call this early in startup to save the original argc/argv values.
@@ -300,37 +307,152 @@ init_ps_display(const char *fixed_part)
#endif /* not PS_USE_NONE */
}
+#ifndef PS_USE_NONE
+static bool
+update_ps_display_precheck(void)
+{
+ /* update_process_title=off disables updates */
+ if (!update_process_title)
+ return false;
+ /* no ps display for stand-alone backend */
+ if (!IsUnderPostmaster)
+ return false;
+
+#ifdef PS_USE_CLOBBER_ARGV
+ /* If ps_buffer is a pointer, it might still be null */
+ if (!ps_buffer)
+ return false;
+#endif
+
+ return true;
+}
+#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.
+ * 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(const char *activity)
+set_ps_display_suffix(const char *suffix)
{
#ifndef PS_USE_NONE
- /* update_process_title=off disables updates */
- if (!update_process_title)
+ size_t len;
+
+ /* first, check if we need to update the process title */
+ if (!update_ps_display_precheck())
return;
- /* no ps display for stand-alone backend */
- if (!IsUnderPostmaster)
+ 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 */
+ flush_ps_display();
+#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
+ /* first, check if we need to update the process title */
+ if (!update_ps_display_precheck())
return;
-#ifdef PS_USE_CLOBBER_ARGV
- /* If ps_buffer is a pointer, it might still be null */
- if (!ps_buffer)
+ /* 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 */
+ flush_ps_display();
+#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_with_len(const char *activity, size_t len)
+{
+ Assert(strlen(activity) == len);
+
+#ifndef PS_USE_NONE
+ /* first, check if we need to update the process title */
+ if (!update_ps_display_precheck())
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 */
+ flush_ps_display();
+#endif /* not PS_USE_NONE */
+}
+#ifndef PS_USE_NONE
+static void
+flush_ps_display(void)
+{
#ifdef PS_USE_SETPROCTITLE
setproctitle("%s", ps_buffer);
#elif defined(PS_USE_SETPROCTITLE_FAST)
@@ -363,9 +485,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);
view thread (2+ messages)
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Make set_ps_display faster and easier to use
In-Reply-To: <CAApHDvrx-kwtWFaO8_hxS5fBYeAe-Rp0YWATZSwBJx8UVu9bJA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox