Thank you for having a look at this. On Fri, 17 Feb 2023 at 14:01, Andres Freund <and...@anarazel.de> 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);