public inbox for [email protected]  

help / color / mirror / Atom feed

From: Andres Freund <[email protected]>
To: David Rowley <[email protected]>
Cc: PostgreSQL Developers <[email protected]>
Subject: Re: Make set_ps_display faster and easier to use
Date: Thu, 16 Feb 2023 17:01:55 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAApHDvocBvvk-0gWNA2Gohe+sv9fMcv+fK_G+siBKJrgDG4O7g@mail.gmail.com>
References: <CAApHDvocBvvk-0gWNA2Gohe+sv9fMcv+fK_G+siBKJrgDG4O7g@mail.gmail.com>

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

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Make set_ps_display faster and easier to use
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox