Re: Speedup generation of command completion tags - Mailing list pgsql-hackers

Thanks for having a look at this.

On Sun, 11 Dec 2022 at 11:05, Andres Freund <andres@anarazel.de> wrote:
>
> On 2022-12-10 20:32:06 +1300, David Rowley wrote:
> > @@ -20,13 +20,14 @@
> >  typedef struct CommandTagBehavior
> >  {
> >       const char *name;
> > +     const uint8 namelen;
>
> Perhaps worth adding a comment noting that namelen is the length without the
> null byte?

I've added that now plus a few other fields that could be easily
documented. I left out commenting all of the remaining fields as
documenting table_rewrite_ok seemed slightly more than this patch
should be doing.  There's commands that rewrite tables, e.g. VACUUM
FULL that have that as false. Looks like this is only used for
commands that *might* rewrite the table. I didn't want to tackle that
in this patch.

> What about moving make_completion_tag() to cmdtag.c? Then we could just get
> the entire CommandTagBehaviour struct at once. It's not super pretty to pass
> QueryCompletion to a routine in cmdtag.c, but it's not awful. And if we deem
> it problematic, we could just pass qc->commandTag, qc->nprocessed as a
> separate arguments.

That seems like a good idea. I've renamed and moved the function in
the attached. I also adjusted how the trailing NUL char is appended to
avoid having to calculate len + 1 and append the NUL char twice for
the most commonly taken path.

> I wonder if any of the other GetCommandTagName() would benefit noticably from
> not having to compute the length. I guess the calls
> set_ps_display(GetCommandTagName()) calls in exec_simple_query() and
> exec_execute_message() might, although set_ps_display() isn't exactly zero
> overhead. But I do see it show up as a few percent in profiles, with the
> biggest contributor being the call to strlen.

I think that could be improved for sure.  It does seem like we'd need
to add set_ps_display_with_len() to make what you said work.  There's
probably lower hanging fruit in that function that could be fixed
without having to do that, however.  For example:

strlcpy(ps_buffer + ps_buffer_fixed_size, activity,
    ps_buffer_size - ps_buffer_fixed_size);
ps_buffer_cur_len = strlen(ps_buffer);

could be written as:

strlcpy(ps_buffer + ps_buffer_fixed_size, activity,
    ps_buffer_size - ps_buffer_fixed_size);
ps_buffer_cur_len = ps_buffer_fixed_size + Min(strlen(activity),
ps_buffer_size - ps_buffer_fixed_size - 1);

That's pretty horrible to read though.

This sort of thing also makes me think that our investment in having
more usages of strlcpy() and fewer usages of strncpy was partially a
mistake. There are exactly 2 usages of the return value of strlcpy in
our entire source tree. That's about 1% of all calls.  Likely what
would be better is a function that returns the number of bytes
*actually* copied instead of one that returns the number of bytes that
it would have copied if it hadn't run out of space. Such a function
could be defined as:

size_t
strdcpy(char * const dst, const char *src, ptrdiff_t len)
{
    char *dstp = dst;

    while (len-- > 0)
    {
        if ((*dstp = *src++) == '\0')
        {
            *dstp = '\0';
            break;
        }
        dstp++;
    }
    return (dstp - dst);
}

Then we could append to strings like:

char buffer[STRING_SIZE];
char *bufp = buffer;

bufp += strdcpy(bufp, "01", STRING_SIZE - (bufp - buffer));
bufp += strdcpy(bufp, "23", STRING_SIZE - (bufp - buffer));
bufp += strdcpy(bufp, "45", STRING_SIZE - (bufp - buffer));

which allows transformation of the set_ps_display() code to:

pg_buffer_cur_len = ps_buffer_fixed_size;
pg_buffer_cur_len += strdcpy(ps_buffer + ps_buffer_fixed_size,
activity, ps_buffer_size - ps_buffer_fixed_size);

(Assume the name strdcpy as a placeholder name for an actual name that
does not conflict with something that it's not.)

I'd rather not go into too much detail about that here though. I don't
see any places that can make use of the known tag length without going
to the trouble of inventing new functions or changing the signature of
existing ones.

David