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
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 ..." */
+ charbuffer[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;
+ boolwaiting = false;
boollogged_recovery_conflict = false;
Assert(BufferIsPinned(buffer));
@@ -4350,11 +4350,11 @@ LockBufferForCleanup(Buffer buffer)
Re: Make set_ps_display faster and easier to use
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
Re: Make set_ps_display faster and easier to use
Thank you for having a look at this.
On Fri, 17 Feb 2023 at 14:01, Andres Freund 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 ..." */
+ charbuffer[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;
+ boolwaiting = false;
boollogged_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);
-
Re: Make set_ps_display faster and easier to use
On Fri, 17 Feb 2023 at 21:44, David Rowley wrote: > Updated patch attached. After making another couple of small adjustments, I've pushed this. Thanks for the review. David
