Обсуждение: 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

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

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