Speedup and increase usability of set proc title functions
authorDavid Rowley <drowley@postgresql.org>
Mon, 20 Feb 2023 03:18:27 +0000 (16:18 +1300)
committerDavid Rowley <drowley@postgresql.org>
Mon, 20 Feb 2023 03:18:27 +0000 (16:18 +1300)
The setting of the process title could be seen on profiles of very
fast-to-execute queries.  In many locations where we call
set_ps_display() we pass along a string constant, the length of which is
known during compilation.  Here we effectively rename set_ps_display() to
set_ps_display_with_len() and then add a static inline function named
set_ps_display() which calls strlen() on the given string.  This allows
the compiler to optimize away the strlen() call when dealing with
call sites passing a string constant.  We can then also use memcpy()
instead of strlcpy() to copy the string into the destination buffer.
That's significantly faster than strlcpy's byte-at-a-time way of
copying.

Here we also take measures to improve some code which was adjusting the
process title to add a " waiting" suffix to it.  Call sites which require
this can now just call set_ps_display_suffix() to add or adjust the suffix
and call set_ps_display_remove_suffix() to remove it again.

Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/CAApHDvocBvvk-0gWNA2Gohe+sv9fMcv+fK_G+siBKJrgDG4O7g@mail.gmail.com

src/backend/replication/syncrep.c
src/backend/storage/buffer/bufmgr.c
src/backend/storage/ipc/standby.c
src/backend/storage/lmgr/lock.c
src/backend/tcop/postgres.c
src/backend/utils/misc/ps_status.c
src/include/utils/ps_status.h

index 80d681b71c84c97ee3bab8dcc3649169e670ec54..889e20b5dd5ba26cd2f7a17a0fab1e4943da70f5 100644 (file)
@@ -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();
 }
 
 /*
index 2d6dbc6561238c8d3f652f439e26bdc831ae7103..98904a7c05ac995b204e0d70ad2680f9f6885c04 100644 (file)
@@ -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;
                        }
 
                        /*
index 94cc860f5fa79aa70ca0fd4c5f8525d637e5d762..9a73ae67d0b4638212eb55c5961b42ab92fd4f14 100644 (file)
@@ -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();
+
 }
 
 /*
index a87372f33f950e3459bacf3d7bebc554e4d96298..42595b38b2c0086d4014e463794c73d889679dc9 100644 (file)
@@ -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 */
+               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 */
+       set_ps_display_remove_suffix();
 
        LOCK_PRINT("WaitOnLock: wakeup on lock",
                           locallock->lock, locallock->tag.mode);
index 5d439f27100407255ac601e5b4e55fe0b831380d..cab709b07b15ba7ca9474659f68f9c0d1c4f1e8a 100644 (file)
@@ -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();
index c99a4f10fa59ad506424b10d96708351cb2c11de..3894a017f3dc78bcb049551e82677609119e1092 100644 (file)
@@ -86,6 +86,14 @@ 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;
+
+static void flush_ps_display(void);
+
 #endif                                                 /* not PS_USE_NONE */
 
 /* save the original argv[] location here */
@@ -300,37 +308,158 @@ init_ps_display(const char *fixed_part)
 #endif                                                 /* not PS_USE_NONE */
 }
 
-
-
+#ifndef 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.
+ * update_ps_display_precheck
+ *             Helper function to determine if updating the process title is
+ *             something that we need to do.
  */
-void
-set_ps_display(const char *activity)
+static bool
+update_ps_display_precheck(void)
 {
-#ifndef PS_USE_NONE
        /* update_process_title=off disables updates */
        if (!update_process_title)
-               return;
+               return false;
 
        /* no ps display for stand-alone backend */
        if (!IsUnderPostmaster)
-               return;
+               return false;
 
 #ifdef PS_USE_CLOBBER_ARGV
        /* If ps_buffer is a pointer, it might still be null */
        if (!ps_buffer)
-               return;
+               return false;
 #endif
 
+       return true;
+}
+#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)
+{
+#ifndef PS_USE_NONE
+       size_t          len;
+
+       /* first, check if we need to update the process title */
+       if (!update_ps_display_precheck())
+               return;
+
+       /* if there's already a suffix, overwrite it */
+       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++] = ' ';
+
+                       /* just add what we can and fill the ps_buffer */
+                       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;
+
+       /* 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;
+
+       /* wipe out any 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 +492,8 @@ set_ps_display(const char *activity)
                ident_handle = CreateEvent(NULL, TRUE, FALSE, name);
        }
 #endif                                                 /* PS_USE_WIN32 */
-#endif                                                 /* not PS_USE_NONE */
 }
-
+#endif                                                 /* not PS_USE_NONE */
 
 /*
  * Returns what's currently in the ps display, in case someone needs
index 6953a326f12ad7ad3eba954eb81b546eb68f0f54..ff5a2b2b8a2274c562c3b2ef62d0011d3591a0b4 100644 (file)
@@ -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);