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