Hi, On 2023-02-16 14:19:24 +1300, David Rowley wrote: > 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" */
Yea, that code is atrocious... It took me a while to figure out that no, LockBufferForCleanup() isn't leaking memory, because it'll always reach the cleanup path *further up* in the function. Avoiding the allocation across loop iterations seems like a completely pointless optimization in these paths - we add the " waiting", precisely because it's a slow path. But of course not allocating memory would be even better... > 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(); That'd definitely be better. It's not really a topic for this patch, but somehow the fact that we have these set_ps_display() calls all over feels wrong, particularly because most of them are paired with a pgstat_report_activity() call. It's not entirely obvious how it should be instead, but it doesn't feel right. > +/* > + * 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; Think this will give you an unused-variable warning in the PS_USE_NONE case. > +#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? > +#ifndef PS_USE_NONE > +static void > +set_ps_display_internal(void) Very very minor nit: Perhaps this should be update_ps_display() or flush_ps_display() instead? Greetings, Andres Freund