Thanks for having a look at this.
On Sun, 11 Dec 2022 at 11:05, Andres Freund <and...@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
From 20b386ca1794421ec0f2aaf81a08ab6f0ed2d9e2 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrow...@gmail.com>
Date: Sat, 10 Dec 2022 20:27:01 +1300
Subject: [PATCH v2] Speed up creation of command completion tags
Author: David Rowley, Andres Freund
---
src/backend/tcop/cmdtag.c | 73 +++++++++++++++++++++++++++++++++++++--
src/backend/tcop/dest.c | 29 +++-------------
src/include/tcop/cmdtag.h | 5 +++
src/include/tcop/dest.h | 2 --
4 files changed, 79 insertions(+), 30 deletions(-)
diff --git a/src/backend/tcop/cmdtag.c b/src/backend/tcop/cmdtag.c
index 262484f561..8e52ff084a 100644
--- a/src/backend/tcop/cmdtag.c
+++ b/src/backend/tcop/cmdtag.c
@@ -15,18 +15,22 @@
#include "miscadmin.h"
#include "tcop/cmdtag.h"
+#include "utils/builtins.h"
typedef struct CommandTagBehavior
{
- const char *name;
+ const char *name; /* tag name, e.g.
"SELECT" */
+ const uint8 namelen; /* set to strlen(name) */
const bool event_trigger_ok;
const bool table_rewrite_ok;
- const bool display_rowcount;
+ const bool display_rowcount; /* should the number of rows
affected be
+ *
shown in the command completion string
+ */
} CommandTagBehavior;
#define PG_CMDTAG(tag, name, evtrgok, rwrok, rowcnt) \
- { name, evtrgok, rwrok, rowcnt },
+ { name, (uint8) (sizeof(name) - 1), evtrgok, rwrok, rowcnt },
static const CommandTagBehavior tag_behavior[COMMAND_TAG_NEXTTAG] = {
#include "tcop/cmdtaglist.h"
@@ -47,6 +51,13 @@ GetCommandTagName(CommandTag commandTag)
return tag_behavior[commandTag].name;
}
+const char *
+GetCommandTagNameAndLen(CommandTag commandTag, Size *len)
+{
+ *len = (Size) tag_behavior[commandTag].namelen;
+ return tag_behavior[commandTag].name;
+}
+
bool
command_tag_display_rowcount(CommandTag commandTag)
{
@@ -96,3 +107,59 @@ GetCommandTagEnum(const char *commandname)
}
return CMDTAG_UNKNOWN;
}
+
+/*
+ * BuildQueryCompletionString
+ * Build a string containing the command tag name with the
+ * QueryCompletion's nprocessed for command tags with
display_rowcount
+ * set. Returns the strlen of the constructed string.
+ *
+ * The caller must ensure that buff is at least COMPLETION_TAG_BUFSIZE bytes.
+ *
+ * If nameonly is true, then the constructed string will contain only the tag
+ * name.
+ */
+Size
+BuildQueryCompletionString(char *buff, const QueryCompletion *qc,
+ bool nameonly)
+{
+ CommandTag tag = qc->commandTag;
+ Size taglen;
+ const char *tagname = GetCommandTagNameAndLen(tag, &taglen);
+ char *bufp;
+
+ /*
+ * We assume the tagname is plain ASCII and therefore requires no
encoding
+ * conversion.
+ */
+ memcpy(buff, tagname, taglen);
+ bufp = buff + taglen;
+
+ /* ensure that the tagname isn't long enough to overrun the buffer */
+ Assert(taglen <= COMPLETION_TAG_BUFSIZE - MAXINT8LEN - 4);
+
+ /*
+ * In PostgreSQL versions 11 and earlier, it was possible to create a
+ * table WITH OIDS. When inserting into such a table, INSERT used to
+ * include the Oid of the inserted record in the completion tag. To
+ * maintain compatibility in the wire protocol, we now write a "0" (for
+ * InvalidOid) in the location where we once wrote the new record's Oid.
+ */
+ if (command_tag_display_rowcount(tag) && !nameonly)
+ {
+ if (tag == CMDTAG_INSERT)
+ {
+ *bufp++ = ' ';
+ *bufp++ = '0';
+ }
+ *bufp++ = ' ';
+ bufp += pg_ulltoa_n(qc->nprocessed, bufp);
+ }
+
+ /* and finally, NUL terminate the string */
+ *bufp = '\0';
+
+ Assert((bufp - buff) == strlen(buff));
+
+ return bufp - buff;
+}
diff --git a/src/backend/tcop/dest.c b/src/backend/tcop/dest.c
index 6afaa153ca..e879304ae1 100644
--- a/src/backend/tcop/dest.c
+++ b/src/backend/tcop/dest.c
@@ -166,8 +166,7 @@ void
EndCommand(const QueryCompletion *qc, CommandDest dest, bool
force_undecorated_output)
{
char completionTag[COMPLETION_TAG_BUFSIZE];
- CommandTag tag;
- const char *tagname;
+ Size len;
switch (dest)
{
@@ -175,29 +174,9 @@ EndCommand(const QueryCompletion *qc, CommandDest dest,
bool force_undecorated_o
case DestRemoteExecute:
case DestRemoteSimple:
- /*
- * We assume the tagname is plain ASCII and therefore
requires no
- * encoding conversion.
- */
- tag = qc->commandTag;
- tagname = GetCommandTagName(tag);
-
- /*
- * In PostgreSQL versions 11 and earlier, it was
possible to
- * create a table WITH OIDS. When inserting into such
a table,
- * INSERT used to include the Oid of the inserted
record in the
- * completion tag. To maintain compatibility in the
wire
- * protocol, we now write a "0" (for InvalidOid) in the
location
- * where we once wrote the new record's Oid.
- */
- if (command_tag_display_rowcount(tag) &&
!force_undecorated_output)
- snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
- tag == CMDTAG_INSERT ?
- "%s 0 " UINT64_FORMAT : "%s "
UINT64_FORMAT,
- tagname, qc->nprocessed);
- else
- snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
"%s", tagname);
- pq_putmessage('C', completionTag, strlen(completionTag)
+ 1);
+ len = BuildQueryCompletionString(completionTag, qc,
+
force_undecorated_output);
+ pq_putmessage('C', completionTag, len + 1);
case DestNone:
case DestDebug:
diff --git a/src/include/tcop/cmdtag.h b/src/include/tcop/cmdtag.h
index 60e3179c74..a45d71d0cb 100644
--- a/src/include/tcop/cmdtag.h
+++ b/src/include/tcop/cmdtag.h
@@ -13,6 +13,8 @@
#ifndef CMDTAG_H
#define CMDTAG_H
+ /* buffer size required for command completion tags */
+#define COMPLETION_TAG_BUFSIZE 64
#define PG_CMDTAG(tag, name, evtrgok, rwrok, rowcnt) \
tag,
@@ -50,9 +52,12 @@ CopyQueryCompletion(QueryCompletion *dst, const
QueryCompletion *src)
extern void InitializeQueryCompletion(QueryCompletion *qc);
extern const char *GetCommandTagName(CommandTag commandTag);
+extern const char *GetCommandTagNameAndLen(CommandTag commandTag, Size *len);
extern bool command_tag_display_rowcount(CommandTag commandTag);
extern bool command_tag_event_trigger_ok(CommandTag commandTag);
extern bool command_tag_table_rewrite_ok(CommandTag commandTag);
extern CommandTag GetCommandTagEnum(const char *commandname);
+extern Size BuildQueryCompletionString(char *buff, const QueryCompletion *qc,
+ bool
nameonly);
#endif /* CMDTAG_H */
diff --git a/src/include/tcop/dest.h b/src/include/tcop/dest.h
index 3c3eabae67..83d1af88b4 100644
--- a/src/include/tcop/dest.h
+++ b/src/include/tcop/dest.h
@@ -71,8 +71,6 @@
#include "tcop/cmdtag.h"
-/* buffer size to use for command completion tags */
-#define COMPLETION_TAG_BUFSIZE 64
/* ----------------
--
2.35.1.windows.2
