Thread: Make set_ps_display faster and easier to use
While doing some benchmarking of some fast-to-execute queries, I see that set_ps_display() popping up on the profiles. Looking a little deeper, there are some inefficiencies in there that we could fix. For example, the following is pretty poor: strlcpy(ps_buffer + ps_buffer_fixed_size, activity, ps_buffer_size - ps_buffer_fixed_size); ps_buffer_cur_len = strlen(ps_buffer); We already know the strlen of the fixed-sized part, so why bother doing strlen on the entire thing? Also, if we did just do strlen(activity), we could just memcpy, which would be much faster than strlcpy's byte-at-a-time method of copying. Adjusting that lead me to notice that we often just pass string constants to set_ps_display(), so we already know the strlen for this at compile time. So maybe we can just have set_ps_display_with_len() and then make a static inline wrapper that does strlen() so that when the compiler can figure out the length, it just hard codes it. After doing that, I went over all usages of set_ps_display() to see if any of those call sites knew the length already in a way that the compiler wouldn't be able to deduce. There were a few cases to adjust when setting the process title to contain the command tag. 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" */ 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(); I considered adding a format version to append the suffix as there's one case that could make use of it, but in the end, decided it might be overkill, so I left that code like: char buffer[32]; sprintf(buffer, "waiting for %X/%X", LSN_FORMAT_ARGS(lsn)); set_ps_display_suffix(buffer); I don't think that's terrible enough to warrant making a va_args version of set_ps_display_suffix(), especially for just 1 instance of it. I also resisted making set_ps_display_suffix_with_len(). The new code should be quite a bit faster already without troubling over that additional function. I've attached the patch. David
Attachment
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
Thank you for having a look at this. On Fri, 17 Feb 2023 at 14:01, Andres Freund <andres@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
Attachment
On Fri, 17 Feb 2023 at 21:44, David Rowley <dgrowleyml@gmail.com> wrote: > Updated patch attached. After making another couple of small adjustments, I've pushed this. Thanks for the review. David