Speedup and increase usability of set proc title functions

david-rowley · pull[bot] · commit 9ae72c5ae70e · 2023-10-26T07:29:53.000Z
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
diff --git 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
@@ -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
@@ -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
@@ -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);
diff --git 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
diff --git a/src/include/utils/ps_status.h b/src/include/utils/ps_status.h
