Why is pq_begintypsend so slow?

Started by Jeff Janesabout 6 years ago29 messages
#1Jeff Janes
jeff.janes@gmail.com

I was using COPY recently and was wondering why BINARY format is not much
(if any) faster than the default format. Once I switched from mostly
exporting ints to mostly exporting double precisions (7e6 rows of 100
columns, randomly generated), it was faster, but not by as much as I
intuitively thought it should be.

Running 'perf top' to profile a "COPY BINARY .. TO '/dev/null'" on a AWS
m5.large machine running Ubuntu 18.04, with self compiled PostgreSQL:

PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
7.4.0-1ubuntu1~18.04.1) 7.4.0, 64-bit

I saw that the hotspot was pq_begintypsend at 20%, which was twice the
percentage as the next place winner (AllocSetAlloc). If I drill down into
teh function, I see something like the below. I don't really speak
assembly, but usually when I see an assembly instruction being especially
hot and not being the inner most instruction in a loop, I blame it on CPU
cache misses. But everything being touched here should already be well
cached, since initStringInfo has just got done setting it up. And if not
for that, then the by the 2nd invocation of appendStringInfoCharMacro it
certainly should be in the cache, yet that one is even slower than the 1st
appendStringInfoCharMacro.

Why is this such a bottleneck?

pq_begintypsend /usr/local/pgsql/bin/postgres

0.15 | push %rbx
0.09 | mov %rdi,%rbx
| initStringInfo(buf);
3.03 | callq initStringInfo
| /* Reserve four bytes for the bytea length word */
| appendStringInfoCharMacro(buf, '\0');
| movslq 0x8(%rbx),%rax
1.05 | lea 0x1(%rax),%edx
0.72 | cmp 0xc(%rbx),%edx
| jge b0
2.92 | mov (%rbx),%rdx
| movb $0x0,(%rdx,%rax,1)
13.76 | mov 0x8(%rbx),%eax
0.81 | mov (%rbx),%rdx
0.52 | add $0x1,%eax
0.12 | mov %eax,0x8(%rbx)
2.85 | cltq
0.01 | movb $0x0,(%rdx,%rax,1)
| appendStringInfoCharMacro(buf, '\0');
10.65 | movslq 0x8(%rbx),%rax
| lea 0x1(%rax),%edx
0.90 | cmp 0xc(%rbx),%edx
| jge ca
0.54 | 42: mov (%rbx),%rdx
1.84 | movb $0x0,(%rdx,%rax,1)
13.88 | mov 0x8(%rbx),%eax
0.03 | mov (%rbx),%rdx
| add $0x1,%eax
0.33 | mov %eax,0x8(%rbx)
2.60 | cltq
0.06 | movb $0x0,(%rdx,%rax,1)
| appendStringInfoCharMacro(buf, '\0');
3.21 | movslq 0x8(%rbx),%rax
0.23 | lea 0x1(%rax),%edx
1.74 | cmp 0xc(%rbx),%edx
| jge e0
0.21 | 67: mov (%rbx),%rdx
1.18 | movb $0x0,(%rdx,%rax,1)
9.29 | mov 0x8(%rbx),%eax
0.18 | mov (%rbx),%rdx
| add $0x1,%eax
0.19 | mov %eax,0x8(%rbx)
3.14 | cltq
0.12 | movb $0x0,(%rdx,%rax,1)
| appendStringInfoCharMacro(buf, '\0');
5.29 | movslq 0x8(%rbx),%rax
0.03 | lea 0x1(%rax),%edx
1.45 | cmp 0xc(%rbx),%edx
| jge f6
0.41 | 8c: mov (%rbx),%rdx

Cheers,

Jeff

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jeff Janes (#1)
Re: Why is pq_begintypsend so slow?

Jeff Janes <jeff.janes@gmail.com> writes:

I saw that the hotspot was pq_begintypsend at 20%, which was twice the
percentage as the next place winner (AllocSetAlloc).

Weird.

Why is this such a bottleneck?

Not sure, but it seems like a pretty dumb way to push the stringinfo's
len forward. We're reading/updating the len word in each line, and
if your perf measurements are to be believed, it's the re-fetches of
the len values that are bottlenecked --- maybe your CPU isn't too
bright about that? The bytes of the string value are getting written
twice too, thanks to uselessly setting up a terminating nul each time.

I'd be inclined to replace the appendStringInfoCharMacro calls with
appendStringInfoSpaces(buf, 4) --- I don't think we care exactly what
is inserted into those bytes at this point. And maybe
appendStringInfoSpaces could stand some micro-optimization, too.
Use a memset and a single len adjustment, perhaps?

regards, tom lane

#3David Fetter
david@fetter.org
In reply to: Tom Lane (#2)
1 attachment(s)
Re: Why is pq_begintypsend so slow?

On Sat, Jan 11, 2020 at 03:19:37PM -0500, Tom Lane wrote:

Jeff Janes <jeff.janes@gmail.com> writes:

I saw that the hotspot was pq_begintypsend at 20%, which was twice the
percentage as the next place winner (AllocSetAlloc).

Weird.

Why is this such a bottleneck?

Not sure, but it seems like a pretty dumb way to push the stringinfo's
len forward. We're reading/updating the len word in each line, and
if your perf measurements are to be believed, it's the re-fetches of
the len values that are bottlenecked --- maybe your CPU isn't too
bright about that? The bytes of the string value are getting written
twice too, thanks to uselessly setting up a terminating nul each time.

I'd be inclined to replace the appendStringInfoCharMacro calls with
appendStringInfoSpaces(buf, 4) --- I don't think we care exactly what
is inserted into those bytes at this point. And maybe
appendStringInfoSpaces could stand some micro-optimization, too.
Use a memset and a single len adjustment, perhaps?

Please find attached a patch that does it both of the things you
suggested.

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Attachments:

v1-0001-Make-pq_begintypsend-more-efficient.patchtext/x-diff; charset=us-asciiDownload
From 0c242cfd6e09d377f75febec0adaa705ff27c7f0 Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sat, 11 Jan 2020 17:38:12 -0800
Subject: [PATCH v1] Make pq_begintypsend more efficient
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.24.1"

This is a multi-part message in MIME format.
--------------2.24.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


by replacing 4 calls to appendStringInfoCharMacro with one call to
appendStringInfoSpaces.

In passing, make appendStringInfoSpaces more efficient by turning a
while() loop into a memset() + increment.

diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index a6f990c2d2..a4c6cdab80 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -329,10 +329,7 @@ pq_begintypsend(StringInfo buf)
 {
 	initStringInfo(buf);
 	/* Reserve four bytes for the bytea length word */
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
+	appendStringInfoSpaces(buf, 4);
 }
 
 /* --------------------------------
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 0badc46554..87074af476 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -211,8 +211,8 @@ appendStringInfoSpaces(StringInfo str, int count)
 		enlargeStringInfo(str, count);
 
 		/* OK, append the spaces */
-		while (--count >= 0)
-			str->data[str->len++] = ' ';
+		memset(&str->data[str->len], ' ', count);
+		str->len += count;
 		str->data[str->len] = '\0';
 	}
 }

--------------2.24.1--


#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: David Fetter (#3)
1 attachment(s)
Re: Why is pq_begintypsend so slow?

David Fetter <david@fetter.org> writes:

On Sat, Jan 11, 2020 at 03:19:37PM -0500, Tom Lane wrote:

Jeff Janes <jeff.janes@gmail.com> writes:

I saw that the hotspot was pq_begintypsend at 20%, which was twice the
percentage as the next place winner (AllocSetAlloc).

I'd be inclined to replace the appendStringInfoCharMacro calls with
appendStringInfoSpaces(buf, 4) --- I don't think we care exactly what
is inserted into those bytes at this point. And maybe
appendStringInfoSpaces could stand some micro-optimization, too.
Use a memset and a single len adjustment, perhaps?

Please find attached a patch that does it both of the things you
suggested.

I've been fooling around with this here. On the test case Jeff
describes --- COPY BINARY tab TO '/dev/null' where tab contains
100 float8 columns filled from random() --- I can reproduce his
results. pq_begintypsend is the top hotspot and if perf's
localization is accurate, it's the instructions that fetch
str->len that hurt the most. Still not very clear why that is...

Converting pq_begintypsend to use appendStringInfoSpaces helps
a bit; it takes my test case from 91725 ms to 88847 ms, or about
3% savings. Noodling around with appendStringInfoSpaces doesn't
help noticeably; I tried memset, as well as open-coding (cf
patch below) but the results were all well within the noise
threshold.

I saw at this point that the remaining top spots were
enlargeStringInfo and appendBinaryStringInfo, so I experimented
with inlining them (again, see patch below). That *did* move
the needle: down to 72691 ms, or 20% better than HEAD. Of
course, that comes at a code-size cost, but it's only about
13kB growth:

before:
$ size src/backend/postgres
text data bss dec hex filename
7485285 58088 203328 7746701 76348d src/backend/postgres
after:
$ size src/backend/postgres
text data bss dec hex filename
7498652 58088 203328 7760068 7668c4 src/backend/postgres

That's under two-tenths of a percent. (This'd affect frontend
executables too, and I didn't check them.)

Seems like this is worth pursuing, especially if it can be
shown to improve any other cases noticeably. It might be
worth inlining some of the other trivial stringinfo functions,
though I'd tread carefully on that.

regards, tom lane

Attachments:

microoptimize-some-stringinfo-stuff-1.patchtext/x-diff; charset=us-ascii; name=microoptimize-some-stringinfo-stuff-1.patchDownload
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index a6f990c..0fc8c3f 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -328,11 +328,8 @@ void
 pq_begintypsend(StringInfo buf)
 {
 	initStringInfo(buf);
-	/* Reserve four bytes for the bytea length word */
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
+	/* Reserve four bytes for the bytea length word; contents not important */
+	appendStringInfoSpaces(buf, 4);
 }
 
 /* --------------------------------
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 0badc46..8f8bb0d 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -207,43 +207,21 @@ appendStringInfoSpaces(StringInfo str, int count)
 {
 	if (count > 0)
 	{
+		char	   *ptr;
+
 		/* Make more room if needed */
 		enlargeStringInfo(str, count);
 
 		/* OK, append the spaces */
+		ptr = str->data + str->len;
+		str->len += count;
 		while (--count >= 0)
-			str->data[str->len++] = ' ';
-		str->data[str->len] = '\0';
+			*ptr++ = ' ';
+		*ptr = '\0';
 	}
 }
 
 /*
- * appendBinaryStringInfo
- *
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Ensures that a trailing null byte is present.
- */
-void
-appendBinaryStringInfo(StringInfo str, const char *data, int datalen)
-{
-	Assert(str != NULL);
-
-	/* Make more room if needed */
-	enlargeStringInfo(str, datalen);
-
-	/* OK, append the data */
-	memcpy(str->data + str->len, data, datalen);
-	str->len += datalen;
-
-	/*
-	 * Keep a trailing null in place, even though it's probably useless for
-	 * binary data.  (Some callers are dealing with text but call this because
-	 * their input isn't null-terminated.)
-	 */
-	str->data[str->len] = '\0';
-}
-
-/*
  * appendBinaryStringInfoNT
  *
  * Append arbitrary binary data to a StringInfo, allocating more space
@@ -263,7 +241,7 @@ appendBinaryStringInfoNT(StringInfo str, const char *data, int datalen)
 }
 
 /*
- * enlargeStringInfo
+ * enlargeStringInfo_OOL
  *
  * Make sure there is enough space for 'needed' more bytes
  * ('needed' does not include the terminating null).
@@ -274,13 +252,16 @@ appendBinaryStringInfoNT(StringInfo str, const char *data, int datalen)
  * can save some palloc overhead by enlarging the buffer before starting
  * to store data in it.
  *
+ * We normally reach here only if enlargement is needed, since callers
+ * go through the inline function which doesn't call this otherwise.
+ *
  * NB: In the backend, because we use repalloc() to enlarge the buffer, the
  * string buffer will remain allocated in the same memory context that was
  * current when initStringInfo was called, even if another context is now
  * current.  This is the desired and indeed critical behavior!
  */
 void
-enlargeStringInfo(StringInfo str, int needed)
+enlargeStringInfo_OOL(StringInfo str, int needed)
 {
 	int			newlen;
 
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 5a2a3db..30ba826 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -87,6 +87,25 @@ extern void initStringInfo(StringInfo str);
 extern void resetStringInfo(StringInfo str);
 
 /*------------------------
+ * enlargeStringInfo
+ * Make sure a StringInfo's buffer can hold at least 'needed' more bytes
+ * ('needed' does not include the terminating null).
+ * The inlined code eliminates the common case where no work is needed.
+ */
+extern void enlargeStringInfo_OOL(StringInfo str, int needed);
+
+static inline void
+enlargeStringInfo(StringInfo str, int needed)
+{
+	/*
+	 * Do the test in unsigned arithmetic so that if "needed" is negative,
+	 * we'll go to the out-of-line code which will error out.
+	 */
+	if ((unsigned) needed >= (unsigned) (str->maxlen - str->len))
+		enlargeStringInfo_OOL(str, needed);
+}
+
+/*------------------------
  * appendStringInfo
  * Format text data under the control of fmt (an sprintf-style format string)
  * and append it to whatever is already in str.  More space is allocated
@@ -139,10 +158,27 @@ extern void appendStringInfoSpaces(StringInfo str, int count);
 /*------------------------
  * appendBinaryStringInfo
  * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary.
+ * if necessary.  Ensures that a trailing null byte is present.
  */
-extern void appendBinaryStringInfo(StringInfo str,
-								   const char *data, int datalen);
+static inline void
+appendBinaryStringInfo(StringInfo str, const char *data, int datalen)
+{
+	Assert(str != NULL);
+
+	/* Make more room if needed */
+	enlargeStringInfo(str, datalen);
+
+	/* OK, append the data */
+	memcpy(str->data + str->len, data, datalen);
+	str->len += datalen;
+
+	/*
+	 * Keep a trailing null in place, even though it's probably useless for
+	 * binary data.  (Some callers are dealing with text but call this because
+	 * their input isn't null-terminated.)
+	 */
+	str->data[str->len] = '\0';
+}
 
 /*------------------------
  * appendBinaryStringInfoNT
@@ -152,10 +188,4 @@ extern void appendBinaryStringInfo(StringInfo str,
 extern void appendBinaryStringInfoNT(StringInfo str,
 									 const char *data, int datalen);
 
-/*------------------------
- * enlargeStringInfo
- * Make sure a StringInfo's buffer can hold at least 'needed' more bytes.
- */
-extern void enlargeStringInfo(StringInfo str, int needed);
-
 #endif							/* STRINGINFO_H */
#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#4)
Re: Why is pq_begintypsend so slow?

I wrote:

I saw at this point that the remaining top spots were
enlargeStringInfo and appendBinaryStringInfo, so I experimented
with inlining them (again, see patch below). That *did* move
the needle: down to 72691 ms, or 20% better than HEAD.

Oh ... marking the test in the inline part of enlargeStringInfo()
as unlikely() helps quite a bit more: 66100 ms, a further 9% gain.
Might be over-optimizing for this particular case, perhaps, but
I think that's a reasonable marking given that we overallocate
the stringinfo buffer for most uses.

(But ... I'm not finding these numbers to be super reproducible
across different ASLR layouts. So take it with a grain of salt.)

regards, tom lane

#6Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#5)
Re: Why is pq_begintypsend so slow?

Hi,

On 2020-01-11 22:32:45 -0500, Tom Lane wrote:

I wrote:

I saw at this point that the remaining top spots were
enlargeStringInfo and appendBinaryStringInfo, so I experimented
with inlining them (again, see patch below). That *did* move
the needle: down to 72691 ms, or 20% better than HEAD.

Oh ... marking the test in the inline part of enlargeStringInfo()
as unlikely() helps quite a bit more: 66100 ms, a further 9% gain.
Might be over-optimizing for this particular case, perhaps

(But ... I'm not finding these numbers to be super reproducible
across different ASLR layouts. So take it with a grain of salt.)

FWIW, I've also observed, in another thread (the node func generation
thing [1]https://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=commit;h=127e860cf65f50434e0bb97acbba4b0ea6f38cfd), that inlining enlargeStringInfo() helps a lot, especially
when inlining some of its callers. Moving e.g. appendStringInfo() inline
allows the compiler to sometimes optimize away the strlen. But if
e.g. an inlined appendBinaryStringInfo() still calls enlargeStringInfo()
unconditionally, successive appends cannot optimize away memory accesses
for ->len/->data.

For the case of send functions, we really ought to have at least
pq_begintypsend(), enlargeStringInfo() and pq_endtypsend() inline. That
way the compiler ought to be able to avoid repeatedly loading/storing
->len, after the initial initStringInfo() call. Might even make sense to
also have initStringInfo() inline, because then the compiler would
probably never actually materialize the StringInfoData (and would
automatically have good aliasing information too).

The commit referenced above is obviously quite WIP-ey, and contains
things that should be split into separate commits. But I think it might
be worth moving more functions into the header, like I've done in that
commit.

The commit also adds appendStringInfoU?Int(32,64) operations - I've
unsuprisingly found these to be *considerably* faster than going through
appendStringInfoString().

but I think that's a reasonable marking given that we overallocate
the stringinfo buffer for most uses.

Wonder if it's worth providing a function to initialize the stringinfo
differently for the many cases where we have at least a very good idea
of how long the string will be. It's sad to allocate 1kb just for
e.g. int4send to send an integer plus length.

Greetings,

Andres Freund

[1]: https://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=commit;h=127e860cf65f50434e0bb97acbba4b0ea6f38cfd

#7Andres Freund
andres@anarazel.de
In reply to: Andres Freund (#6)
6 attachment(s)
Re: Why is pq_begintypsend so slow?

Hi,

On 2020-01-13 15:18:04 -0800, Andres Freund wrote:

On 2020-01-11 22:32:45 -0500, Tom Lane wrote:

I wrote:

I saw at this point that the remaining top spots were
enlargeStringInfo and appendBinaryStringInfo, so I experimented
with inlining them (again, see patch below). That *did* move
the needle: down to 72691 ms, or 20% better than HEAD.

Oh ... marking the test in the inline part of enlargeStringInfo()
as unlikely() helps quite a bit more: 66100 ms, a further 9% gain.
Might be over-optimizing for this particular case, perhaps

(But ... I'm not finding these numbers to be super reproducible
across different ASLR layouts. So take it with a grain of salt.)

FWIW, I've also observed, in another thread (the node func generation
thing [1]), that inlining enlargeStringInfo() helps a lot, especially
when inlining some of its callers. Moving e.g. appendStringInfo() inline
allows the compiler to sometimes optimize away the strlen. But if
e.g. an inlined appendBinaryStringInfo() still calls enlargeStringInfo()
unconditionally, successive appends cannot optimize away memory accesses
for ->len/->data.

With a set of patches doing so, int4send itself is not a significant
factor for my test benchmark [1]CREATE TABLE lotsaints4(c01 int4 NOT NULL, c02 int4 NOT NULL, c03 int4 NOT NULL, c04 int4 NOT NULL, c05 int4 NOT NULL, c06 int4 NOT NULL, c07 int4 NOT NULL, c08 int4 NOT NULL, c09 int4 NOT NULL, c10 int4 NOT NULL); INSERT INTO lotsaints4 SELECT ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int FROM generate_series(1, 2000000); VACUUM FREEZE lotsaints4; COPY lotsaints4 TO '/dev/null' WITH binary; anymore. The assembly looks about as
good as one could hope, I think:

# save rbx on the stack
0x00000000004b7f90 <+0>: push %rbx
0x00000000004b7f91 <+1>: sub $0x20,%rsp
# store integer to be sent into rbx
0x00000000004b7f95 <+5>: mov 0x20(%rdi),%rbx
# palloc length argument
0x00000000004b7f99 <+9>: mov $0x9,%edi
0x00000000004b7f9e <+14>: callq 0x5d9aa0 <palloc>
# store integer in buffer (ebx is 4 byte portion of rbx)
0x00000000004b7fa3 <+19>: movbe %ebx,0x4(%rax)
# store varlena header
0x00000000004b7fa8 <+24>: movl $0x20,(%rax)
# restore stack and rbx registerz
0x00000000004b7fae <+30>: add $0x20,%rsp
0x00000000004b7fb2 <+34>: pop %rbx
0x00000000004b7fb3 <+35>: retq

All the $0x20 constants are a bit confusing, but they just happen to be
the same for int4send. It's the size of the stack frame,
offset for FunctionCallInfoBaseData->args[0], the varlena header (and then the stack
frame again) respectively.

Note that I had to annotate palloc with __attribute__((malloc)) to make
the compiler understand that palloc's returned value will not alias with
anything problematic (e.g. the potential of aliasing with fcinfo
prevents optimizing to the above without that annotation). I think such
annotations would be a good idea anyway, precisely because they allow
the compiler to optimize code significantly better.

These together yields about a 1.8x speedup for me. The profile shows
that the overhead now is overwhelmingly elsewhere:
+   26.30%  postgres  postgres          [.] CopyOneRowTo
+   13.40%  postgres  postgres          [.] tts_buffer_heap_getsomeattrs
+   10.61%  postgres  postgres          [.] AllocSetAlloc
+    9.26%  postgres  libc-2.29.so      [.] __memmove_avx_unaligned_erms
+    7.32%  postgres  postgres          [.] SendFunctionCall
+    6.02%  postgres  postgres          [.] palloc
+    4.45%  postgres  postgres          [.] int4send
+    3.68%  postgres  libc-2.29.so      [.] _IO_fwrite
+    2.71%  postgres  postgres          [.] heapgettup_pagemode
+    1.96%  postgres  postgres          [.] AllocSetReset
+    1.83%  postgres  postgres          [.] CopySendEndOfRow
+    1.75%  postgres  libc-2.29.so      [.] _IO_file_xsputn@@GLIBC_2.2.5
+    1.60%  postgres  postgres          [.] ExecStoreBufferHeapTuple
+    1.57%  postgres  postgres          [.] DoCopyTo
+    1.16%  postgres  postgres          [.] memcpy@plt
+    1.07%  postgres  postgres          [.] heapgetpage
Even without using the new pq_begintypesend_ex()/initStringInfoEx(), the
generated code is still considerably better than before, yielding a
1.58x speedup. Tallocator overhead unsurprisingly is higher:
+   24.93%  postgres  postgres          [.] CopyOneRowTo
+   17.10%  postgres  postgres          [.] AllocSetAlloc
+   10.09%  postgres  postgres          [.] tts_buffer_heap_getsomeattrs
+    6.50%  postgres  libc-2.29.so      [.] __memmove_avx_unaligned_erms
+    5.99%  postgres  postgres          [.] SendFunctionCall
+    5.11%  postgres  postgres          [.] palloc
+    3.95%  postgres  libc-2.29.so      [.] _int_malloc
+    3.38%  postgres  postgres          [.] int4send
+    2.54%  postgres  postgres          [.] heapgettup_pagemode
+    2.11%  postgres  libc-2.29.so      [.] _int_free
+    2.06%  postgres  postgres          [.] MemoryContextReset
+    2.02%  postgres  postgres          [.] AllocSetReset
+    1.97%  postgres  libc-2.29.so      [.] _IO_fwrite
+    1.47%  postgres  postgres          [.] DoCopyTo
+    1.14%  postgres  postgres          [.] ExecStoreBufferHeapTuple
+    1.06%  postgres  libc-2.29.so      [.] _IO_file_xsputn@@GLIBC_2.2.5
+    1.04%  postgres  libc-2.29.so      [.] malloc

Adding a few pg_restrict*, and using appendBinaryStringInfoNT instead of
appendBinaryStringInfo in CopySend* gains another 1.05x.

This does result in some code growth, but given the size of the
improvements, and that the improvements are significant even without
code changes to callsites, that seems worth it.

before:
text data bss dec hex filename
8482739 172304 204240 8859283 872e93 src/backend/postgres
after:
text data bss dec hex filename
8604300 172304 204240 8980844 89096c src/backend/postgres

Regards,

Andres

[1]: CREATE TABLE lotsaints4(c01 int4 NOT NULL, c02 int4 NOT NULL, c03 int4 NOT NULL, c04 int4 NOT NULL, c05 int4 NOT NULL, c06 int4 NOT NULL, c07 int4 NOT NULL, c08 int4 NOT NULL, c09 int4 NOT NULL, c10 int4 NOT NULL); INSERT INTO lotsaints4 SELECT ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int FROM generate_series(1, 2000000); VACUUM FREEZE lotsaints4; COPY lotsaints4 TO '/dev/null' WITH binary;
CREATE TABLE lotsaints4(c01 int4 NOT NULL, c02 int4 NOT NULL, c03 int4 NOT NULL, c04 int4 NOT NULL, c05 int4 NOT NULL, c06 int4 NOT NULL, c07 int4 NOT NULL, c08 int4 NOT NULL, c09 int4 NOT NULL, c10 int4 NOT NULL);
INSERT INTO lotsaints4 SELECT ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int FROM generate_series(1, 2000000);
VACUUM FREEZE lotsaints4;
COPY lotsaints4 TO '/dev/null' WITH binary;

CREATE TABLE lotsaints8(c01 int8 NOT NULL, c02 int8 NOT NULL, c03 int8 NOT NULL, c04 int8 NOT NULL, c05 int8 NOT NULL, c06 int8 NOT NULL, c07 int8 NOT NULL, c08 int8 NOT NULL, c09 int8 NOT NULL, c10 int8 NOT NULL);
INSERT INTO lotsaints8 SELECT ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int, ((random() * 2^31) - 1)::int FROM generate_series(1, 2000000);
VACUUM FREEZE lotsaints8;
COPY lotsaints8 TO '/dev/null' WITH binary;

Attachments:

v1-0001-stringinfo-Move-more-functions-inline-provide-ini.patchtext/x-diff; charset=us-asciiDownload
From cd3f74953d7b343dfd6e68aa687cc2c0d0bd5bc0 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:46:20 -0800
Subject: [PATCH v1 1/6] stringinfo: Move more functions inline, provide
 initStringInfoEx().

By moving the whole lifecycle of a stringinfo into inline functions,
a good compiler sometimes can be able to optimize away the existence
of the StringInfoData itself.

initStringInfoEx() allows stringinfo users to determine the initial
allocation size, avoiding over/under allocation when known.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/lib/stringinfo.h | 205 +++++++++++++++++++++++++++--------
 src/common/stringinfo.c      | 173 ++---------------------------
 2 files changed, 171 insertions(+), 207 deletions(-)

diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 5a2a3dbfbc0..38211323d9d 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -18,6 +18,9 @@
 #ifndef STRINGINFO_H
 #define STRINGINFO_H
 
+#include "common/int.h"
+#include "common/string.h"
+
 /*-------------------------
  * StringInfoData holds information about an extensible string.
  *		data	is the current buffer for the string (allocated with palloc).
@@ -66,25 +69,137 @@ typedef StringInfoData *StringInfo;
  *-------------------------
  */
 
-/*------------------------
- * makeStringInfo
- * Create an empty 'StringInfoData' & return a pointer to it.
- */
-extern StringInfo makeStringInfo(void);
-
-/*------------------------
- * initStringInfo
- * Initialize a StringInfoData struct (with previously undefined contents)
- * to describe an empty string.
- */
-extern void initStringInfo(StringInfo str);
 
 /*------------------------
  * resetStringInfo
  * Clears the current content of the StringInfo, if any. The
  * StringInfo remains valid.
  */
-extern void resetStringInfo(StringInfo str);
+static inline void
+resetStringInfo(StringInfoData *pg_restrict str)
+{
+	*(char *pg_restrict) (str->data) = '\0';
+	str->len = 0;
+	str->cursor = 0;
+}
+
+/*------------------------
+ * initStringInfo
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ */
+static inline void
+initStringInfo(StringInfoData *pg_restrict str)
+{
+	int			size = 1024;	/* initial default buffer size */
+
+	str->data = (char *) palloc(size);
+	str->maxlen = size;
+	resetStringInfo(str);
+}
+
+/*------------------------
+ * initStringInfoEx
+ *
+ * Like initStringInfo(), but allows to specify the size of the initial
+ * allocation.
+ */
+static inline void
+initStringInfoEx(StringInfoData *pg_restrict str, int size)
+{
+	/*
+	 * FIXME: Adding 1 to account for always present trailing \0 byte. Should
+	 * that instead be done at the caller level? Should we round up?
+	 */
+	str->data = (char *) palloc(size + 1);
+	str->maxlen = size + 1;
+	resetStringInfo(str);
+}
+
+/*------------------------
+ * makeStringInfo
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ */
+static inline StringInfo
+makeStringInfo(void)
+{
+	StringInfo	res;
+
+	res = (StringInfo) palloc(sizeof(StringInfoData));
+
+	initStringInfo(res);
+
+	return res;
+}
+
+/*------------------------
+ * enlargeStringInfoImpl
+ *
+ * Actually enlarge the string, only to be called by enlargeStringInfo().
+ */
+extern void enlargeStringInfoImpl(StringInfo str, int needed);
+
+/*------------------------
+ * enlargeStringInfo
+ * Make sure a StringInfo's buffer can hold at least 'needed' more bytes.
+ *
+ * External callers usually need not concern themselves with this, since
+ * all stringinfo.c routines do it automatically.  However, if a caller
+ * knows that a StringInfo will eventually become X bytes large, it
+ * can save some palloc overhead by enlarging the buffer before starting
+ * to store data in it.
+ *
+ * NB: In the backend, because we use repalloc() to enlarge the buffer, the
+ * string buffer will remain allocated in the same memory context that was
+ * current when initStringInfo was called, even if another context is now
+ * current.  This is the desired and indeed critical behavior!
+ */
+static inline void
+enlargeStringInfo(StringInfoData *pg_restrict str, int datalen)
+{
+	int res;
+
+	if (unlikely(pg_add_s32_overflow(str->len, datalen, &res)) ||
+		unlikely(res >= str->maxlen))
+		enlargeStringInfoImpl(str, datalen);
+}
+
+/*------------------------
+ * appendBinaryStringInfoNT
+ * Append arbitrary binary data to a StringInfo, allocating more space
+ * if necessary. Does not ensure a trailing null-byte exists.
+ */
+static inline void
+appendBinaryStringInfoNT(StringInfoData *pg_restrict str, const char *pg_restrict data, int datalen)
+{
+	Assert(str != NULL);
+
+	/* Make more room if needed */
+	enlargeStringInfo(str, datalen);
+
+	/* OK, append the data */
+	memcpy((char *pg_restrict) (str->data + str->len), data, datalen);
+	str->len += datalen;
+}
+
+/*------------------------
+ * appendBinaryStringInfo
+ * Append arbitrary binary data to a StringInfo, allocating more space
+ * if necessary. Ensures that a trailing null byte is present.
+ */
+static inline void
+appendBinaryStringInfo(StringInfoData *pg_restrict str, const char *pg_restrict data, int datalen)
+{
+	appendBinaryStringInfoNT(str, data, datalen);
+
+	/*
+	 * Keep a trailing null in place, even though it's probably useless for
+	 * binary data.  (Some callers are dealing with text but call this because
+	 * their input isn't null-terminated.)
+	 */
+	*(char *pg_restrict) (str->data + str->len) = '\0';
+}
+
 
 /*------------------------
  * appendStringInfo
@@ -111,51 +226,49 @@ extern int	appendStringInfoVA(StringInfo str, const char *fmt, va_list args) pg_
  * Append a null-terminated string to str.
  * Like appendStringInfo(str, "%s", s) but faster.
  */
-extern void appendStringInfoString(StringInfo str, const char *s);
+static inline void
+appendStringInfoString(StringInfoData *pg_restrict str, const char *pg_restrict s)
+{
+	appendBinaryStringInfo(str, s, strlen(s));
+}
 
 /*------------------------
  * appendStringInfoChar
  * Append a single byte to str.
  * Like appendStringInfo(str, "%c", ch) but much faster.
  */
-extern void appendStringInfoChar(StringInfo str, char ch);
+static inline void
+appendStringInfoChar(StringInfoData *pg_restrict str, char ch)
+{
+	/* Make more room if needed */
+	enlargeStringInfo(str, 1);
 
-/*------------------------
- * appendStringInfoCharMacro
- * As above, but a macro for even more speed where it matters.
- * Caution: str argument will be evaluated multiple times.
- */
-#define appendStringInfoCharMacro(str,ch) \
-	(((str)->len + 1 >= (str)->maxlen) ? \
-	 appendStringInfoChar(str, ch) : \
-	 (void)((str)->data[(str)->len] = (ch), (str)->data[++(str)->len] = '\0'))
+	/* OK, append the character */
+	*(char *pg_restrict) (str->data + str->len) = ch;
+	str->len++;
+	*(char *pg_restrict) (str->data + str->len) = '\0';
+}
+
+/* backward compat for external code */
+#define appendStringInfoCharMacro appendStringInfoChar
 
 /*------------------------
  * appendStringInfoSpaces
  * Append a given number of spaces to str.
  */
-extern void appendStringInfoSpaces(StringInfo str, int count);
+static inline void
+appendStringInfoSpaces(StringInfoData *pg_restrict str, int count)
+{
+	if (count > 0)
+	{
+		/* Make more room if needed */
+		enlargeStringInfo(str, count);
 
-/*------------------------
- * appendBinaryStringInfo
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary.
- */
-extern void appendBinaryStringInfo(StringInfo str,
-								   const char *data, int datalen);
-
-/*------------------------
- * appendBinaryStringInfoNT
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Does not ensure a trailing null-byte exists.
- */
-extern void appendBinaryStringInfoNT(StringInfo str,
-									 const char *data, int datalen);
-
-/*------------------------
- * enlargeStringInfo
- * Make sure a StringInfo's buffer can hold at least 'needed' more bytes.
- */
-extern void enlargeStringInfo(StringInfo str, int needed);
+		/* OK, append the spaces */
+		memset((char *pg_restrict) (str->data + str->len), ' ', count);
+		str->len += count;
+		*(char *pg_restrict) (str->data + str->len) = '\0';
+	}
+}
 
 #endif							/* STRINGINFO_H */
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 0badc465545..0ea5e0abe56 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -32,53 +32,6 @@
 #include "lib/stringinfo.h"
 
 
-/*
- * makeStringInfo
- *
- * Create an empty 'StringInfoData' & return a pointer to it.
- */
-StringInfo
-makeStringInfo(void)
-{
-	StringInfo	res;
-
-	res = (StringInfo) palloc(sizeof(StringInfoData));
-
-	initStringInfo(res);
-
-	return res;
-}
-
-/*
- * initStringInfo
- *
- * Initialize a StringInfoData struct (with previously undefined contents)
- * to describe an empty string.
- */
-void
-initStringInfo(StringInfo str)
-{
-	int			size = 1024;	/* initial default buffer size */
-
-	str->data = (char *) palloc(size);
-	str->maxlen = size;
-	resetStringInfo(str);
-}
-
-/*
- * resetStringInfo
- *
- * Reset the StringInfo: the data buffer remains valid, but its
- * previous content, if any, is cleared.
- */
-void
-resetStringInfo(StringInfo str)
-{
-	str->data[0] = '\0';
-	str->len = 0;
-	str->cursor = 0;
-}
-
 /*
  * appendStringInfo
  *
@@ -167,120 +120,18 @@ appendStringInfoVA(StringInfo str, const char *fmt, va_list args)
 }
 
 /*
- * appendStringInfoString
+ * enlargeStringInfoImpl
  *
- * Append a null-terminated string to str.
- * Like appendStringInfo(str, "%s", s) but faster.
+ * Make enough space for 'needed' more bytes ('needed' does not include the
+ * terminating null). This is not for external consumption, it's only to be
+ * called by enlargeStringInfo() when more space is actually needed (including
+ * when we'd overflow the maximum size).
+ *
+ * As this normally shouldn't be the common case, mark as noinline, to avoid
+ * including the function into the fastpath.
  */
-void
-appendStringInfoString(StringInfo str, const char *s)
-{
-	appendBinaryStringInfo(str, s, strlen(s));
-}
-
-/*
- * appendStringInfoChar
- *
- * Append a single byte to str.
- * Like appendStringInfo(str, "%c", ch) but much faster.
- */
-void
-appendStringInfoChar(StringInfo str, char ch)
-{
-	/* Make more room if needed */
-	if (str->len + 1 >= str->maxlen)
-		enlargeStringInfo(str, 1);
-
-	/* OK, append the character */
-	str->data[str->len] = ch;
-	str->len++;
-	str->data[str->len] = '\0';
-}
-
-/*
- * appendStringInfoSpaces
- *
- * Append the specified number of spaces to a buffer.
- */
-void
-appendStringInfoSpaces(StringInfo str, int count)
-{
-	if (count > 0)
-	{
-		/* Make more room if needed */
-		enlargeStringInfo(str, count);
-
-		/* OK, append the spaces */
-		while (--count >= 0)
-			str->data[str->len++] = ' ';
-		str->data[str->len] = '\0';
-	}
-}
-
-/*
- * appendBinaryStringInfo
- *
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Ensures that a trailing null byte is present.
- */
-void
-appendBinaryStringInfo(StringInfo str, const char *data, int datalen)
-{
-	Assert(str != NULL);
-
-	/* Make more room if needed */
-	enlargeStringInfo(str, datalen);
-
-	/* OK, append the data */
-	memcpy(str->data + str->len, data, datalen);
-	str->len += datalen;
-
-	/*
-	 * Keep a trailing null in place, even though it's probably useless for
-	 * binary data.  (Some callers are dealing with text but call this because
-	 * their input isn't null-terminated.)
-	 */
-	str->data[str->len] = '\0';
-}
-
-/*
- * appendBinaryStringInfoNT
- *
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Does not ensure a trailing null-byte exists.
- */
-void
-appendBinaryStringInfoNT(StringInfo str, const char *data, int datalen)
-{
-	Assert(str != NULL);
-
-	/* Make more room if needed */
-	enlargeStringInfo(str, datalen);
-
-	/* OK, append the data */
-	memcpy(str->data + str->len, data, datalen);
-	str->len += datalen;
-}
-
-/*
- * enlargeStringInfo
- *
- * Make sure there is enough space for 'needed' more bytes
- * ('needed' does not include the terminating null).
- *
- * External callers usually need not concern themselves with this, since
- * all stringinfo.c routines do it automatically.  However, if a caller
- * knows that a StringInfo will eventually become X bytes large, it
- * can save some palloc overhead by enlarging the buffer before starting
- * to store data in it.
- *
- * NB: In the backend, because we use repalloc() to enlarge the buffer, the
- * string buffer will remain allocated in the same memory context that was
- * current when initStringInfo was called, even if another context is now
- * current.  This is the desired and indeed critical behavior!
- */
-void
-enlargeStringInfo(StringInfo str, int needed)
+pg_noinline void
+enlargeStringInfoImpl(StringInfo str, int needed)
 {
 	int			newlen;
 
@@ -317,8 +168,8 @@ enlargeStringInfo(StringInfo str, int needed)
 
 	/* Because of the above test, we now have needed <= MaxAllocSize */
 
-	if (needed <= str->maxlen)
-		return;					/* got enough space already */
+	/* should only be called when needed */
+	Assert(needed > str->maxlen);
 
 	/*
 	 * We don't want to allocate just a little more space with each append;
-- 
2.25.0.rc1

v1-0002-stringinfo-Remove-in-core-use-of-appendStringInfo.patchtext/x-diff; charset=us-asciiDownload
From b71048bb26b561e9c5f022219c672705f130812f Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:58:00 -0800
Subject: [PATCH v1 2/6] stringinfo: Remove in-core use of
 appendStringInfoCharMacro().

Kept seperate only to reduce size of patch moving to more inlining for
stringinfo.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/commands/copy.c          |  2 +-
 src/backend/commands/explain.c       |  8 ++++----
 src/backend/libpq/pqformat.c         |  2 +-
 src/backend/utils/adt/json.c         |  6 +++---
 src/backend/utils/adt/jsonb.c        | 10 +++++-----
 src/backend/utils/adt/jsonfuncs.c    | 28 ++++++++++++++--------------
 src/backend/utils/adt/jsonpath.c     |  6 +++---
 src/backend/utils/adt/rowtypes.c     |  8 ++++----
 src/backend/utils/adt/varlena.c      |  4 ++--
 src/backend/utils/adt/xml.c          |  2 +-
 src/backend/utils/error/elog.c       | 12 ++++++------
 src/backend/utils/mb/stringinfo_mb.c |  2 +-
 contrib/tcn/tcn.c                    | 16 ++++++++--------
 13 files changed, 53 insertions(+), 53 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c93a788798d..88df90deb5b 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -510,7 +510,7 @@ CopySendString(CopyState cstate, const char *str)
 static void
 CopySendChar(CopyState cstate, char c)
 {
-	appendStringInfoCharMacro(cstate->fe_msgbuf, c);
+	appendStringInfoChar(cstate->fe_msgbuf, c);
 }
 
 static void
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d189b8d573a..a603763579b 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3900,16 +3900,16 @@ ExplainXMLTag(const char *tagname, int flags, ExplainState *es)
 
 	if ((flags & X_NOWHITESPACE) == 0)
 		appendStringInfoSpaces(es->str, 2 * es->indent);
-	appendStringInfoCharMacro(es->str, '<');
+	appendStringInfoChar(es->str, '<');
 	if ((flags & X_CLOSING) != 0)
-		appendStringInfoCharMacro(es->str, '/');
+		appendStringInfoChar(es->str, '/');
 	for (s = tagname; *s; s++)
 		appendStringInfoChar(es->str, strchr(valid, *s) ? *s : '-');
 	if ((flags & X_CLOSE_IMMEDIATE) != 0)
 		appendStringInfoString(es->str, " /");
-	appendStringInfoCharMacro(es->str, '>');
+	appendStringInfoChar(es->str, '>');
 	if ((flags & X_NOWHITESPACE) == 0)
-		appendStringInfoCharMacro(es->str, '\n');
+		appendStringInfoChar(es->str, '\n');
 }
 
 /*
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index a6f990c2d29..999252681c2 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -234,7 +234,7 @@ pq_send_ascii_string(StringInfo buf, const char *str)
 
 		if (IS_HIGHBIT_SET(ch))
 			ch = '?';
-		appendStringInfoCharMacro(buf, ch);
+		appendStringInfoChar(buf, ch);
 	}
 	appendStringInfoChar(buf, '\0');
 }
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 458505abfd8..1cdf75345cd 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -2484,7 +2484,7 @@ escape_json(StringInfo buf, const char *str)
 {
 	const char *p;
 
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 	for (p = str; *p; p++)
 	{
 		switch (*p)
@@ -2514,11 +2514,11 @@ escape_json(StringInfo buf, const char *str)
 				if ((unsigned char) *p < ' ')
 					appendStringInfo(buf, "\\u%04x", (int) *p);
 				else
-					appendStringInfoCharMacro(buf, *p);
+					appendStringInfoChar(buf, *p);
 				break;
 		}
 	}
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 }
 
 /*
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index c4a4ec78b0e..3234df61ef3 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -515,7 +515,7 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 				if (!v.val.array.rawScalar)
 				{
 					add_indent(out, use_indent && !last_was_key, level);
-					appendStringInfoCharMacro(out, '[');
+					appendStringInfoChar(out, '[');
 				}
 				else
 					raw_scalar = true;
@@ -528,7 +528,7 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 					appendBinaryStringInfo(out, ", ", ispaces);
 
 				add_indent(out, use_indent && !last_was_key, level);
-				appendStringInfoCharMacro(out, '{');
+				appendStringInfoChar(out, '{');
 
 				first = true;
 				level++;
@@ -576,14 +576,14 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 				if (!raw_scalar)
 				{
 					add_indent(out, use_indent, level);
-					appendStringInfoCharMacro(out, ']');
+					appendStringInfoChar(out, ']');
 				}
 				first = false;
 				break;
 			case WJB_END_OBJECT:
 				level--;
 				add_indent(out, use_indent, level);
-				appendStringInfoCharMacro(out, '}');
+				appendStringInfoChar(out, '}');
 				first = false;
 				break;
 			default:
@@ -605,7 +605,7 @@ add_indent(StringInfo out, bool indent, int level)
 	{
 		int			i;
 
-		appendStringInfoCharMacro(out, '\n');
+		appendStringInfoChar(out, '\n');
 		for (i = 0; i < level; i++)
 			appendBinaryStringInfo(out, "    ", 4);
 	}
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index ab5a24a8584..95cbbbe1311 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -3860,7 +3860,7 @@ sn_object_start(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '{');
+	appendStringInfoChar(_state->strval, '{');
 }
 
 static void
@@ -3868,7 +3868,7 @@ sn_object_end(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '}');
+	appendStringInfoChar(_state->strval, '}');
 }
 
 static void
@@ -3876,7 +3876,7 @@ sn_array_start(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '[');
+	appendStringInfoChar(_state->strval, '[');
 }
 
 static void
@@ -3884,7 +3884,7 @@ sn_array_end(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, ']');
+	appendStringInfoChar(_state->strval, ']');
 }
 
 static void
@@ -3904,7 +3904,7 @@ sn_object_field_start(void *state, char *fname, bool isnull)
 	}
 
 	if (_state->strval->data[_state->strval->len - 1] != '{')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 
 	/*
 	 * Unfortunately we don't have the quoted and escaped string any more, so
@@ -3912,7 +3912,7 @@ sn_object_field_start(void *state, char *fname, bool isnull)
 	 */
 	escape_json(_state->strval, fname);
 
-	appendStringInfoCharMacro(_state->strval, ':');
+	appendStringInfoChar(_state->strval, ':');
 }
 
 static void
@@ -3921,7 +3921,7 @@ sn_array_element_start(void *state, bool isnull)
 	StripnullState *_state = (StripnullState *) state;
 
 	if (_state->strval->data[_state->strval->len - 1] != '[')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 }
 
 static void
@@ -5178,7 +5178,7 @@ transform_string_values_object_start(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '{');
+	appendStringInfoChar(_state->strval, '{');
 }
 
 static void
@@ -5186,7 +5186,7 @@ transform_string_values_object_end(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '}');
+	appendStringInfoChar(_state->strval, '}');
 }
 
 static void
@@ -5194,7 +5194,7 @@ transform_string_values_array_start(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '[');
+	appendStringInfoChar(_state->strval, '[');
 }
 
 static void
@@ -5202,7 +5202,7 @@ transform_string_values_array_end(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, ']');
+	appendStringInfoChar(_state->strval, ']');
 }
 
 static void
@@ -5211,14 +5211,14 @@ transform_string_values_object_field_start(void *state, char *fname, bool isnull
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
 	if (_state->strval->data[_state->strval->len - 1] != '{')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 
 	/*
 	 * Unfortunately we don't have the quoted and escaped string any more, so
 	 * we have to re-escape it.
 	 */
 	escape_json(_state->strval, fname);
-	appendStringInfoCharMacro(_state->strval, ':');
+	appendStringInfoChar(_state->strval, ':');
 }
 
 static void
@@ -5227,7 +5227,7 @@ transform_string_values_array_element_start(void *state, bool isnull)
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
 	if (_state->strval->data[_state->strval->len - 1] != '[')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 }
 
 static void
diff --git a/src/backend/utils/adt/jsonpath.c b/src/backend/utils/adt/jsonpath.c
index 3c0dc38a7f8..43d5914477a 100644
--- a/src/backend/utils/adt/jsonpath.c
+++ b/src/backend/utils/adt/jsonpath.c
@@ -441,13 +441,13 @@ alignStringInfoInt(StringInfo buf)
 	switch (INTALIGN(buf->len) - buf->len)
 	{
 		case 3:
-			appendStringInfoCharMacro(buf, 0);
+			appendStringInfoChar(buf, 0);
 			/* FALLTHROUGH */
 		case 2:
-			appendStringInfoCharMacro(buf, 0);
+			appendStringInfoChar(buf, 0);
 			/* FALLTHROUGH */
 		case 1:
-			appendStringInfoCharMacro(buf, 0);
+			appendStringInfoChar(buf, 0);
 			/* FALLTHROUGH */
 		default:
 			break;
diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index 06ad83d7cae..fc80dfb1aea 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -424,17 +424,17 @@ record_out(PG_FUNCTION_ARGS)
 
 		/* And emit the string */
 		if (nq)
-			appendStringInfoCharMacro(&buf, '"');
+			appendStringInfoChar(&buf, '"');
 		for (tmp = value; *tmp; tmp++)
 		{
 			char		ch = *tmp;
 
 			if (ch == '"' || ch == '\\')
-				appendStringInfoCharMacro(&buf, ch);
-			appendStringInfoCharMacro(&buf, ch);
+				appendStringInfoChar(&buf, ch);
+			appendStringInfoChar(&buf, ch);
 		}
 		if (nq)
-			appendStringInfoCharMacro(&buf, '"');
+			appendStringInfoChar(&buf, '"');
 	}
 
 	appendStringInfoChar(&buf, ')');
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 1b351cbc688..b11df3f92cd 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5523,7 +5523,7 @@ text_format(PG_FUNCTION_ARGS)
 		 */
 		if (*cp != '%')
 		{
-			appendStringInfoCharMacro(&str, *cp);
+			appendStringInfoChar(&str, *cp);
 			continue;
 		}
 
@@ -5532,7 +5532,7 @@ text_format(PG_FUNCTION_ARGS)
 		/* Easy case: %% outputs a single % */
 		if (*cp == '%')
 		{
-			appendStringInfoCharMacro(&str, *cp);
+			appendStringInfoChar(&str, *cp);
 			continue;
 		}
 
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 3808c307f6f..8e71953831e 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -2391,7 +2391,7 @@ escape_xml(const char *str)
 				appendStringInfoString(&buf, "&#x0d;");
 				break;
 			default:
-				appendStringInfoCharMacro(&buf, *p);
+				appendStringInfoChar(&buf, *p);
 				break;
 		}
 	}
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index f5b0211f66b..55317881ea0 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2724,14 +2724,14 @@ appendCSVLiteral(StringInfo buf, const char *data)
 	if (p == NULL)
 		return;
 
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 	while ((c = *p++) != '\0')
 	{
 		if (c == '"')
-			appendStringInfoCharMacro(buf, '"');
-		appendStringInfoCharMacro(buf, c);
+			appendStringInfoChar(buf, '"');
+		appendStringInfoChar(buf, c);
 	}
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 }
 
 /*
@@ -3491,9 +3491,9 @@ append_with_tabs(StringInfo buf, const char *str)
 
 	while ((ch = *str++) != '\0')
 	{
-		appendStringInfoCharMacro(buf, ch);
+		appendStringInfoChar(buf, ch);
 		if (ch == '\n')
-			appendStringInfoCharMacro(buf, '\t');
+			appendStringInfoChar(buf, '\t');
 	}
 }
 
diff --git a/src/backend/utils/mb/stringinfo_mb.c b/src/backend/utils/mb/stringinfo_mb.c
index c153b770076..1d7245b0a47 100644
--- a/src/backend/utils/mb/stringinfo_mb.c
+++ b/src/backend/utils/mb/stringinfo_mb.c
@@ -61,7 +61,7 @@ appendStringInfoStringQuoted(StringInfo str, const char *s, int maxlen)
 		ellipsis = false;
 	}
 
-	appendStringInfoCharMacro(str, '\'');
+	appendStringInfoChar(str, '\'');
 
 	while ((chunk_end = strchr(chunk_search_start, '\'')) != NULL)
 	{
diff --git a/contrib/tcn/tcn.c b/contrib/tcn/tcn.c
index 552f107bf6b..d38d6e3bca8 100644
--- a/contrib/tcn/tcn.c
+++ b/contrib/tcn/tcn.c
@@ -32,15 +32,15 @@ PG_MODULE_MAGIC;
 static void
 strcpy_quoted(StringInfo r, const char *s, const char q)
 {
-	appendStringInfoCharMacro(r, q);
+	appendStringInfoChar(r, q);
 	while (*s)
 	{
 		if (*s == q)
-			appendStringInfoCharMacro(r, q);
-		appendStringInfoCharMacro(r, *s);
+			appendStringInfoChar(r, q);
+		appendStringInfoChar(r, *s);
 		s++;
 	}
-	appendStringInfoCharMacro(r, q);
+	appendStringInfoChar(r, q);
 }
 
 /*
@@ -147,17 +147,17 @@ triggered_change_notification(PG_FUNCTION_ARGS)
 				foundPK = true;
 
 				strcpy_quoted(payload, RelationGetRelationName(rel), '"');
-				appendStringInfoCharMacro(payload, ',');
-				appendStringInfoCharMacro(payload, operation);
+				appendStringInfoChar(payload, ',');
+				appendStringInfoChar(payload, operation);
 
 				for (i = 0; i < indnkeyatts; i++)
 				{
 					int			colno = index->indkey.values[i];
 					Form_pg_attribute attr = TupleDescAttr(tupdesc, colno - 1);
 
-					appendStringInfoCharMacro(payload, ',');
+					appendStringInfoChar(payload, ',');
 					strcpy_quoted(payload, NameStr(attr->attname), '"');
-					appendStringInfoCharMacro(payload, '=');
+					appendStringInfoChar(payload, '=');
 					strcpy_quoted(payload, SPI_getvalue(trigtuple, tupdesc, colno), '\'');
 				}
 
-- 
2.25.0.rc1

v1-0003-pqformat-Move-functions-for-type-sending-inline-a.patchtext/x-diff; charset=us-asciiDownload
From 93e27b84bef5232346d01cfffe60851434451c2b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:50:06 -0800
Subject: [PATCH v1 3/6] pqformat: Move functions for type sending inline, add
 pq_begintypsend_ex().

Combined with the previous commit inlining more functions in
stringinfo this allows many type send functions to be optimized
considerably better, optimizing away the StringInfoData entirely.

The new pq_begintypsend_ex() is useful to avoid over-allocating for
send functions that only output a small amount of data, e.g. int4send
now allocates 9 bytes, instead of 1024.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/libpq/pqformat.h | 52 ++++++++++++++++++++++++++++++++++++
 src/backend/libpq/pqformat.c | 38 --------------------------
 2 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index af31e9caba3..e0dbb783c6a 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -31,6 +31,58 @@ extern void pq_send_ascii_string(StringInfo buf, const char *str);
 extern void pq_sendfloat4(StringInfo buf, float4 f);
 extern void pq_sendfloat8(StringInfo buf, float8 f);
 
+
+/* --------------------------------
+ *		pq_begintypsend		- initialize for constructing a bytea result
+ * --------------------------------
+ */
+static inline void
+pq_begintypsend(StringInfo buf)
+{
+	initStringInfo(buf);
+	/* Reserve four bytes for the bytea length word */
+	appendStringInfoSpaces(buf, 4);
+}
+
+/* --------------------------------
+ *		pq_begintypsend_ex	- like pq_begintypesend, but with length hint
+ *
+ * This can be used over pq_begintypesend if the caller can cheaply determine
+ * how much data will be sent, reducing the initial size of the
+ * StringInfo. The passed in size need not include the overhead of the length
+ * word.
+ * --------------------------------
+ */
+static inline void
+pq_begintypsend_ex(StringInfo buf, int size)
+{
+	initStringInfoEx(buf, size + 4);
+	/* Reserve four bytes for the bytea length word */
+	appendStringInfoSpaces(buf, 4);
+}
+
+
+/* --------------------------------
+ *		pq_endtypsend	- finish constructing a bytea result
+ *
+ * The data buffer is returned as the palloc'd bytea value.  (We expect
+ * that it will be suitably aligned for this because it has been palloc'd.)
+ * We assume the StringInfoData is just a local variable in the caller and
+ * need not be pfree'd.
+ * --------------------------------
+ */
+static inline bytea *
+pq_endtypsend(StringInfo buf)
+{
+	bytea	   *result = (bytea *) buf->data;
+
+	/* Insert correct length into bytea length word */
+	Assert(buf->len >= VARHDRSZ);
+	SET_VARSIZE(result, buf->len);
+
+	return result;
+}
+
 /*
  * Append a [u]int8 to a StringInfo buffer, which already has enough space
  * preallocated.
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index 999252681c2..347a0aa697a 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -319,44 +319,6 @@ pq_endmessage_reuse(StringInfo buf)
 	(void) pq_putmessage(buf->cursor, buf->data, buf->len);
 }
 
-
-/* --------------------------------
- *		pq_begintypsend		- initialize for constructing a bytea result
- * --------------------------------
- */
-void
-pq_begintypsend(StringInfo buf)
-{
-	initStringInfo(buf);
-	/* Reserve four bytes for the bytea length word */
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-}
-
-/* --------------------------------
- *		pq_endtypsend	- finish constructing a bytea result
- *
- * The data buffer is returned as the palloc'd bytea value.  (We expect
- * that it will be suitably aligned for this because it has been palloc'd.)
- * We assume the StringInfoData is just a local variable in the caller and
- * need not be pfree'd.
- * --------------------------------
- */
-bytea *
-pq_endtypsend(StringInfo buf)
-{
-	bytea	   *result = (bytea *) buf->data;
-
-	/* Insert correct length into bytea length word */
-	Assert(buf->len >= VARHDRSZ);
-	SET_VARSIZE(result, buf->len);
-
-	return result;
-}
-
-
 /* --------------------------------
  *		pq_puttextmessage - generate a character set-converted message in one step
  *
-- 
2.25.0.rc1

v1-0004-WIP-Annotate-palloc-with-malloc-and-other-compile.patchtext/x-diff; charset=us-asciiDownload
From 86224c8f292adba4440f0a2cd4077afd7277c85c Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:53:33 -0800
Subject: [PATCH v1 4/6] WIP: Annotate palloc() with malloc and other compiler
 attributes.

In particularly malloc is useful, allowing the compiler to realize
that the return value does not alias with other data, allowing other
optimizations.

If we were to do this, we'd obviously need to hide these behind
macros to support compilers without those attributes.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/utils/palloc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h
index cc356a63728..a611148be67 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -74,7 +74,7 @@ extern void *MemoryContextAllocZeroAligned(MemoryContext context, Size size);
 extern void *MemoryContextAllocExtended(MemoryContext context,
 										Size size, int flags);
 
-extern void *palloc(Size size);
+extern void *palloc(Size size) __attribute__((malloc, alloc_size(1), assume_aligned(MAXIMUM_ALIGNOF), returns_nonnull, warn_unused_result));
 extern void *palloc0(Size size);
 extern void *palloc_extended(Size size, int flags);
 extern void *repalloc(void *pointer, Size size);
-- 
2.25.0.rc1

v1-0005-Move-int4-int8-send-to-use-pq_begintypsend_ex.patchtext/x-diff; charset=us-asciiDownload
From be4a71efa94e486e47b4e2ba1fb93149f1247b22 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:58:55 -0800
Subject: [PATCH v1 5/6] Move {int4,int8}send to use pq_begintypsend_ex.

If we were to introduce pq_begintypsend_ex(), we'd probably want to do
this to quite a few more functions.
---
 src/backend/utils/adt/int.c  | 2 +-
 src/backend/utils/adt/int8.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index 583ce71e664..1c2528c3095 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -305,7 +305,7 @@ int4send(PG_FUNCTION_ARGS)
 	int32		arg1 = PG_GETARG_INT32(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_ex(&buf, 4);
 	pq_sendint32(&buf, arg1);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index fcdf77331e7..be08530acbc 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -176,7 +176,7 @@ int8send(PG_FUNCTION_ARGS)
 	int64		arg1 = PG_GETARG_INT64(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_ex(&buf, 8);
 	pq_sendint64(&buf, arg1);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
-- 
2.25.0.rc1

v1-0006-copy-Use-appendBinaryStringInfoNT-for-sending-bin.patchtext/x-diff; charset=us-asciiDownload
From d84bf0f4a4e0cc0ac088ea18162f0c8e94cfadf3 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 13:00:26 -0800
Subject: [PATCH v1 6/6] copy: Use appendBinaryStringInfoNT() for sending
 binary data.

Maintaining the trailing byte is useless overhead.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/commands/copy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 88df90deb5b..9c0b38224d1 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -498,13 +498,13 @@ SendCopyEnd(CopyState cstate)
 static void
 CopySendData(CopyState cstate, const void *databuf, int datasize)
 {
-	appendBinaryStringInfo(cstate->fe_msgbuf, databuf, datasize);
+	appendBinaryStringInfoNT(cstate->fe_msgbuf, databuf, datasize);
 }
 
 static void
 CopySendString(CopyState cstate, const char *str)
 {
-	appendBinaryStringInfo(cstate->fe_msgbuf, str, strlen(str));
+	appendBinaryStringInfoNT(cstate->fe_msgbuf, str, strlen(str));
 }
 
 static void
-- 
2.25.0.rc1

#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#7)
2 attachment(s)
Re: Why is pq_begintypsend so slow?

Andres Freund <andres@anarazel.de> writes:

FWIW, I've also observed, in another thread (the node func generation
thing [1]), that inlining enlargeStringInfo() helps a lot, especially
when inlining some of its callers. Moving e.g. appendStringInfo() inline
allows the compiler to sometimes optimize away the strlen. But if
e.g. an inlined appendBinaryStringInfo() still calls enlargeStringInfo()
unconditionally, successive appends cannot optimize away memory accesses
for ->len/->data.

With a set of patches doing so, int4send itself is not a significant
factor for my test benchmark [1] anymore.

This thread seems to have died out, possibly because the last set of
patches that Andres posted was sufficiently complicated and invasive
that nobody wanted to review it. I thought about this again after
seeing that [1]/messages/by-id/CAMovtNoHFod2jMAKQjjxv209PCTJx5Kc66anwWvX0mEiaXwgmA@mail.gmail.com is mostly about pq_begintypsend overhead, and had
an epiphany: there isn't really a strong reason for pq_begintypsend
to be inserting bits into the buffer at all. The bytes will be
filled by pq_endtypsend, and nothing in between should be touching
them. So I propose 0001 attached. It's poking into the stringinfo
abstraction a bit more than I would want to do if there weren't a
compelling performance reason to do so, but there evidently is.

With 0001, pq_begintypsend drops from being the top single routine
in a profile of a test case like [1]/messages/by-id/CAMovtNoHFod2jMAKQjjxv209PCTJx5Kc66anwWvX0mEiaXwgmA@mail.gmail.com to being well down the list.
The next biggest cost compared to text-format output is that
printtup() itself is noticeably more expensive. A lot of the extra
cost there seems to be from pq_sendint32(), which is getting inlined
into printtup(), and there probably isn't much we can do to make that
cheaper. But eliminating a common subexpression as in 0002 below does
help noticeably, at least with the rather old gcc I'm using.

For me, the combination of these two eliminates most but not quite
all of the cost penalty of binary over text output as seen in [1]/messages/by-id/CAMovtNoHFod2jMAKQjjxv209PCTJx5Kc66anwWvX0mEiaXwgmA@mail.gmail.com.

regards, tom lane

[1]: /messages/by-id/CAMovtNoHFod2jMAKQjjxv209PCTJx5Kc66anwWvX0mEiaXwgmA@mail.gmail.com

Attachments:

0001-fix-pq_begintypsend.patchtext/x-diff; charset=us-ascii; name=0001-fix-pq_begintypsend.patchDownload
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index a6f990c..03b7404 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -328,11 +328,16 @@ void
 pq_begintypsend(StringInfo buf)
 {
 	initStringInfo(buf);
-	/* Reserve four bytes for the bytea length word */
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
+
+	/*
+	 * Reserve four bytes for the bytea length word.  We don't need to fill
+	 * them with anything (pq_endtypsend will do that), and this function is
+	 * enough of a hot spot that it's worth cheating to save some cycles. Note
+	 * in particular that we don't bother to guarantee that the buffer is
+	 * null-terminated.
+	 */
+	Assert(buf->maxlen > 4);
+	buf->len = 4;
 }
 
 /* --------------------------------
0002-minor-printtup-fix.patchtext/x-diff; charset=us-ascii; name=0002-minor-printtup-fix.patchDownload
diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index dd1bac0..a9315c6 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -438,11 +438,12 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 		{
 			/* Binary output */
 			bytea	   *outputbytes;
+			int			outputlen;
 
 			outputbytes = SendFunctionCall(&thisState->finfo, attr);
-			pq_sendint32(buf, VARSIZE(outputbytes) - VARHDRSZ);
-			pq_sendbytes(buf, VARDATA(outputbytes),
-						 VARSIZE(outputbytes) - VARHDRSZ);
+			outputlen = VARSIZE(outputbytes) - VARHDRSZ;
+			pq_sendint32(buf, outputlen);
+			pq_sendbytes(buf, VARDATA(outputbytes), outputlen);
 		}
 	}
 
#9Ranier Vilela
ranier.vf@gmail.com
In reply to: Tom Lane (#8)
Re: Why is pq_begintypsend so slow?

Em seg., 18 de mai. de 2020 às 13:38, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

Andres Freund <andres@anarazel.de> writes:

FWIW, I've also observed, in another thread (the node func generation
thing [1]), that inlining enlargeStringInfo() helps a lot, especially
when inlining some of its callers. Moving e.g. appendStringInfo() inline
allows the compiler to sometimes optimize away the strlen. But if
e.g. an inlined appendBinaryStringInfo() still calls enlargeStringInfo()
unconditionally, successive appends cannot optimize away memory accesses
for ->len/->data.

With a set of patches doing so, int4send itself is not a significant
factor for my test benchmark [1] anymore.

This thread seems to have died out, possibly because the last set of
patches that Andres posted was sufficiently complicated and invasive
that nobody wanted to review it. I thought about this again after
seeing that [1] is mostly about pq_begintypsend overhead, and had
an epiphany: there isn't really a strong reason for pq_begintypsend
to be inserting bits into the buffer at all. The bytes will be
filled by pq_endtypsend, and nothing in between should be touching
them. So I propose 0001 attached. It's poking into the stringinfo
abstraction a bit more than I would want to do if there weren't a
compelling performance reason to do so, but there evidently is.

With 0001, pq_begintypsend drops from being the top single routine
in a profile of a test case like [1] to being well down the list.
The next biggest cost compared to text-format output is that
printtup() itself is noticeably more expensive. A lot of the extra
cost there seems to be from pq_sendint32(), which is getting inlined
into printtup(), and there probably isn't much we can do to make that
cheaper. But eliminating a common subexpression as in 0002 below does
help noticeably, at least with the rather old gcc I'm using.

Again, I see problems with the types declared in Postgres.
1. pq_sendint32 (StringInfo buf, uint32 i)
2. extern void pq_sendbytes (StringInfo buf, const char * data, int
datalen);

Wouldn't it be better to declare outputlen (0002) as uint32?
To avoid converting from (int) to (uint32), even if afterwards there is a
conversion from (uint32) to (int)?

regards,
Ranier Vilela

#10Tom Lane
tgl@sss.pgh.pa.us
In reply to: Ranier Vilela (#9)
Re: Why is pq_begintypsend so slow?

Ranier Vilela <ranier.vf@gmail.com> writes:

Again, I see problems with the types declared in Postgres.
1. pq_sendint32 (StringInfo buf, uint32 i)
2. extern void pq_sendbytes (StringInfo buf, const char * data, int
datalen);

We could spend the next ten years cleaning up minor discrepancies
like that, and have nothing much to show for the work.

To avoid converting from (int) to (uint32), even if afterwards there is a
conversion from (uint32) to (int)?

You do realize that that conversion costs nothing?

regards, tom lane

#11Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#8)
9 attachment(s)
Re: Why is pq_begintypsend so slow?

Hi,

On 2020-05-18 12:38:05 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

FWIW, I've also observed, in another thread (the node func generation
thing [1]), that inlining enlargeStringInfo() helps a lot, especially
when inlining some of its callers. Moving e.g. appendStringInfo() inline
allows the compiler to sometimes optimize away the strlen. But if
e.g. an inlined appendBinaryStringInfo() still calls enlargeStringInfo()
unconditionally, successive appends cannot optimize away memory accesses
for ->len/->data.

With a set of patches doing so, int4send itself is not a significant
factor for my test benchmark [1] anymore.

This thread seems to have died out, possibly because the last set of
patches that Andres posted was sufficiently complicated and invasive
that nobody wanted to review it.

Well, I wasn't really planning to try to get that patchset into 13, and
it wasn't in the CF...

I thought about this again after seeing that [1] is mostly about
pq_begintypsend overhead

I'm not really convinced that that's the whole problem. Using the
benchmark from upthread, I get (median of three):
master: 1181.581
yours: 1171.445
mine: 598.031

That's a very significant difference, imo. It helps a bit with the
benchmark from your [1], but not that much.

With 0001, pq_begintypsend drops from being the top single routine
in a profile of a test case like [1] to being well down the list.
The next biggest cost compared to text-format output is that
printtup() itself is noticeably more expensive. A lot of the extra
cost there seems to be from pq_sendint32(), which is getting inlined
into printtup(), and there probably isn't much we can do to make that
cheaper. But eliminating a common subexpression as in 0002 below does
help noticeably, at least with the rather old gcc I'm using.

I think there's plenty more we can do:

First, it's unnecessary to re-initialize a FunctionCallInfo for every
send/recv/out/in call. Instead we can reuse the same over and over.

After that, the biggest remaining overhead for Jack's test is the palloc
for the stringinfo, as far as I can see. I've complained about that
before...

I've just hacked up a modification where, for send functions,
fcinfo->context contains a stringinfo set up by printtup/CopyTo. That,
combined with using a FunctionCallInfo set up beforehand, instead of
re-initializing it in every printtup cycle, results in a pretty good
saving.

Making the binary protocol 20% faster than text, in Jack's testcase. And
my lotsaints4 test, goes further down to ~410ms (this is 2.5x faster
than where started).

Now obviously, the hack with passing a StringInfo in ->context is just
that, a hack. A somewhat gross one even. But I think it pretty clearly
shows the problem and the way out.

I don't know what the best non-gross solution for the overhead of the
out/send functions is. There seems to be at least the following
major options (and a lots of variants thereof):

1) Just continue to incur significant overhead for every datum
2) Accept the uglyness of passing in a buffer via
FunctionCallInfo->context. Change nearly all in-core send functions
over to that.
3) Pass string buffer through an new INTERNAL argument to send/output
function, allow both old/new style send functions. Use a wrapper
function to adapt the "old style" to the "new style".
4) Like 3, but make the new argument optional, and use ad-hoc
stringbuffer if not provided. I don't like the unnecessary branches
this adds.

The biggest problem after that is that we waste a lot of time memcpying
stuff around repeatedly. There is:
1) send function: datum -> per datum stringinfo
2) printtup: per datum stringinfo -> per row stringinfo
3) socket_putmessage: per row stringinfo -> PqSendBuffer
4) send(): PqSendBuffer -> kernel buffer

It's obviously hard to avoid 1) and 4) in the common case, but the
number of other copies seem pretty clearly excessive.

If we change the signature of the out/send function to always target a
string buffer, we could pretty easily avoid 2), and for out functions
we'd not have to redundantly call strlen (as the argument to
pq_sendcountedtext) anymore, which seems substantial too.

As I argued before, I think it's unnecessary to have a separate buffer
between 3-4). We should construct the outgoing message inside the send
buffer. I still don't understand what "recursion" danger there is,
nothing below printtup should ever send protocol messages, no?

Sometimes there's also 0) in the above, when detoasting a datum...

Greetings,

Andres Freund

Attachments:

v2-0001-stringinfo-Move-more-functions-inline-provide-ini.patchtext/x-diff; charset=us-asciiDownload
From ae981295c55e7b02f81e85171ad840b6a3d5b330 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:46:20 -0800
Subject: [PATCH v2 1/9] stringinfo: Move more functions inline, provide
 initStringInfoEx().

By moving the whole lifecycle of a stringinfo into inline functions,
a good compiler sometimes can be able to optimize away the existence
of the StringInfoData itself.

initStringInfoEx() allows stringinfo users to determine the initial
allocation size, avoiding over/under allocation when known.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/lib/stringinfo.h | 205 +++++++++++++++++++++++++++--------
 src/common/stringinfo.c      | 173 ++---------------------------
 2 files changed, 171 insertions(+), 207 deletions(-)

diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 5a2a3dbfbc0..38211323d9d 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -18,6 +18,9 @@
 #ifndef STRINGINFO_H
 #define STRINGINFO_H
 
+#include "common/int.h"
+#include "common/string.h"
+
 /*-------------------------
  * StringInfoData holds information about an extensible string.
  *		data	is the current buffer for the string (allocated with palloc).
@@ -66,25 +69,137 @@ typedef StringInfoData *StringInfo;
  *-------------------------
  */
 
-/*------------------------
- * makeStringInfo
- * Create an empty 'StringInfoData' & return a pointer to it.
- */
-extern StringInfo makeStringInfo(void);
-
-/*------------------------
- * initStringInfo
- * Initialize a StringInfoData struct (with previously undefined contents)
- * to describe an empty string.
- */
-extern void initStringInfo(StringInfo str);
 
 /*------------------------
  * resetStringInfo
  * Clears the current content of the StringInfo, if any. The
  * StringInfo remains valid.
  */
-extern void resetStringInfo(StringInfo str);
+static inline void
+resetStringInfo(StringInfoData *pg_restrict str)
+{
+	*(char *pg_restrict) (str->data) = '\0';
+	str->len = 0;
+	str->cursor = 0;
+}
+
+/*------------------------
+ * initStringInfo
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ */
+static inline void
+initStringInfo(StringInfoData *pg_restrict str)
+{
+	int			size = 1024;	/* initial default buffer size */
+
+	str->data = (char *) palloc(size);
+	str->maxlen = size;
+	resetStringInfo(str);
+}
+
+/*------------------------
+ * initStringInfoEx
+ *
+ * Like initStringInfo(), but allows to specify the size of the initial
+ * allocation.
+ */
+static inline void
+initStringInfoEx(StringInfoData *pg_restrict str, int size)
+{
+	/*
+	 * FIXME: Adding 1 to account for always present trailing \0 byte. Should
+	 * that instead be done at the caller level? Should we round up?
+	 */
+	str->data = (char *) palloc(size + 1);
+	str->maxlen = size + 1;
+	resetStringInfo(str);
+}
+
+/*------------------------
+ * makeStringInfo
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ */
+static inline StringInfo
+makeStringInfo(void)
+{
+	StringInfo	res;
+
+	res = (StringInfo) palloc(sizeof(StringInfoData));
+
+	initStringInfo(res);
+
+	return res;
+}
+
+/*------------------------
+ * enlargeStringInfoImpl
+ *
+ * Actually enlarge the string, only to be called by enlargeStringInfo().
+ */
+extern void enlargeStringInfoImpl(StringInfo str, int needed);
+
+/*------------------------
+ * enlargeStringInfo
+ * Make sure a StringInfo's buffer can hold at least 'needed' more bytes.
+ *
+ * External callers usually need not concern themselves with this, since
+ * all stringinfo.c routines do it automatically.  However, if a caller
+ * knows that a StringInfo will eventually become X bytes large, it
+ * can save some palloc overhead by enlarging the buffer before starting
+ * to store data in it.
+ *
+ * NB: In the backend, because we use repalloc() to enlarge the buffer, the
+ * string buffer will remain allocated in the same memory context that was
+ * current when initStringInfo was called, even if another context is now
+ * current.  This is the desired and indeed critical behavior!
+ */
+static inline void
+enlargeStringInfo(StringInfoData *pg_restrict str, int datalen)
+{
+	int res;
+
+	if (unlikely(pg_add_s32_overflow(str->len, datalen, &res)) ||
+		unlikely(res >= str->maxlen))
+		enlargeStringInfoImpl(str, datalen);
+}
+
+/*------------------------
+ * appendBinaryStringInfoNT
+ * Append arbitrary binary data to a StringInfo, allocating more space
+ * if necessary. Does not ensure a trailing null-byte exists.
+ */
+static inline void
+appendBinaryStringInfoNT(StringInfoData *pg_restrict str, const char *pg_restrict data, int datalen)
+{
+	Assert(str != NULL);
+
+	/* Make more room if needed */
+	enlargeStringInfo(str, datalen);
+
+	/* OK, append the data */
+	memcpy((char *pg_restrict) (str->data + str->len), data, datalen);
+	str->len += datalen;
+}
+
+/*------------------------
+ * appendBinaryStringInfo
+ * Append arbitrary binary data to a StringInfo, allocating more space
+ * if necessary. Ensures that a trailing null byte is present.
+ */
+static inline void
+appendBinaryStringInfo(StringInfoData *pg_restrict str, const char *pg_restrict data, int datalen)
+{
+	appendBinaryStringInfoNT(str, data, datalen);
+
+	/*
+	 * Keep a trailing null in place, even though it's probably useless for
+	 * binary data.  (Some callers are dealing with text but call this because
+	 * their input isn't null-terminated.)
+	 */
+	*(char *pg_restrict) (str->data + str->len) = '\0';
+}
+
 
 /*------------------------
  * appendStringInfo
@@ -111,51 +226,49 @@ extern int	appendStringInfoVA(StringInfo str, const char *fmt, va_list args) pg_
  * Append a null-terminated string to str.
  * Like appendStringInfo(str, "%s", s) but faster.
  */
-extern void appendStringInfoString(StringInfo str, const char *s);
+static inline void
+appendStringInfoString(StringInfoData *pg_restrict str, const char *pg_restrict s)
+{
+	appendBinaryStringInfo(str, s, strlen(s));
+}
 
 /*------------------------
  * appendStringInfoChar
  * Append a single byte to str.
  * Like appendStringInfo(str, "%c", ch) but much faster.
  */
-extern void appendStringInfoChar(StringInfo str, char ch);
+static inline void
+appendStringInfoChar(StringInfoData *pg_restrict str, char ch)
+{
+	/* Make more room if needed */
+	enlargeStringInfo(str, 1);
 
-/*------------------------
- * appendStringInfoCharMacro
- * As above, but a macro for even more speed where it matters.
- * Caution: str argument will be evaluated multiple times.
- */
-#define appendStringInfoCharMacro(str,ch) \
-	(((str)->len + 1 >= (str)->maxlen) ? \
-	 appendStringInfoChar(str, ch) : \
-	 (void)((str)->data[(str)->len] = (ch), (str)->data[++(str)->len] = '\0'))
+	/* OK, append the character */
+	*(char *pg_restrict) (str->data + str->len) = ch;
+	str->len++;
+	*(char *pg_restrict) (str->data + str->len) = '\0';
+}
+
+/* backward compat for external code */
+#define appendStringInfoCharMacro appendStringInfoChar
 
 /*------------------------
  * appendStringInfoSpaces
  * Append a given number of spaces to str.
  */
-extern void appendStringInfoSpaces(StringInfo str, int count);
+static inline void
+appendStringInfoSpaces(StringInfoData *pg_restrict str, int count)
+{
+	if (count > 0)
+	{
+		/* Make more room if needed */
+		enlargeStringInfo(str, count);
 
-/*------------------------
- * appendBinaryStringInfo
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary.
- */
-extern void appendBinaryStringInfo(StringInfo str,
-								   const char *data, int datalen);
-
-/*------------------------
- * appendBinaryStringInfoNT
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Does not ensure a trailing null-byte exists.
- */
-extern void appendBinaryStringInfoNT(StringInfo str,
-									 const char *data, int datalen);
-
-/*------------------------
- * enlargeStringInfo
- * Make sure a StringInfo's buffer can hold at least 'needed' more bytes.
- */
-extern void enlargeStringInfo(StringInfo str, int needed);
+		/* OK, append the spaces */
+		memset((char *pg_restrict) (str->data + str->len), ' ', count);
+		str->len += count;
+		*(char *pg_restrict) (str->data + str->len) = '\0';
+	}
+}
 
 #endif							/* STRINGINFO_H */
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 0badc465545..0ea5e0abe56 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -32,53 +32,6 @@
 #include "lib/stringinfo.h"
 
 
-/*
- * makeStringInfo
- *
- * Create an empty 'StringInfoData' & return a pointer to it.
- */
-StringInfo
-makeStringInfo(void)
-{
-	StringInfo	res;
-
-	res = (StringInfo) palloc(sizeof(StringInfoData));
-
-	initStringInfo(res);
-
-	return res;
-}
-
-/*
- * initStringInfo
- *
- * Initialize a StringInfoData struct (with previously undefined contents)
- * to describe an empty string.
- */
-void
-initStringInfo(StringInfo str)
-{
-	int			size = 1024;	/* initial default buffer size */
-
-	str->data = (char *) palloc(size);
-	str->maxlen = size;
-	resetStringInfo(str);
-}
-
-/*
- * resetStringInfo
- *
- * Reset the StringInfo: the data buffer remains valid, but its
- * previous content, if any, is cleared.
- */
-void
-resetStringInfo(StringInfo str)
-{
-	str->data[0] = '\0';
-	str->len = 0;
-	str->cursor = 0;
-}
-
 /*
  * appendStringInfo
  *
@@ -167,120 +120,18 @@ appendStringInfoVA(StringInfo str, const char *fmt, va_list args)
 }
 
 /*
- * appendStringInfoString
+ * enlargeStringInfoImpl
  *
- * Append a null-terminated string to str.
- * Like appendStringInfo(str, "%s", s) but faster.
+ * Make enough space for 'needed' more bytes ('needed' does not include the
+ * terminating null). This is not for external consumption, it's only to be
+ * called by enlargeStringInfo() when more space is actually needed (including
+ * when we'd overflow the maximum size).
+ *
+ * As this normally shouldn't be the common case, mark as noinline, to avoid
+ * including the function into the fastpath.
  */
-void
-appendStringInfoString(StringInfo str, const char *s)
-{
-	appendBinaryStringInfo(str, s, strlen(s));
-}
-
-/*
- * appendStringInfoChar
- *
- * Append a single byte to str.
- * Like appendStringInfo(str, "%c", ch) but much faster.
- */
-void
-appendStringInfoChar(StringInfo str, char ch)
-{
-	/* Make more room if needed */
-	if (str->len + 1 >= str->maxlen)
-		enlargeStringInfo(str, 1);
-
-	/* OK, append the character */
-	str->data[str->len] = ch;
-	str->len++;
-	str->data[str->len] = '\0';
-}
-
-/*
- * appendStringInfoSpaces
- *
- * Append the specified number of spaces to a buffer.
- */
-void
-appendStringInfoSpaces(StringInfo str, int count)
-{
-	if (count > 0)
-	{
-		/* Make more room if needed */
-		enlargeStringInfo(str, count);
-
-		/* OK, append the spaces */
-		while (--count >= 0)
-			str->data[str->len++] = ' ';
-		str->data[str->len] = '\0';
-	}
-}
-
-/*
- * appendBinaryStringInfo
- *
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Ensures that a trailing null byte is present.
- */
-void
-appendBinaryStringInfo(StringInfo str, const char *data, int datalen)
-{
-	Assert(str != NULL);
-
-	/* Make more room if needed */
-	enlargeStringInfo(str, datalen);
-
-	/* OK, append the data */
-	memcpy(str->data + str->len, data, datalen);
-	str->len += datalen;
-
-	/*
-	 * Keep a trailing null in place, even though it's probably useless for
-	 * binary data.  (Some callers are dealing with text but call this because
-	 * their input isn't null-terminated.)
-	 */
-	str->data[str->len] = '\0';
-}
-
-/*
- * appendBinaryStringInfoNT
- *
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Does not ensure a trailing null-byte exists.
- */
-void
-appendBinaryStringInfoNT(StringInfo str, const char *data, int datalen)
-{
-	Assert(str != NULL);
-
-	/* Make more room if needed */
-	enlargeStringInfo(str, datalen);
-
-	/* OK, append the data */
-	memcpy(str->data + str->len, data, datalen);
-	str->len += datalen;
-}
-
-/*
- * enlargeStringInfo
- *
- * Make sure there is enough space for 'needed' more bytes
- * ('needed' does not include the terminating null).
- *
- * External callers usually need not concern themselves with this, since
- * all stringinfo.c routines do it automatically.  However, if a caller
- * knows that a StringInfo will eventually become X bytes large, it
- * can save some palloc overhead by enlarging the buffer before starting
- * to store data in it.
- *
- * NB: In the backend, because we use repalloc() to enlarge the buffer, the
- * string buffer will remain allocated in the same memory context that was
- * current when initStringInfo was called, even if another context is now
- * current.  This is the desired and indeed critical behavior!
- */
-void
-enlargeStringInfo(StringInfo str, int needed)
+pg_noinline void
+enlargeStringInfoImpl(StringInfo str, int needed)
 {
 	int			newlen;
 
@@ -317,8 +168,8 @@ enlargeStringInfo(StringInfo str, int needed)
 
 	/* Because of the above test, we now have needed <= MaxAllocSize */
 
-	if (needed <= str->maxlen)
-		return;					/* got enough space already */
+	/* should only be called when needed */
+	Assert(needed > str->maxlen);
 
 	/*
 	 * We don't want to allocate just a little more space with each append;
-- 
2.25.0.114.g5b0ca878e0

v2-0002-stringinfo-Remove-in-core-use-of-appendStringInfo.patchtext/x-diff; charset=us-asciiDownload
From f62b09d1fd2dd0d5e6b807ecb3a5481f48106692 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:58:00 -0800
Subject: [PATCH v2 2/9] stringinfo: Remove in-core use of
 appendStringInfoCharMacro().

Kept seperate only to reduce size of patch moving to more inlining for
stringinfo.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/commands/copy.c          |  2 +-
 src/backend/commands/explain.c       |  8 ++++----
 src/backend/libpq/pqformat.c         |  2 +-
 src/backend/utils/adt/json.c         |  6 +++---
 src/backend/utils/adt/jsonb.c        | 10 +++++-----
 src/backend/utils/adt/jsonfuncs.c    | 28 ++++++++++++++--------------
 src/backend/utils/adt/jsonpath.c     |  6 +++---
 src/backend/utils/adt/rowtypes.c     |  8 ++++----
 src/backend/utils/adt/varlena.c      |  4 ++--
 src/backend/utils/adt/xml.c          |  2 +-
 src/backend/utils/error/elog.c       | 12 ++++++------
 src/backend/utils/mb/stringinfo_mb.c |  2 +-
 contrib/tcn/tcn.c                    | 16 ++++++++--------
 13 files changed, 53 insertions(+), 53 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 6d53dc463c1..e51c181f55c 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -511,7 +511,7 @@ CopySendString(CopyState cstate, const char *str)
 static void
 CopySendChar(CopyState cstate, char c)
 {
-	appendStringInfoCharMacro(cstate->fe_msgbuf, c);
+	appendStringInfoChar(cstate->fe_msgbuf, c);
 }
 
 static void
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index efd7201d611..08a0b069ed1 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -4551,16 +4551,16 @@ ExplainXMLTag(const char *tagname, int flags, ExplainState *es)
 
 	if ((flags & X_NOWHITESPACE) == 0)
 		appendStringInfoSpaces(es->str, 2 * es->indent);
-	appendStringInfoCharMacro(es->str, '<');
+	appendStringInfoChar(es->str, '<');
 	if ((flags & X_CLOSING) != 0)
-		appendStringInfoCharMacro(es->str, '/');
+		appendStringInfoChar(es->str, '/');
 	for (s = tagname; *s; s++)
 		appendStringInfoChar(es->str, strchr(valid, *s) ? *s : '-');
 	if ((flags & X_CLOSE_IMMEDIATE) != 0)
 		appendStringInfoString(es->str, " /");
-	appendStringInfoCharMacro(es->str, '>');
+	appendStringInfoChar(es->str, '>');
 	if ((flags & X_NOWHITESPACE) == 0)
-		appendStringInfoCharMacro(es->str, '\n');
+		appendStringInfoChar(es->str, '\n');
 }
 
 /*
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index a6f990c2d29..999252681c2 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -234,7 +234,7 @@ pq_send_ascii_string(StringInfo buf, const char *str)
 
 		if (IS_HIGHBIT_SET(ch))
 			ch = '?';
-		appendStringInfoCharMacro(buf, ch);
+		appendStringInfoChar(buf, ch);
 	}
 	appendStringInfoChar(buf, '\0');
 }
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 641ae3fdf8e..08f3eeb927f 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -1280,7 +1280,7 @@ escape_json(StringInfo buf, const char *str)
 {
 	const char *p;
 
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 	for (p = str; *p; p++)
 	{
 		switch (*p)
@@ -1310,11 +1310,11 @@ escape_json(StringInfo buf, const char *str)
 				if ((unsigned char) *p < ' ')
 					appendStringInfo(buf, "\\u%04x", (int) *p);
 				else
-					appendStringInfoCharMacro(buf, *p);
+					appendStringInfoChar(buf, *p);
 				break;
 		}
 	}
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 }
 
 /*
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 1e9ca046c69..7e8792dff62 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -515,7 +515,7 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 				if (!v.val.array.rawScalar)
 				{
 					add_indent(out, use_indent && !last_was_key, level);
-					appendStringInfoCharMacro(out, '[');
+					appendStringInfoChar(out, '[');
 				}
 				else
 					raw_scalar = true;
@@ -528,7 +528,7 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 					appendBinaryStringInfo(out, ", ", ispaces);
 
 				add_indent(out, use_indent && !last_was_key, level);
-				appendStringInfoCharMacro(out, '{');
+				appendStringInfoChar(out, '{');
 
 				first = true;
 				level++;
@@ -576,14 +576,14 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 				if (!raw_scalar)
 				{
 					add_indent(out, use_indent, level);
-					appendStringInfoCharMacro(out, ']');
+					appendStringInfoChar(out, ']');
 				}
 				first = false;
 				break;
 			case WJB_END_OBJECT:
 				level--;
 				add_indent(out, use_indent, level);
-				appendStringInfoCharMacro(out, '}');
+				appendStringInfoChar(out, '}');
 				first = false;
 				break;
 			default:
@@ -605,7 +605,7 @@ add_indent(StringInfo out, bool indent, int level)
 	{
 		int			i;
 
-		appendStringInfoCharMacro(out, '\n');
+		appendStringInfoChar(out, '\n');
 		for (i = 0; i < level; i++)
 			appendBinaryStringInfo(out, "    ", 4);
 	}
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 5a09d65fdce..eefb402e010 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -3979,7 +3979,7 @@ sn_object_start(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '{');
+	appendStringInfoChar(_state->strval, '{');
 }
 
 static void
@@ -3987,7 +3987,7 @@ sn_object_end(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '}');
+	appendStringInfoChar(_state->strval, '}');
 }
 
 static void
@@ -3995,7 +3995,7 @@ sn_array_start(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '[');
+	appendStringInfoChar(_state->strval, '[');
 }
 
 static void
@@ -4003,7 +4003,7 @@ sn_array_end(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, ']');
+	appendStringInfoChar(_state->strval, ']');
 }
 
 static void
@@ -4023,7 +4023,7 @@ sn_object_field_start(void *state, char *fname, bool isnull)
 	}
 
 	if (_state->strval->data[_state->strval->len - 1] != '{')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 
 	/*
 	 * Unfortunately we don't have the quoted and escaped string any more, so
@@ -4031,7 +4031,7 @@ sn_object_field_start(void *state, char *fname, bool isnull)
 	 */
 	escape_json(_state->strval, fname);
 
-	appendStringInfoCharMacro(_state->strval, ':');
+	appendStringInfoChar(_state->strval, ':');
 }
 
 static void
@@ -4040,7 +4040,7 @@ sn_array_element_start(void *state, bool isnull)
 	StripnullState *_state = (StripnullState *) state;
 
 	if (_state->strval->data[_state->strval->len - 1] != '[')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 }
 
 static void
@@ -5364,7 +5364,7 @@ transform_string_values_object_start(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '{');
+	appendStringInfoChar(_state->strval, '{');
 }
 
 static void
@@ -5372,7 +5372,7 @@ transform_string_values_object_end(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '}');
+	appendStringInfoChar(_state->strval, '}');
 }
 
 static void
@@ -5380,7 +5380,7 @@ transform_string_values_array_start(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '[');
+	appendStringInfoChar(_state->strval, '[');
 }
 
 static void
@@ -5388,7 +5388,7 @@ transform_string_values_array_end(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, ']');
+	appendStringInfoChar(_state->strval, ']');
 }
 
 static void
@@ -5397,14 +5397,14 @@ transform_string_values_object_field_start(void *state, char *fname, bool isnull
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
 	if (_state->strval->data[_state->strval->len - 1] != '{')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 
 	/*
 	 * Unfortunately we don't have the quoted and escaped string any more, so
 	 * we have to re-escape it.
 	 */
 	escape_json(_state->strval, fname);
-	appendStringInfoCharMacro(_state->strval, ':');
+	appendStringInfoChar(_state->strval, ':');
 }
 
 static void
@@ -5413,7 +5413,7 @@ transform_string_values_array_element_start(void *state, bool isnull)
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
 	if (_state->strval->data[_state->strval->len - 1] != '[')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 }
 
 static void
diff --git a/src/backend/utils/adt/jsonpath.c b/src/backend/utils/adt/jsonpath.c
index 3c0dc38a7f8..43d5914477a 100644
--- a/src/backend/utils/adt/jsonpath.c
+++ b/src/backend/utils/adt/jsonpath.c
@@ -441,13 +441,13 @@ alignStringInfoInt(StringInfo buf)
 	switch (INTALIGN(buf->len) - buf->len)
 	{
 		case 3:
-			appendStringInfoCharMacro(buf, 0);
+			appendStringInfoChar(buf, 0);
 			/* FALLTHROUGH */
 		case 2:
-			appendStringInfoCharMacro(buf, 0);
+			appendStringInfoChar(buf, 0);
 			/* FALLTHROUGH */
 		case 1:
-			appendStringInfoCharMacro(buf, 0);
+			appendStringInfoChar(buf, 0);
 			/* FALLTHROUGH */
 		default:
 			break;
diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index 80cba2f4c26..987ebc9d7f8 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -424,17 +424,17 @@ record_out(PG_FUNCTION_ARGS)
 
 		/* And emit the string */
 		if (nq)
-			appendStringInfoCharMacro(&buf, '"');
+			appendStringInfoChar(&buf, '"');
 		for (tmp = value; *tmp; tmp++)
 		{
 			char		ch = *tmp;
 
 			if (ch == '"' || ch == '\\')
-				appendStringInfoCharMacro(&buf, ch);
-			appendStringInfoCharMacro(&buf, ch);
+				appendStringInfoChar(&buf, ch);
+			appendStringInfoChar(&buf, ch);
 		}
 		if (nq)
-			appendStringInfoCharMacro(&buf, '"');
+			appendStringInfoChar(&buf, '"');
 	}
 
 	appendStringInfoChar(&buf, ')');
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 2eaabd6231d..f3c1a63455a 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5557,7 +5557,7 @@ text_format(PG_FUNCTION_ARGS)
 		 */
 		if (*cp != '%')
 		{
-			appendStringInfoCharMacro(&str, *cp);
+			appendStringInfoChar(&str, *cp);
 			continue;
 		}
 
@@ -5566,7 +5566,7 @@ text_format(PG_FUNCTION_ARGS)
 		/* Easy case: %% outputs a single % */
 		if (*cp == '%')
 		{
-			appendStringInfoCharMacro(&str, *cp);
+			appendStringInfoChar(&str, *cp);
 			continue;
 		}
 
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 4c299057a6f..9718cd05912 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -2373,7 +2373,7 @@ escape_xml(const char *str)
 				appendStringInfoString(&buf, "&#x0d;");
 				break;
 			default:
-				appendStringInfoCharMacro(&buf, *p);
+				appendStringInfoChar(&buf, *p);
 				break;
 		}
 	}
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index e9762010309..3697bdc53aa 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2630,14 +2630,14 @@ appendCSVLiteral(StringInfo buf, const char *data)
 	if (p == NULL)
 		return;
 
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 	while ((c = *p++) != '\0')
 	{
 		if (c == '"')
-			appendStringInfoCharMacro(buf, '"');
-		appendStringInfoCharMacro(buf, c);
+			appendStringInfoChar(buf, '"');
+		appendStringInfoChar(buf, c);
 	}
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 }
 
 /*
@@ -3407,9 +3407,9 @@ append_with_tabs(StringInfo buf, const char *str)
 
 	while ((ch = *str++) != '\0')
 	{
-		appendStringInfoCharMacro(buf, ch);
+		appendStringInfoChar(buf, ch);
 		if (ch == '\n')
-			appendStringInfoCharMacro(buf, '\t');
+			appendStringInfoChar(buf, '\t');
 	}
 }
 
diff --git a/src/backend/utils/mb/stringinfo_mb.c b/src/backend/utils/mb/stringinfo_mb.c
index 5f51f538c18..a9f43d0b32b 100644
--- a/src/backend/utils/mb/stringinfo_mb.c
+++ b/src/backend/utils/mb/stringinfo_mb.c
@@ -61,7 +61,7 @@ appendStringInfoStringQuoted(StringInfo str, const char *s, int maxlen)
 		ellipsis = false;
 	}
 
-	appendStringInfoCharMacro(str, '\'');
+	appendStringInfoChar(str, '\'');
 
 	while ((chunk_end = strchr(chunk_search_start, '\'')) != NULL)
 	{
diff --git a/contrib/tcn/tcn.c b/contrib/tcn/tcn.c
index 552f107bf6b..d38d6e3bca8 100644
--- a/contrib/tcn/tcn.c
+++ b/contrib/tcn/tcn.c
@@ -32,15 +32,15 @@ PG_MODULE_MAGIC;
 static void
 strcpy_quoted(StringInfo r, const char *s, const char q)
 {
-	appendStringInfoCharMacro(r, q);
+	appendStringInfoChar(r, q);
 	while (*s)
 	{
 		if (*s == q)
-			appendStringInfoCharMacro(r, q);
-		appendStringInfoCharMacro(r, *s);
+			appendStringInfoChar(r, q);
+		appendStringInfoChar(r, *s);
 		s++;
 	}
-	appendStringInfoCharMacro(r, q);
+	appendStringInfoChar(r, q);
 }
 
 /*
@@ -147,17 +147,17 @@ triggered_change_notification(PG_FUNCTION_ARGS)
 				foundPK = true;
 
 				strcpy_quoted(payload, RelationGetRelationName(rel), '"');
-				appendStringInfoCharMacro(payload, ',');
-				appendStringInfoCharMacro(payload, operation);
+				appendStringInfoChar(payload, ',');
+				appendStringInfoChar(payload, operation);
 
 				for (i = 0; i < indnkeyatts; i++)
 				{
 					int			colno = index->indkey.values[i];
 					Form_pg_attribute attr = TupleDescAttr(tupdesc, colno - 1);
 
-					appendStringInfoCharMacro(payload, ',');
+					appendStringInfoChar(payload, ',');
 					strcpy_quoted(payload, NameStr(attr->attname), '"');
-					appendStringInfoCharMacro(payload, '=');
+					appendStringInfoChar(payload, '=');
 					strcpy_quoted(payload, SPI_getvalue(trigtuple, tupdesc, colno), '\'');
 				}
 
-- 
2.25.0.114.g5b0ca878e0

v2-0003-pqformat-Move-functions-for-type-sending-inline-a.patchtext/x-diff; charset=us-asciiDownload
From f351d204bb6151d5deca2b0cbf3e92ce277323c8 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:50:06 -0800
Subject: [PATCH v2 3/9] pqformat: Move functions for type sending inline, add
 pq_begintypsend_ex().

Combined with the previous commit inlining more functions in
stringinfo this allows many type send functions to be optimized
considerably better, optimizing away the StringInfoData entirely.

The new pq_begintypsend_ex() is useful to avoid over-allocating for
send functions that only output a small amount of data, e.g. int4send
now allocates 9 bytes, instead of 1024.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/libpq/pqformat.h | 52 ++++++++++++++++++++++++++++++++++++
 src/backend/libpq/pqformat.c | 38 --------------------------
 2 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index af31e9caba3..e0dbb783c6a 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -31,6 +31,58 @@ extern void pq_send_ascii_string(StringInfo buf, const char *str);
 extern void pq_sendfloat4(StringInfo buf, float4 f);
 extern void pq_sendfloat8(StringInfo buf, float8 f);
 
+
+/* --------------------------------
+ *		pq_begintypsend		- initialize for constructing a bytea result
+ * --------------------------------
+ */
+static inline void
+pq_begintypsend(StringInfo buf)
+{
+	initStringInfo(buf);
+	/* Reserve four bytes for the bytea length word */
+	appendStringInfoSpaces(buf, 4);
+}
+
+/* --------------------------------
+ *		pq_begintypsend_ex	- like pq_begintypesend, but with length hint
+ *
+ * This can be used over pq_begintypesend if the caller can cheaply determine
+ * how much data will be sent, reducing the initial size of the
+ * StringInfo. The passed in size need not include the overhead of the length
+ * word.
+ * --------------------------------
+ */
+static inline void
+pq_begintypsend_ex(StringInfo buf, int size)
+{
+	initStringInfoEx(buf, size + 4);
+	/* Reserve four bytes for the bytea length word */
+	appendStringInfoSpaces(buf, 4);
+}
+
+
+/* --------------------------------
+ *		pq_endtypsend	- finish constructing a bytea result
+ *
+ * The data buffer is returned as the palloc'd bytea value.  (We expect
+ * that it will be suitably aligned for this because it has been palloc'd.)
+ * We assume the StringInfoData is just a local variable in the caller and
+ * need not be pfree'd.
+ * --------------------------------
+ */
+static inline bytea *
+pq_endtypsend(StringInfo buf)
+{
+	bytea	   *result = (bytea *) buf->data;
+
+	/* Insert correct length into bytea length word */
+	Assert(buf->len >= VARHDRSZ);
+	SET_VARSIZE(result, buf->len);
+
+	return result;
+}
+
 /*
  * Append a [u]int8 to a StringInfo buffer, which already has enough space
  * preallocated.
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index 999252681c2..347a0aa697a 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -319,44 +319,6 @@ pq_endmessage_reuse(StringInfo buf)
 	(void) pq_putmessage(buf->cursor, buf->data, buf->len);
 }
 
-
-/* --------------------------------
- *		pq_begintypsend		- initialize for constructing a bytea result
- * --------------------------------
- */
-void
-pq_begintypsend(StringInfo buf)
-{
-	initStringInfo(buf);
-	/* Reserve four bytes for the bytea length word */
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-}
-
-/* --------------------------------
- *		pq_endtypsend	- finish constructing a bytea result
- *
- * The data buffer is returned as the palloc'd bytea value.  (We expect
- * that it will be suitably aligned for this because it has been palloc'd.)
- * We assume the StringInfoData is just a local variable in the caller and
- * need not be pfree'd.
- * --------------------------------
- */
-bytea *
-pq_endtypsend(StringInfo buf)
-{
-	bytea	   *result = (bytea *) buf->data;
-
-	/* Insert correct length into bytea length word */
-	Assert(buf->len >= VARHDRSZ);
-	SET_VARSIZE(result, buf->len);
-
-	return result;
-}
-
-
 /* --------------------------------
  *		pq_puttextmessage - generate a character set-converted message in one step
  *
-- 
2.25.0.114.g5b0ca878e0

v2-0004-WIP-Annotate-palloc-with-malloc-and-other-compile.patchtext/x-diff; charset=us-asciiDownload
From 03037c6590589d6a10d45aa6b9bd8aa0add2b817 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:53:33 -0800
Subject: [PATCH v2 4/9] WIP: Annotate palloc() with malloc and other compiler
 attributes.

In particularly malloc is useful, allowing the compiler to realize
that the return value does not alias with other data, allowing other
optimizations.

If we were to do this, we'd obviously need to hide these behind
macros to support compilers without those attributes.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/utils/palloc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h
index cc356a63728..a611148be67 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -74,7 +74,7 @@ extern void *MemoryContextAllocZeroAligned(MemoryContext context, Size size);
 extern void *MemoryContextAllocExtended(MemoryContext context,
 										Size size, int flags);
 
-extern void *palloc(Size size);
+extern void *palloc(Size size) __attribute__((malloc, alloc_size(1), assume_aligned(MAXIMUM_ALIGNOF), returns_nonnull, warn_unused_result));
 extern void *palloc0(Size size);
 extern void *palloc_extended(Size size, int flags);
 extern void *repalloc(void *pointer, Size size);
-- 
2.25.0.114.g5b0ca878e0

v2-0005-Move-int4-int8-send-to-use-pq_begintypsend_ex.patchtext/x-diff; charset=us-asciiDownload
From 57f228a71d886bf1ae4cf7e83fc8906dba021317 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:58:55 -0800
Subject: [PATCH v2 5/9] Move {int4,int8}send to use pq_begintypsend_ex.

If we were to introduce pq_begintypsend_ex(), we'd probably want to do
this to quite a few more functions.
---
 src/backend/utils/adt/int.c  | 2 +-
 src/backend/utils/adt/int8.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index 63c59c56b3f..84d22fc5b99 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -305,7 +305,7 @@ int4send(PG_FUNCTION_ARGS)
 	int32		arg1 = PG_GETARG_INT32(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_ex(&buf, 4);
 	pq_sendint32(&buf, arg1);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index abba8f1df04..b258778622d 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -175,7 +175,7 @@ int8send(PG_FUNCTION_ARGS)
 	int64		arg1 = PG_GETARG_INT64(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_ex(&buf, 8);
 	pq_sendint64(&buf, arg1);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
-- 
2.25.0.114.g5b0ca878e0

v2-0006-copy-Use-appendBinaryStringInfoNT-for-sending-bin.patchtext/x-diff; charset=us-asciiDownload
From 18642eec1e616b914fc9c8a98e1bb258ceb8975a Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 13:00:26 -0800
Subject: [PATCH v2 6/9] copy: Use appendBinaryStringInfoNT() for sending
 binary data.

Maintaining the trailing byte is useless overhead.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/commands/copy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e51c181f55c..c12855a304a 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -499,13 +499,13 @@ SendCopyEnd(CopyState cstate)
 static void
 CopySendData(CopyState cstate, const void *databuf, int datasize)
 {
-	appendBinaryStringInfo(cstate->fe_msgbuf, databuf, datasize);
+	appendBinaryStringInfoNT(cstate->fe_msgbuf, databuf, datasize);
 }
 
 static void
 CopySendString(CopyState cstate, const char *str)
 {
-	appendBinaryStringInfo(cstate->fe_msgbuf, str, strlen(str));
+	appendBinaryStringInfoNT(cstate->fe_msgbuf, str, strlen(str));
 }
 
 static void
-- 
2.25.0.114.g5b0ca878e0

v2-0007-wip-make-send-recv-calls-in-printtup.c-copy.c-che.patchtext/x-diff; charset=us-asciiDownload
From ff2892ecd7ceebde13321bade5b9855433da3295 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 2 Jun 2020 16:47:26 -0700
Subject: [PATCH v2 7/9] wip: make send/recv calls in printtup.c & copy.c
 cheaper.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/access/common/printtup.c |  37 ++++-
 src/backend/commands/copy.c          | 208 ++++++++++++++++++++-------
 2 files changed, 189 insertions(+), 56 deletions(-)

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index dd1bac0aa9e..db23bb99c16 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -56,6 +56,15 @@ typedef struct
 	bool		typisvarlena;	/* is it varlena (ie possibly toastable)? */
 	int16		format;			/* format code for this column */
 	FmgrInfo	finfo;			/* Precomputed call info for output fn */
+
+	/* use union with FunctionCallInfoBaseData to guarantee alignment */
+	union
+	{
+		FunctionCallInfoBaseData fcinfo;
+		/* ensure enough space for nargs args is available */
+		char fcinfo_data[SizeForFunctionCallInfo(1)];
+	} fcinfo_data;
+
 } PrinttupAttrInfo;
 
 typedef struct
@@ -355,6 +364,9 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
 							  &thisState->typoutput,
 							  &thisState->typisvarlena);
 			fmgr_info(thisState->typoutput, &thisState->finfo);
+			InitFunctionCallInfoData(thisState->fcinfo_data.fcinfo,
+									 &thisState->finfo, 1, InvalidOid,
+									 NULL, NULL);
 		}
 		else if (format == 1)
 		{
@@ -362,6 +374,9 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
 									&thisState->typsend,
 									&thisState->typisvarlena);
 			fmgr_info(thisState->typsend, &thisState->finfo);
+			InitFunctionCallInfoData(thisState->fcinfo_data.fcinfo,
+									 &thisState->finfo, 1, InvalidOid,
+									 NULL, NULL);
 		}
 		else
 			ereport(ERROR,
@@ -438,11 +453,25 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 		{
 			/* Binary output */
 			bytea	   *outputbytes;
+			int			outputlen;
+			Datum		result;
+			FunctionCallInfo fcinfo = &thisState->fcinfo_data.fcinfo;
 
-			outputbytes = SendFunctionCall(&thisState->finfo, attr);
-			pq_sendint32(buf, VARSIZE(outputbytes) - VARHDRSZ);
-			pq_sendbytes(buf, VARDATA(outputbytes),
-						 VARSIZE(outputbytes) - VARHDRSZ);
+			fcinfo->args[0].value = attr;
+			fcinfo->args[0].isnull = false;
+			result = FunctionCallInvoke(fcinfo);
+
+			/* Check for null result, since caller is clearly not expecting one */
+			if (unlikely(fcinfo->isnull))
+				elog(ERROR, "send function return null");
+
+			outputbytes = DatumGetByteaP(result);
+			outputlen = VARSIZE(outputbytes) - VARHDRSZ;
+
+			Assert(outputlen > 0);
+
+			pq_sendint32(buf, outputlen);
+			pq_sendbytes(buf, VARDATA(outputbytes), outputlen);
 		}
 	}
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c12855a304a..bec1ce51260 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -95,6 +95,37 @@ typedef enum CopyInsertMethod
 	CIM_MULTI_CONDITIONAL		/* use table_multi_insert only if valid */
 } CopyInsertMethod;
 
+typedef struct CopyInAttributeInfo
+{
+	AttrNumber num;
+	const char *name;
+
+	Oid		   typioparam;
+	int32	   typmod;
+
+	FmgrInfo in_finfo;
+	union
+	{
+		FunctionCallInfoBaseData fcinfo;
+		char fcinfo_data[SizeForFunctionCallInfo(3)];
+	} in_fcinfo;
+
+} CopyInAttributeInfo;
+
+typedef struct CopyOutAttributeInfo
+{
+	AttrNumber num;
+	const char *name;
+	int32	   typid;
+
+	FmgrInfo out_finfo;
+	union
+	{
+		FunctionCallInfoBaseData fcinfo;
+		char fcinfo_data[SizeForFunctionCallInfo(1)];
+	} out_fcinfo;
+} CopyOutAttributeInfo;
+
 /*
  * This struct contains all the state variables used throughout a COPY
  * operation. For simplicity, we use the same struct for all variants of COPY,
@@ -169,15 +200,14 @@ typedef struct CopyStateData
 	/*
 	 * Working state for COPY TO
 	 */
-	FmgrInfo   *out_functions;	/* lookup info for output functions */
+	CopyOutAttributeInfo *out_attributes;
 	MemoryContext rowcontext;	/* per-row evaluation context */
 
 	/*
 	 * Working state for COPY FROM
 	 */
 	AttrNumber	num_defaults;
-	FmgrInfo   *in_functions;	/* array of input functions for each attrs */
-	Oid		   *typioparams;	/* array of element types for in_functions */
+	CopyInAttributeInfo *in_attributes;
 	int		   *defmap;			/* array of default att numbers */
 	ExprState **defexprs;		/* array of default att expressions */
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
@@ -368,8 +398,8 @@ static bool CopyReadLineText(CopyState cstate);
 static int	CopyReadAttributesText(CopyState cstate);
 static int	CopyReadAttributesCSV(CopyState cstate);
 static Datum CopyReadBinaryAttribute(CopyState cstate,
-									 int column_no, FmgrInfo *flinfo,
-									 Oid typioparam, int32 typmod,
+									 int column_no,
+									 CopyInAttributeInfo *attr,
 									 bool *isnull);
 static void CopyAttributeOutText(CopyState cstate, char *string);
 static void CopyAttributeOutCSV(CopyState cstate, char *string,
@@ -2027,23 +2057,34 @@ CopyTo(CopyState cstate)
 	cstate->fe_msgbuf = makeStringInfo();
 
 	/* Get info about the columns we need to process. */
-	cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
+	cstate->out_attributes =
+		(CopyOutAttributeInfo *) palloc(num_phys_attrs * sizeof(CopyOutAttributeInfo));
 	foreach(cur, cstate->attnumlist)
 	{
 		int			attnum = lfirst_int(cur);
+		CopyOutAttributeInfo *attr = &cstate->out_attributes[attnum - 1];
+		Form_pg_attribute pgatt;
 		Oid			out_func_oid;
 		bool		isvarlena;
-		Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
+
+		pgatt = TupleDescAttr(tupDesc, attnum - 1);
+		attr->num = attnum;
+		attr->name = NameStr(pgatt->attname);
+		attr->typid = pgatt->atttypid;
 
 		if (cstate->binary)
-			getTypeBinaryOutputInfo(attr->atttypid,
+			getTypeBinaryOutputInfo(attr->typid,
 									&out_func_oid,
 									&isvarlena);
 		else
-			getTypeOutputInfo(attr->atttypid,
+			getTypeOutputInfo(attr->typid,
 							  &out_func_oid,
 							  &isvarlena);
-		fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+
+		fmgr_info(out_func_oid, &attr->out_finfo);
+		InitFunctionCallInfoData(attr->out_fcinfo.fcinfo, &attr->out_finfo,
+								 1, InvalidOid,
+								 NULL, NULL);
 	}
 
 	/*
@@ -2156,7 +2197,7 @@ static void
 CopyOneRowTo(CopyState cstate, TupleTableSlot *slot)
 {
 	bool		need_delim = false;
-	FmgrInfo   *out_functions = cstate->out_functions;
+	CopyOutAttributeInfo *out_attributes = cstate->out_attributes;
 	MemoryContext oldcontext;
 	ListCell   *cur;
 	char	   *string;
@@ -2176,6 +2217,8 @@ CopyOneRowTo(CopyState cstate, TupleTableSlot *slot)
 	foreach(cur, cstate->attnumlist)
 	{
 		int			attnum = lfirst_int(cur);
+		CopyOutAttributeInfo *attr = &out_attributes[attnum - 1];
+		FunctionCallInfo fcinfo = &attr->out_fcinfo.fcinfo;
 		Datum		value = slot->tts_values[attnum - 1];
 		bool		isnull = slot->tts_isnull[attnum - 1];
 
@@ -2197,8 +2240,19 @@ CopyOneRowTo(CopyState cstate, TupleTableSlot *slot)
 		{
 			if (!cstate->binary)
 			{
-				string = OutputFunctionCall(&out_functions[attnum - 1],
-											value);
+				Datum		result;
+
+				fcinfo->args[0].value = value;
+				fcinfo->args[0].isnull = false;
+
+				result = FunctionCallInvoke(fcinfo);
+
+				/* Check for null result, since caller is clearly not expecting one */
+				if (unlikely(fcinfo->isnull))
+					elog(ERROR, "send function return null");
+
+				string = DatumGetCString(result);
+
 				if (cstate->csv_mode)
 					CopyAttributeOutCSV(cstate, string,
 										cstate->force_quote_flags[attnum - 1],
@@ -2208,13 +2262,23 @@ CopyOneRowTo(CopyState cstate, TupleTableSlot *slot)
 			}
 			else
 			{
+				int			outputlen;
+				Datum		result;
 				bytea	   *outputbytes;
 
-				outputbytes = SendFunctionCall(&out_functions[attnum - 1],
-											   value);
-				CopySendInt32(cstate, VARSIZE(outputbytes) - VARHDRSZ);
-				CopySendData(cstate, VARDATA(outputbytes),
-							 VARSIZE(outputbytes) - VARHDRSZ);
+				fcinfo->args[0].value = value;
+				fcinfo->args[0].isnull = false;
+				result = FunctionCallInvoke(fcinfo);
+
+				/* Check for null result, since caller is clearly not expecting one */
+				if (unlikely(fcinfo->isnull))
+					elog(ERROR, "send function return null");
+
+				outputbytes = DatumGetByteaP(result);
+				outputlen = VARSIZE(outputbytes) - VARHDRSZ;
+
+				CopySendInt32(cstate, outputlen);
+				CopySendData(cstate, VARDATA(outputbytes), outputlen);
 			}
 		}
 	}
@@ -3344,8 +3408,7 @@ BeginCopyFrom(ParseState *pstate,
 	TupleDesc	tupDesc;
 	AttrNumber	num_phys_attrs,
 				num_defaults;
-	FmgrInfo   *in_functions;
-	Oid		   *typioparams;
+	CopyInAttributeInfo *in_attributes;
 	int			attnum;
 	Oid			in_func_oid;
 	int		   *defmap;
@@ -3386,30 +3449,39 @@ BeginCopyFrom(ParseState *pstate,
 	 * the input function), and info about defaults and constraints. (Which
 	 * input function we use depends on text/binary format choice.)
 	 */
-	in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
-	typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));
+	in_attributes = (CopyInAttributeInfo * ) palloc0(num_phys_attrs * sizeof(CopyInAttributeInfo));
+
 	defmap = (int *) palloc(num_phys_attrs * sizeof(int));
 	defexprs = (ExprState **) palloc(num_phys_attrs * sizeof(ExprState *));
 
 	for (attnum = 1; attnum <= num_phys_attrs; attnum++)
 	{
-		Form_pg_attribute att = TupleDescAttr(tupDesc, attnum - 1);
+		CopyInAttributeInfo *attr = &in_attributes[attnum - 1];
+		Form_pg_attribute pgatt = TupleDescAttr(tupDesc, attnum - 1);
 
 		/* We don't need info for dropped attributes */
-		if (att->attisdropped)
+		if (pgatt->attisdropped)
 			continue;
 
+		attr->num = attnum;
+		attr->typmod = pgatt->atttypmod;
+		attr->name = NameStr(pgatt->attname);
+
 		/* Fetch the input function and typioparam info */
 		if (cstate->binary)
-			getTypeBinaryInputInfo(att->atttypid,
-								   &in_func_oid, &typioparams[attnum - 1]);
+			getTypeBinaryInputInfo(pgatt->atttypid,
+								   &in_func_oid, &attr->typioparam);
 		else
-			getTypeInputInfo(att->atttypid,
-							 &in_func_oid, &typioparams[attnum - 1]);
-		fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+			getTypeInputInfo(pgatt->atttypid,
+							 &in_func_oid, &attr->typioparam);
+		attr->typmod = pgatt->atttypmod;
+
+		fmgr_info(in_func_oid, &attr->in_finfo);
+		InitFunctionCallInfoData(attr->in_fcinfo.fcinfo, &attr->in_finfo, 3,
+								 InvalidOid, NULL, NULL);
 
 		/* Get default info if needed */
-		if (!list_member_int(cstate->attnumlist, attnum) && !att->attgenerated)
+		if (!list_member_int(cstate->attnumlist, attnum) && !pgatt->attgenerated)
 		{
 			/* attribute is NOT to be copied from input */
 			/* use default value if one exists */
@@ -3446,8 +3518,7 @@ BeginCopyFrom(ParseState *pstate,
 	}
 
 	/* We keep those variables in cstate. */
-	cstate->in_functions = in_functions;
-	cstate->typioparams = typioparams;
+	cstate->in_attributes = in_attributes;
 	cstate->defmap = defmap;
 	cstate->defexprs = defexprs;
 	cstate->volatile_defexprs = volatile_defexprs;
@@ -3639,8 +3710,7 @@ NextCopyFrom(CopyState cstate, ExprContext *econtext,
 	AttrNumber	num_phys_attrs,
 				attr_count,
 				num_defaults = cstate->num_defaults;
-	FmgrInfo   *in_functions = cstate->in_functions;
-	Oid		   *typioparams = cstate->typioparams;
+	CopyInAttributeInfo *in_attributes = cstate->in_attributes;
 	int			i;
 	int		   *defmap = cstate->defmap;
 	ExprState **defexprs = cstate->defexprs;
@@ -3678,13 +3748,14 @@ NextCopyFrom(CopyState cstate, ExprContext *econtext,
 		{
 			int			attnum = lfirst_int(cur);
 			int			m = attnum - 1;
-			Form_pg_attribute att = TupleDescAttr(tupDesc, m);
+			CopyInAttributeInfo *attr = &in_attributes[m];
+			FunctionCallInfo fcinfo = &attr->in_fcinfo.fcinfo;
 
 			if (fieldno >= fldct)
 				ereport(ERROR,
 						(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
 						 errmsg("missing data for column \"%s\"",
-								NameStr(att->attname))));
+								attr->name)));
 			string = field_strings[fieldno++];
 
 			if (cstate->convert_select_flags &&
@@ -3718,14 +3789,39 @@ NextCopyFrom(CopyState cstate, ExprContext *econtext,
 				}
 			}
 
-			cstate->cur_attname = NameStr(att->attname);
+			cstate->cur_attname = attr->name;
 			cstate->cur_attval = string;
-			values[m] = InputFunctionCall(&in_functions[m],
-										  string,
-										  typioparams[m],
-										  att->atttypmod);
-			if (string != NULL)
-				nulls[m] = false;
+
+			if (string == NULL && fcinfo->flinfo->fn_strict)
+				nulls[m] = true;
+			else
+			{
+				fcinfo->args[0].value = CStringGetDatum(string);
+				fcinfo->args[0].isnull = false;
+				fcinfo->args[1].value = ObjectIdGetDatum(attr->typioparam);
+				fcinfo->args[1].isnull = false;
+				fcinfo->args[2].value = Int32GetDatum(attr->typmod);
+				fcinfo->args[2].isnull = false;
+
+				values[m] = FunctionCallInvoke(fcinfo);
+
+				/* Should get null result if and only if str is NULL */
+				if (string == NULL)
+				{
+					if (unlikely(!fcinfo->isnull))
+						elog(ERROR, "input function %u returned non-NULL",
+							 fcinfo->flinfo->fn_oid);
+				}
+				else
+				{
+					if (unlikely(fcinfo->isnull))
+						elog(ERROR, "input function %u returned NULL",
+							 fcinfo->flinfo->fn_oid);
+				}
+
+				nulls[m] = string == NULL;
+			}
+
 			cstate->cur_attname = NULL;
 			cstate->cur_attval = NULL;
 		}
@@ -3787,9 +3883,7 @@ NextCopyFrom(CopyState cstate, ExprContext *econtext,
 			i++;
 			values[m] = CopyReadBinaryAttribute(cstate,
 												i,
-												&in_functions[m],
-												typioparams[m],
-												att->atttypmod,
+												&in_attributes[m],
 												&nulls[m]);
 			cstate->cur_attname = NULL;
 		}
@@ -4714,13 +4808,13 @@ endfield:
  * Read a binary attribute
  */
 static Datum
-CopyReadBinaryAttribute(CopyState cstate,
-						int column_no, FmgrInfo *flinfo,
-						Oid typioparam, int32 typmod,
+CopyReadBinaryAttribute(CopyState cstate, int column_no,
+						CopyInAttributeInfo *attr,
 						bool *isnull)
 {
 	int32		fld_size;
 	Datum		result;
+	FunctionCallInfo fcinfo = &attr->in_fcinfo.fcinfo;
 
 	if (!CopyGetInt32(cstate, &fld_size))
 		ereport(ERROR,
@@ -4729,7 +4823,7 @@ CopyReadBinaryAttribute(CopyState cstate,
 	if (fld_size == -1)
 	{
 		*isnull = true;
-		return ReceiveFunctionCall(flinfo, NULL, typioparam, typmod);
+		return ReceiveFunctionCall(fcinfo->flinfo, NULL, attr->typioparam, attr->typmod);
 	}
 	if (fld_size < 0)
 		ereport(ERROR,
@@ -4750,8 +4844,18 @@ CopyReadBinaryAttribute(CopyState cstate,
 	cstate->attribute_buf.data[fld_size] = '\0';
 
 	/* Call the column type's binary input converter */
-	result = ReceiveFunctionCall(flinfo, &cstate->attribute_buf,
-								 typioparam, typmod);
+	fcinfo->args[0].value = PointerGetDatum(&cstate->attribute_buf);
+	fcinfo->args[0].isnull = false;
+	fcinfo->args[1].value = ObjectIdGetDatum(attr->typioparam);
+	fcinfo->args[1].isnull = false;
+	fcinfo->args[2].value = Int32GetDatum(attr->typmod);
+	fcinfo->args[2].isnull = false;
+
+	result = FunctionCallInvoke(fcinfo);
+
+	if (unlikely(fcinfo->isnull))
+		elog(ERROR, "receive function %u returned NULL",
+			 fcinfo->flinfo->fn_oid);
 
 	/* Trouble if it didn't eat the whole buffer */
 	if (cstate->attribute_buf.cursor != cstate->attribute_buf.len)
-- 
2.25.0.114.g5b0ca878e0

v2-0008-inline-pq_sendbytes-stop-maintaining-trailing-nul.patchtext/x-diff; charset=us-asciiDownload
From 6a569ceade530fcbc3f4f18bae003d118239e169 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 2 Jun 2020 17:48:04 -0700
Subject: [PATCH v2 8/9] inline pq_sendbytes, stop maintaining trailing null
 byte.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/libpq/pqformat.h | 13 ++++++++++++-
 src/backend/libpq/pqformat.c | 11 -----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index e0dbb783c6a..6af153b7f77 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -22,7 +22,6 @@ extern void pq_beginmessage_reuse(StringInfo buf, char msgtype);
 extern void pq_endmessage(StringInfo buf);
 extern void pq_endmessage_reuse(StringInfo buf);
 
-extern void pq_sendbytes(StringInfo buf, const char *data, int datalen);
 extern void pq_sendcountedtext(StringInfo buf, const char *str, int slen,
 							   bool countincludesself);
 extern void pq_sendtext(StringInfo buf, const char *str, int slen);
@@ -215,6 +214,18 @@ pq_sendbyte(StringInfo buf, uint8 byt)
 	pq_sendint8(buf, byt);
 }
 
+static inline void
+pq_sendbytes(StringInfo buf, const char *data, int datalen)
+{
+	/*
+	 * FIXME: used to say:
+	 * use variant that maintains a trailing null-byte, out of caution but
+	 * that doesn't make much sense to me, and proves to be a performance
+	 * issue.
+	 */
+	appendBinaryStringInfoNT(buf, data, datalen);
+}
+
 /*
  * Append a binary integer to a StringInfo buffer
  *
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index 347a0aa697a..377335a94ae 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -117,17 +117,6 @@ pq_beginmessage_reuse(StringInfo buf, char msgtype)
 	buf->cursor = msgtype;
 }
 
-/* --------------------------------
- *		pq_sendbytes	- append raw data to a StringInfo buffer
- * --------------------------------
- */
-void
-pq_sendbytes(StringInfo buf, const char *data, int datalen)
-{
-	/* use variant that maintains a trailing null-byte, out of caution */
-	appendBinaryStringInfo(buf, data, datalen);
-}
-
 /* --------------------------------
  *		pq_sendcountedtext - append a counted text string (with character set conversion)
  *
-- 
2.25.0.114.g5b0ca878e0

v2-0009-heavy-wip-Allow-string-buffer-reuse-in-send-funct.patchtext/x-diff; charset=us-asciiDownload
From 7b9913519e51a683059193261c8630eb953b3b99 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 2 Jun 2020 17:48:43 -0700
Subject: [PATCH v2 9/9] heavy-wip: Allow string buffer reuse in send
 functions.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/libpq/pqformat.h         | 30 +++++++++++++---------------
 src/backend/access/common/printtup.c |  9 +++++++--
 src/backend/commands/copy.c          |  5 ++++-
 src/backend/utils/adt/int.c          | 10 ++++++----
 src/backend/utils/adt/int8.c         |  9 +++++----
 src/backend/utils/adt/varlena.c      | 10 ++++++----
 src/backend/utils/fmgr/fmgr.c        | 16 ++++++++++++++-
 7 files changed, 57 insertions(+), 32 deletions(-)

diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index 6af153b7f77..c8ac781c726 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -39,27 +39,25 @@ static inline void
 pq_begintypsend(StringInfo buf)
 {
 	initStringInfo(buf);
-	/* Reserve four bytes for the bytea length word */
-	appendStringInfoSpaces(buf, 4);
+
+	/*
+	 * Reserve four bytes for the bytea length word.  We don't need to fill
+	 * them with anything (pq_endtypsend will do that), and this function is
+	 * enough of a hot spot that it's worth cheating to save some cycles. Note
+	 * in particular that we don't bother to guarantee that the buffer is
+	 * null-terminated.
+	 */
+	Assert(buf->maxlen > 4);
+	buf->len = 4;
 }
 
-/* --------------------------------
- *		pq_begintypsend_ex	- like pq_begintypesend, but with length hint
- *
- * This can be used over pq_begintypesend if the caller can cheaply determine
- * how much data will be sent, reducing the initial size of the
- * StringInfo. The passed in size need not include the overhead of the length
- * word.
- * --------------------------------
- */
 static inline void
-pq_begintypsend_ex(StringInfo buf, int size)
+pq_begintypsend_res(StringInfo buf)
 {
-	initStringInfoEx(buf, size + 4);
-	/* Reserve four bytes for the bytea length word */
-	appendStringInfoSpaces(buf, 4);
-}
+	Assert(buf && buf->data && buf->len == 0);
 
+	buf->len = 4;
+}
 
 /* --------------------------------
  *		pq_endtypsend	- finish constructing a bytea result
diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index db23bb99c16..7065dc2110b 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -76,6 +76,7 @@ typedef struct
 	int			nattrs;
 	PrinttupAttrInfo *myinfo;	/* Cached info about each attr */
 	StringInfoData buf;			/* output buffer (*not* in tmpcontext) */
+	StringInfoData fieldbuf;	/*  */
 	MemoryContext tmpcontext;	/* Memory context for per-row workspace */
 } DR_printtup;
 
@@ -148,6 +149,8 @@ printtup_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 	 */
 	initStringInfo(&myState->buf);
 
+	initStringInfo(&myState->fieldbuf);
+
 	/*
 	 * Create a temporary memory context that we can reset once per row to
 	 * recover palloc'd memory.  This avoids any problems with leaks inside
@@ -366,7 +369,7 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
 			fmgr_info(thisState->typoutput, &thisState->finfo);
 			InitFunctionCallInfoData(thisState->fcinfo_data.fcinfo,
 									 &thisState->finfo, 1, InvalidOid,
-									 NULL, NULL);
+									 (Node *) &myState->fieldbuf, NULL);
 		}
 		else if (format == 1)
 		{
@@ -376,7 +379,7 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
 			fmgr_info(thisState->typsend, &thisState->finfo);
 			InitFunctionCallInfoData(thisState->fcinfo_data.fcinfo,
 									 &thisState->finfo, 1, InvalidOid,
-									 NULL, NULL);
+									 (Node *) &myState->fieldbuf, NULL);
 		}
 		else
 			ereport(ERROR,
@@ -396,6 +399,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 	DR_printtup *myState = (DR_printtup *) self;
 	MemoryContext oldcontext;
 	StringInfo	buf = &myState->buf;
+	StringInfo	fieldbuf = &myState->fieldbuf;
 	int			natts = typeinfo->natts;
 	int			i;
 
@@ -472,6 +476,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 
 			pq_sendint32(buf, outputlen);
 			pq_sendbytes(buf, VARDATA(outputbytes), outputlen);
+			resetStringInfo(fieldbuf);
 		}
 	}
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index bec1ce51260..45945c2f67f 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1902,6 +1902,7 @@ BeginCopyTo(ParseState *pstate,
 
 	cstate = BeginCopy(pstate, false, rel, query, queryRelId, attnamelist,
 					   options);
+	initStringInfo(&cstate->attribute_buf);
 	oldcontext = MemoryContextSwitchTo(cstate->copycontext);
 
 	if (pipe)
@@ -2084,7 +2085,7 @@ CopyTo(CopyState cstate)
 		fmgr_info(out_func_oid, &attr->out_finfo);
 		InitFunctionCallInfoData(attr->out_fcinfo.fcinfo, &attr->out_finfo,
 								 1, InvalidOid,
-								 NULL, NULL);
+								 (Node *) &cstate->attribute_buf, NULL);
 	}
 
 	/*
@@ -2201,6 +2202,7 @@ CopyOneRowTo(CopyState cstate, TupleTableSlot *slot)
 	MemoryContext oldcontext;
 	ListCell   *cur;
 	char	   *string;
+	StringInfo	attribute_buf = &cstate->attribute_buf;
 
 	MemoryContextReset(cstate->rowcontext);
 	oldcontext = MemoryContextSwitchTo(cstate->rowcontext);
@@ -2279,6 +2281,7 @@ CopyOneRowTo(CopyState cstate, TupleTableSlot *slot)
 
 				CopySendInt32(cstate, outputlen);
 				CopySendData(cstate, VARDATA(outputbytes), outputlen);
+				resetStringInfo(attribute_buf);
 			}
 		}
 	}
diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index 84d22fc5b99..811527b0eb0 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -303,11 +303,13 @@ Datum
 int4send(PG_FUNCTION_ARGS)
 {
 	int32		arg1 = PG_GETARG_INT32(0);
-	StringInfoData buf;
+	StringInfo	buf;
 
-	pq_begintypsend_ex(&buf, 4);
-	pq_sendint32(&buf, arg1);
-	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
+	buf = (StringInfo ) fcinfo->context;
+
+	pq_begintypsend_res(buf);
+	pq_sendint32(buf, arg1);
+	PG_RETURN_BYTEA_P(pq_endtypsend(buf));
 }
 
 
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index b258778622d..dad0663dc58 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -173,11 +173,12 @@ Datum
 int8send(PG_FUNCTION_ARGS)
 {
 	int64		arg1 = PG_GETARG_INT64(0);
-	StringInfoData buf;
+	StringInfo	buf;
 
-	pq_begintypsend_ex(&buf, 8);
-	pq_sendint64(&buf, arg1);
-	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
+	buf = (StringInfo ) fcinfo->context;
+	pq_begintypsend_res(buf);
+	pq_sendint64(buf, arg1);
+	PG_RETURN_BYTEA_P(pq_endtypsend(buf));
 }
 
 
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index f3c1a63455a..29edb03b49a 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -582,11 +582,13 @@ Datum
 textsend(PG_FUNCTION_ARGS)
 {
 	text	   *t = PG_GETARG_TEXT_PP(0);
-	StringInfoData buf;
+	StringInfo	buf = (StringInfo ) fcinfo->context;
 
-	pq_begintypsend(&buf);
-	pq_sendtext(&buf, VARDATA_ANY(t), VARSIZE_ANY_EXHDR(t));
-	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
+	Assert(fcinfo->context);
+
+	pq_begintypsend_res(buf);
+	pq_sendtext(buf, VARDATA_ANY(t), VARSIZE_ANY_EXHDR(t));
+	PG_RETURN_BYTEA_P(pq_endtypsend(buf));
 }
 
 
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 03c614b234a..13483b09881 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -1637,7 +1637,21 @@ ReceiveFunctionCall(FmgrInfo *flinfo, StringInfo buf,
 bytea *
 SendFunctionCall(FmgrInfo *flinfo, Datum val)
 {
-	return DatumGetByteaP(FunctionCall1(flinfo, val));
+	StringInfoData buf;
+	Datum result;
+	LOCAL_FCINFO(fcinfo, 1);
+
+	initStringInfo(&buf);
+
+	InitFunctionCallInfoData(*fcinfo, flinfo, 1, InvalidOid, (Node *) &buf, NULL);
+	fcinfo->args[0].value = val;
+	fcinfo->args[0].isnull = false;
+	result = FunctionCallInvoke(fcinfo);
+
+	if (fcinfo->isnull)
+		elog(ERROR, "function %u returned NULL", flinfo->fn_oid);
+
+	return DatumGetByteaP(result);
 }
 
 /*
-- 
2.25.0.114.g5b0ca878e0

#12Robert Haas
robertmhaas@gmail.com
In reply to: Andres Freund (#11)
Re: Why is pq_begintypsend so slow?

On Tue, Jun 2, 2020 at 9:56 PM Andres Freund <andres@anarazel.de> wrote:

The biggest problem after that is that we waste a lot of time memcpying
stuff around repeatedly. There is:
1) send function: datum -> per datum stringinfo
2) printtup: per datum stringinfo -> per row stringinfo
3) socket_putmessage: per row stringinfo -> PqSendBuffer
4) send(): PqSendBuffer -> kernel buffer

It's obviously hard to avoid 1) and 4) in the common case, but the
number of other copies seem pretty clearly excessive.

I too have seen recent benchmarking data where this was a big problem.
Basically, you need a workload where the server doesn't have much or
any actual query processing to do, but is just returning a lot of
stuff to a really fast client - e.g. a locally connected client.
That's not necessarily the most common case but, if you have it, all
this extra copying is really pretty expensive.

My first thought was to wonder about changing all of our send/output
functions to write into a buffer passed as an argument rather than
returning something which we then have to copy into a different
buffer, but that would be a somewhat painful change, so it is probably
better to first pursue the idea of getting rid of some of the other
copies that happen in more centralized places (e.g. printtup). I
wonder if we could replace the whole
pq_beginmessage...()/pq_send....()/pq_endmessage...() system with
something a bit better-designed. For instance, suppose we get rid of
the idea that the caller supplies the buffer, and we move the
responsibility for error recovery into the pqcomm layer. So you do
something like:

my_message = xyz_beginmessage('D');
xyz_sendint32(my_message, 42);
xyz_endmessage(my_message);

Maybe what happens here under the hood is we keep a pool of free
message buffers sitting around, and you just grab one and put your
data into it. When you end the message we add it to a list of used
message buffers that are waiting to be sent, and once we send the data
it goes back on the free list. If an error occurs after
xyz_beginmessage() and before xyz_endmessage(), we put the buffer back
on the free list. That would allow us to merge (2) and (3) into a
single copy. To go further, we could allow send/output functions to
opt in to receiving a message buffer rather than returning a value,
and then we could get rid of (1) for types that participate. (4) seems
unavoidable AFAIK.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#13Andres Freund
andres@anarazel.de
In reply to: Robert Haas (#12)
Re: Why is pq_begintypsend so slow?

Hi,

On 2020-06-03 11:30:42 -0400, Robert Haas wrote:

I too have seen recent benchmarking data where this was a big problem.
Basically, you need a workload where the server doesn't have much or
any actual query processing to do, but is just returning a lot of
stuff to a really fast client - e.g. a locally connected client.
That's not necessarily the most common case but, if you have it, all
this extra copying is really pretty expensive.

Even when the query actually is doing something, it's still quite
possible to get the memcpies to be be measurable (say > 10% of
cycles). Obviously not in a huge aggregating query. Even in something
like pgbench -M prepared -S, which is obviously spending most of its
cycles elsewhere, the patches upthread improve throughput by ~1.5% (and
that's not eliding several unnecessary copies).

My first thought was to wonder about changing all of our send/output
functions to write into a buffer passed as an argument rather than
returning something which we then have to copy into a different
buffer, but that would be a somewhat painful change, so it is probably
better to first pursue the idea of getting rid of some of the other
copies that happen in more centralized places (e.g. printtup).

For those I think the allocator overhead is the bigger issue than the
memcpy itself. I wonder how much we could transparently hide in
pq_begintypsend()/pq_endtypsend().

I
wonder if we could replace the whole
pq_beginmessage...()/pq_send....()/pq_endmessage...() system with
something a bit better-designed. For instance, suppose we get rid of
the idea that the caller supplies the buffer, and we move the
responsibility for error recovery into the pqcomm layer. So you do
something like:

my_message = xyz_beginmessage('D');
xyz_sendint32(my_message, 42);
xyz_endmessage(my_message);

Maybe what happens here under the hood is we keep a pool of free
message buffers sitting around, and you just grab one and put your
data into it.

Why do we need multiple buffers? ISTM we don't want to just send
messages at endmsg() time, because that implies unnecessary syscall
overhead. Nor do we want to imply the overhead of the copy from the
message buffer to the network buffer.

To me that seems to imply that the best approach would be to have
PqSendBuffer be something stringbuffer like, and have pg_beginmessage()
record the starting position of the current message somewhere
(->cursor?). When an error is thrown, we reset the position to be where
the in-progress message would have begun.

I've previously outlined a slightly more complicated scheme, where we
have "proxy" stringinfos that point into another stringinfo, instead of
their own buffer. And know how to resize the "outer" buffer when
needed. That'd have some advantages, but I'm not sure it's really
needed.

There's some disadvantages with what I describe above, in particular
when dealing with send() sending only parts of our network buffer. We
couldn't cheaply reuse the already sent memory in that case.

I've before wondered / suggested that we should have StringInfos not
insist on having one consecutive buffer (which obviously implies needing
to copy contents when growing). Instead it should have a list of buffers
containing chunks of the data, and never copy contents around while the
string is being built. We'd only allocate a buffer big enough for all
data when the caller actually wants to have all the resulting data in
one string (rather than using an API that can iterate over chunks).

For the network buffer case that'd allow us to reuse the earlier buffers
even in the "partial send" case. And more generally it'd allow us to be
less wasteful with buffer sizes, and perhaps even have a small "inline"
buffer inside StringInfoData avoiding unnecessary memory allocations in
a lot of cases where the common case is only a small amount of data
being used. And I think the overhead while appending data to such a
stringinfo should be neglegible, because it'd just require the exact
same checks we already have to do for enlargeStringInfo().

(4) seems unavoidable AFAIK.

Not entirely. Linux can do zero-copy sends, but it does require somewhat
complicated black magic rituals. Including more complex buffer
management for the application, because the memory containing the
to-be-sent data cannot be reused until the kernel notifies that it's
done with the buffer.

See https://www.kernel.org/doc/html/latest/networking/msg_zerocopy.html

That might be something worth pursuing in the future (since it, I think,
basically avoids spending any cpu cycles on copying data around in the
happy path, relying on DMA instead), but I think for now there's much
bigger fish to fry.

I am hoping that somebody will write a nicer abstraction for zero-copy
sends using io_uring. avoiding the need of a separate completion queue,
by simply only signalling completion for the sendmsg operation once the
buffer isn't needed anymore. There's no corresponding completion logic
for normal sendmsg() calls, so it makes sense that something had to be
invented before something like io_uring existed.

Greetings,

Andres Freund

#14Robert Haas
robertmhaas@gmail.com
In reply to: Andres Freund (#13)
Re: Why is pq_begintypsend so slow?

On Wed, Jun 3, 2020 at 2:10 PM Andres Freund <andres@anarazel.de> wrote:

Why do we need multiple buffers? ISTM we don't want to just send
messages at endmsg() time, because that implies unnecessary syscall
overhead. Nor do we want to imply the overhead of the copy from the
message buffer to the network buffer.

It would only matter if there are multiple messages being constructed
at the same time, and that's probably not common, but maybe there's
some way it can happen. It doesn't seem like it really costs anything
to allow for it, and it might be useful sometime. For instance,
consider your idea of using Linux black magic to do zero-copy sends.
Now you either need multiple buffers, or you need one big buffer that
you can recycle a bit at a time.

To me that seems to imply that the best approach would be to have
PqSendBuffer be something stringbuffer like, and have pg_beginmessage()
record the starting position of the current message somewhere
(->cursor?). When an error is thrown, we reset the position to be where
the in-progress message would have begun.

Yeah, I thought about that, but then how you detect the case where two
different people try to undertake message construction at the same
time?

Like, with the idea I was proposing, you could still decide to limit
yourself to 1 buffer at the same time, and just elog() if someone
tries to allocate a second buffer when you've already reached the
maximum number of allocated buffers (i.e. one). But if you just have
one buffer in a global variable and everybody writes into it, you
might not notice if some unrelated code writes data into that buffer
in the middle of someone else's message construction. Doing it the way
I proposed, writing data requires passing a buffer pointer, so you can
be sure that somebody had to get the buffer from somewhere... and any
rules you want to enforce can be enforced at that point.

I've before wondered / suggested that we should have StringInfos not
insist on having one consecutive buffer (which obviously implies needing
to copy contents when growing). Instead it should have a list of buffers
containing chunks of the data, and never copy contents around while the
string is being built. We'd only allocate a buffer big enough for all
data when the caller actually wants to have all the resulting data in
one string (rather than using an API that can iterate over chunks).

It's a thought. I doubt it's worth it for small amounts of data, but
for large amounts it might be. On the other hand, a better idea still
might be to size the buffer correctly from the start...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#15Andres Freund
andres@anarazel.de
In reply to: Robert Haas (#14)
Re: Why is pq_begintypsend so slow?

Hi,

On 2020-06-09 11:46:09 -0400, Robert Haas wrote:

On Wed, Jun 3, 2020 at 2:10 PM Andres Freund <andres@anarazel.de> wrote:

Why do we need multiple buffers? ISTM we don't want to just send
messages at endmsg() time, because that implies unnecessary syscall
overhead. Nor do we want to imply the overhead of the copy from the
message buffer to the network buffer.

It would only matter if there are multiple messages being constructed
at the same time, and that's probably not common, but maybe there's
some way it can happen.

ISTM that it'd be pretty broken if it could happen. We cannot have two
different parts of the system send messages to the client
independently. The protocol is pretty stateful...

To me that seems to imply that the best approach would be to have
PqSendBuffer be something stringbuffer like, and have pg_beginmessage()
record the starting position of the current message somewhere
(->cursor?). When an error is thrown, we reset the position to be where
the in-progress message would have begun.

Yeah, I thought about that, but then how you detect the case where two
different people try to undertake message construction at the same
time?

Set a boolean and assert out if one already is in progress? We'd need
some state to know where to reset the position to on error anyway.

Like, with the idea I was proposing, you could still decide to limit
yourself to 1 buffer at the same time, and just elog() if someone
tries to allocate a second buffer when you've already reached the
maximum number of allocated buffers (i.e. one). But if you just have
one buffer in a global variable and everybody writes into it, you
might not notice if some unrelated code writes data into that buffer
in the middle of someone else's message construction. Doing it the way
I proposed, writing data requires passing a buffer pointer, so you can
be sure that somebody had to get the buffer from somewhere... and any
rules you want to enforce can be enforced at that point.

I'd hope that we'd encapsulate the buffer management into file local
variables in pqcomm.c or such, and that code outside of that cannot
access the out buffer directly without using the appropriate helpers.

I've before wondered / suggested that we should have StringInfos not
insist on having one consecutive buffer (which obviously implies needing
to copy contents when growing). Instead it should have a list of buffers
containing chunks of the data, and never copy contents around while the
string is being built. We'd only allocate a buffer big enough for all
data when the caller actually wants to have all the resulting data in
one string (rather than using an API that can iterate over chunks).

It's a thought. I doubt it's worth it for small amounts of data, but
for large amounts it might be. On the other hand, a better idea still
might be to size the buffer correctly from the start...

I think those are complimentary. I do agree that's it's useful to size
stringinfos more appropriately immediately (there's an upthread patch
adding a version of initStringInfo() that does so, quite useful for
small stringinfos in particular). But there's enough cases where that's
not really knowable ahead of time that I think it'd be quite useful to
have support for the type of buffer I describe above.

Greetings,

Andres Freund

#16Robert Haas
robertmhaas@gmail.com
In reply to: Andres Freund (#15)
Re: Why is pq_begintypsend so slow?

On Tue, Jun 9, 2020 at 3:23 PM Andres Freund <andres@anarazel.de> wrote:

ISTM that it'd be pretty broken if it could happen. We cannot have two
different parts of the system send messages to the client
independently. The protocol is pretty stateful...

There's a difference between building messages concurrently and
sending them concurrently.

Set a boolean and assert out if one already is in progress? We'd need
some state to know where to reset the position to on error anyway.

Sure, that's basically just different notation for the same thing. I
might prefer my notation over yours, but you might prefer the reverse.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#17Robert Haas
robertmhaas@gmail.com
In reply to: Andres Freund (#11)
Re: Why is pq_begintypsend so slow?

On Tue, Jun 2, 2020 at 9:56 PM Andres Freund <andres@anarazel.de> wrote:

I don't know what the best non-gross solution for the overhead of the
out/send functions is. There seems to be at least the following
major options (and a lots of variants thereof):

1) Just continue to incur significant overhead for every datum
2) Accept the uglyness of passing in a buffer via
FunctionCallInfo->context. Change nearly all in-core send functions
over to that.
3) Pass string buffer through an new INTERNAL argument to send/output
function, allow both old/new style send functions. Use a wrapper
function to adapt the "old style" to the "new style".
4) Like 3, but make the new argument optional, and use ad-hoc
stringbuffer if not provided. I don't like the unnecessary branches
this adds.

I ran into this problem in another context today while poking at some
pg_basebackup stuff. There's another way of solving this problem which
I think we should consider: just get rid of the per-row stringinfo and
push the bytes directly from wherever they are into PqSendBuffer. Once
we start doing this, we can't error out, because internal_flush()
might've been called, sending a partial message to the client. Trying
to now switch to sending an ErrorResponse will break protocol sync.
But it seems possible to avoid that. Just call all of the output
functions first, and also do any required encoding conversions
(pq_sendcountedtext -> pg_server_to_client). Then, do a bunch of
pq_putbytes() calls to shove the message out -- there's the small
matter of an assertion failure, but whatever. This effectively
collapses two copies into one. Or alternatively, build up an array of
iovecs and then have a variant of pq_putmessage(), like
pq_putmessage_iovec(), that knows what to do with them.

One advantage of this approach is that it approximately doubles the
size of the DataRow message we can send. We're currently limited to
<1GB because of palloc, but the wire protocol just needs it to be <2GB
so that a signed integer does not overflow. It would be nice to buy
more than a factor of two here, but that would require a wire protocol
change, and 2x is not bad.

Another advantage of this approach is that it doesn't require passing
StringInfos all over the place. For the use case that I was looking
at, that appears awkward. I'm not saying I couldn't make it work, but
it wouldn't be my first choice. Right now, I've got data bubbling down
a chain of handlers until it eventually gets sent off to the client;
with your approach, I think I'd need to bubble buffers up and then
bubble data down, which seems quite a bit more complex.

A disadvantage of this approach is that we still end up doing three
copies: one from the datum to the per-datum StringInfo, a second into
PqSendBuffer, and a third from there to the kernel. However, we could
probably improve on this. Whenever we internal_flush(), consider
whether the chunk of data we're the process of copying (the current
call to pq_putbytes(), or the current iovec) has enough bytes
remaining to completely refill the buffer. If so, secure_write() a
buffer's worth of bytes (or more) directly, bypassing PqSendBuffer.
That way, we avoid individual system calls (or calls to OpenSSL or
GSS) for small numbers of bytes, but we also avoid extra copying when
transmitting larger amounts of data.

Even with that optimization, this still seems like it could end up
being less efficient than your proposal (surprise, surprise). If we've
got a preallocated buffer which we won't be forced to resize during
message construction -- and for DataRow messages we can get there just
by keeping the buffer around, so that we only need to reallocate when
we see a larger message than we've ever seen before -- and we write
all the data directly into that buffer and then send it from there
straight to the kernel, we only ever do 2 copies, whereas what I'm
proposing sometimes does 3 copies and sometimes only 2.

While I admit that's not great, it seems likely to still be a
significant win over what we have now, and it's a *lot* less invasive
than your proposal. Not only does your approach require changing all
of the type-output and type-sending functions inside and outside core
to use this new model, admittedly with the possibility of backward
compatibility, but it also means that we could need similarly invasive
changes in any other place that wants to use this new style of message
construction. You can't write any data anywhere that you might want to
later incorporate into a protocol message unless you write it into a
StringInfo; and not only that, but you have to be able to get the
right amount of data into the right place in the StringInfo right from
the start. I think that in some cases that will require fairly complex
orchestration.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#18Robert Haas
robertmhaas@gmail.com
In reply to: Andres Freund (#11)
Re: Why is pq_begintypsend so slow?

Here is some review for the first few patches in this series.

I am generally in favor of applying 0001-0003 no matter what else we
decide to do here. However, as might be expected, I have a few
questions and comments.

Regarding 0001:

I dislike the name initStringInfoEx() because we do not use the "Ex"
convention for function names anywhere in the code base. We do
sometimes use "extended", so this could be initStringInfoExtended(),
perhaps, or something more specific, like initStringInfoWithLength().

Regarding the FIXME in that function, I suggest that this should be
the caller's job. Otherwise, there will probably be some caller which
doesn't want the add-one behavior, and then said caller will subtract
one to compensate, and that will be silly.

I am not familiar with pg_restrict and don't entirely understand the
motivation for it here. I suspect you have done some experiments and
figured out that it produces better code, but it would be interesting
to hear more about how you got there. Perhaps there could even be some
brief comments about it. Locutions like this are particularly
confusing to me:

+static inline void
+resetStringInfo(StringInfoData *pg_restrict str)
+{
+       *(char *pg_restrict) (str->data) = '\0';
+       str->len = 0;
+       str->cursor = 0;
+}

I don't understand how this can be saving anything. I think the
restrict definitions here mean that str->data does not overlap with
str itself, but considering that these are unconditional stores, so
what? If the code were doing something like memset(str->data, 0,
str->len) then I'd get it: it might be useful to know for optimization
purposes that the memset isn't overwriting str->len. But what code can
we produce for this that wouldn't be valid if str->data = &str? I
assume this is my lack of understanding rather than an actual problem
with the patch, but I would be grateful if you could explain.

It is easier to see why the pg_restrict stuff you've introduced into
appendBinaryStringInfoNT is potentially helpful: e.g. in
appendBinaryStringInfoNT, it promises that memcpy can't clobber
str->len, so the compiler is free to reorder without changing the
results. Or so I imagine. But then the one in appendBinaryStringInfo()
confuses me again: if str->data is already declared as a restricted
pointer, then why do we need to cast str->data + str->len to be
restricted also?

In appendStringInfoChar, why do we need to cast to restrict twice? Can
we not just do something like this:

char *pg_restrict ep = str->data+str->len;
ep[0] = ch;
ep[1] = '\0';

Regarding 0002:

Totally mechanical, seems fine.

Regarding 0003:

For the same reasons as above, I suggest renaming pq_begintypsend_ex()
to pq_begintypsend_extended() or pq_begintypsend_with_length() or
something of that sort, rather than using _ex.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#19Andres Freund
andres@anarazel.de
In reply to: Robert Haas (#18)
Re: Why is pq_begintypsend so slow?

Hi,

On 2020-07-31 11:14:46 -0400, Robert Haas wrote:

Here is some review for the first few patches in this series.

Thanks!

I am generally in favor of applying 0001-0003 no matter what else we
decide to do here. However, as might be expected, I have a few
questions and comments.

Regarding 0001:

I dislike the name initStringInfoEx() because we do not use the "Ex"
convention for function names anywhere in the code base. We do
sometimes use "extended", so this could be initStringInfoExtended(),
perhaps, or something more specific, like initStringInfoWithLength().

I dislike the length of the function name, but ...

Regarding the FIXME in that function, I suggest that this should be
the caller's job. Otherwise, there will probably be some caller which
doesn't want the add-one behavior, and then said caller will subtract
one to compensate, and that will be silly.

Fair point.

I am not familiar with pg_restrict and don't entirely understand the
motivation for it here. I suspect you have done some experiments and
figured out that it produces better code, but it would be interesting
to hear more about how you got there. Perhaps there could even be some
brief comments about it. Locutions like this are particularly
confusing to me:

+static inline void
+resetStringInfo(StringInfoData *pg_restrict str)
+{
+       *(char *pg_restrict) (str->data) = '\0';
+       str->len = 0;
+       str->cursor = 0;
+}

The restrict tells the compiler that 'str' and 'str->data' won't be
pointing to the same memory. Which can simpilify the code its
generating. E.g. it'll allow the compiler to keep str->data in a
register, instead of reloading it for the next
appendStringInfo*. Without the restrict it can't, because str->data = 0
could otherwise overwrite parts of the value of ->data itself, if ->data
pointed into the StringInfo. Similarly, str->data could be overwritten
by str->len in some other cases.

Partially the reason we need to add the markers is that we compile with
-fno-strict-aliasing. But even if we weren't, this case wouldn't be
solved without an explicit marker even then, because char * is allowed
to alias...

Besides keeping ->data in a register, the restrict can also just
entirely elide the null byte write in some cases, e.g. because the
resetStringInfo() is followed by a appendStringInfoString(, "constant").

I don't understand how this can be saving anything. I think the
restrict definitions here mean that str->data does not overlap with
str itself, but considering that these are unconditional stores, so
what? If the code were doing something like memset(str->data, 0,
str->len) then I'd get it: it might be useful to know for optimization
purposes that the memset isn't overwriting str->len. But what code can
we produce for this that wouldn't be valid if str->data = &str? I
assume this is my lack of understanding rather than an actual problem
with the patch, but I would be grateful if you could explain.

I hope the above makes this make sene now? It's about subsequent uses of
the StringInfo, rather than the body of resetStringInfo itself.

It is easier to see why the pg_restrict stuff you've introduced into
appendBinaryStringInfoNT is potentially helpful: e.g. in
appendBinaryStringInfoNT, it promises that memcpy can't clobber
str->len, so the compiler is free to reorder without changing the
results. Or so I imagine. But then the one in appendBinaryStringInfo()
confuses me again: if str->data is already declared as a restricted
pointer, then why do we need to cast str->data + str->len to be
restricted also?

But str->data isn't declared restricted without the explicit use of
restrict? str is restrict'ed, but it doesn't apply "recursively" to all
pointers contained therein.

In appendStringInfoChar, why do we need to cast to restrict twice? Can
we not just do something like this:

char *pg_restrict ep = str->data+str->len;
ep[0] = ch;
ep[1] = '\0';

I don't think that'd tell the compiler that this couldn't overlap with
str itself? A single 'restrict' can never (?) help, you need *two*
things that are marked as not overlapping in any way.

Greetings,

Andres Freund

#20Robert Haas
robertmhaas@gmail.com
In reply to: Andres Freund (#19)
Re: Why is pq_begintypsend so slow?

On Fri, Jul 31, 2020 at 11:50 AM Andres Freund <andres@anarazel.de> wrote:

I hope the above makes this make sene now? It's about subsequent uses of
the StringInfo, rather than the body of resetStringInfo itself.

That does make sense, except that
https://en.cppreference.com/w/c/language/restrict says "During each
execution of a block in which a restricted pointer P is declared
(typically each execution of a function body in which P is a function
parameter), if some object that is accessible through P (directly or
indirectly) is modified, by any means, then all accesses to that
object (both reads and writes) in that block must occur through P
(directly or indirectly), otherwise the behavior is undefined." So my
interpretation of this was that it couldn't really affect what
happened outside of the function itself, even if the compiler chose to
perform inlining. But I think see what you're saying: the *inference*
is only valid with respect to restrict pointers in a particular
function, but what can be optimized as a result of that inference may
be something further afield, if inlining is performed. Perhaps we
could add a comment about this, e.g.

Marking these pointers with pg_restrict tells the compiler that str
and str->data can't overlap, which may allow the compiler to optimize
better when this code is inlined. For example, it may be possible to
keep str->data in a register across consecutive appendStringInfoString
operations.

Since pg_restrict is not widely used, I think it's worth adding this
kind of annotation, lest other hackers get confused. I'm probably not
the only one who isn't on top of this.

In appendStringInfoChar, why do we need to cast to restrict twice? Can
we not just do something like this:

char *pg_restrict ep = str->data+str->len;
ep[0] = ch;
ep[1] = '\0';

I don't think that'd tell the compiler that this couldn't overlap with
str itself? A single 'restrict' can never (?) help, you need *two*
things that are marked as not overlapping in any way.

But what's the difference between:

+       *(char *pg_restrict) (str->data + str->len) = ch;
+       str->len++;
+       *(char *pg_restrict) (str->data + str->len) = '\0';

And:

char *pg_restrict ep = str->data+str->len;
ep[0] = ch;
ep[1] = '\0';
++str->len;

Whether or not str itself is marked restricted is another question;
what I'm talking about is why we need to repeat (char *pg_restrict)
(str->data + str->len).

I don't have any further comment on the remainder of your reply.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#21Andres Freund
andres@anarazel.de
In reply to: Robert Haas (#20)
Re: Why is pq_begintypsend so slow?

Hi,

On 2020-07-31 12:28:04 -0400, Robert Haas wrote:

On Fri, Jul 31, 2020 at 11:50 AM Andres Freund <andres@anarazel.de> wrote:

I hope the above makes this make sene now? It's about subsequent uses of
the StringInfo, rather than the body of resetStringInfo itself.

That does make sense, except that
https://en.cppreference.com/w/c/language/restrict says "During each
execution of a block in which a restricted pointer P is declared
(typically each execution of a function body in which P is a function
parameter), if some object that is accessible through P (directly or
indirectly) is modified, by any means, then all accesses to that
object (both reads and writes) in that block must occur through P
(directly or indirectly), otherwise the behavior is undefined." So my
interpretation of this was that it couldn't really affect what
happened outside of the function itself, even if the compiler chose to
perform inlining. But I think see what you're saying: the *inference*
is only valid with respect to restrict pointers in a particular
function, but what can be optimized as a result of that inference may
be something further afield, if inlining is performed.

Right. There's two aspects:

1) By looking at the function, with the restrict, the compiler can infer
more about the behaviour of the function. E.g. knowing that -> len
has a specific value, or that ->data[n] does. That information then
can be used together with subsequent operations, e.g. avoiding a
re-read of ->len. That could work in some cases even if subsequent
operations were *not* marked up with restrict.

2) The restrict signals to the compiler that we guarantee (i.e. it would
be undefined behaviour if not) that the pointers do not
overlap. Which means it can assume that in some of the calling code
as well, if it can analyze that ->data isn't changed, for example.

Perhaps we could add a comment about this, e.g.
Marking these pointers with pg_restrict tells the compiler that str
and str->data can't overlap, which may allow the compiler to optimize
better when this code is inlined. For example, it may be possible to
keep str->data in a register across consecutive appendStringInfoString
operations.

Since pg_restrict is not widely used, I think it's worth adding this
kind of annotation, lest other hackers get confused. I'm probably not
the only one who isn't on top of this.

Would it make more sense to have a bit of an explanation at
pg_restrict's definition, instead of having it at (eventually) multiple
places?

In appendStringInfoChar, why do we need to cast to restrict twice? Can
we not just do something like this:

char *pg_restrict ep = str->data+str->len;
ep[0] = ch;
ep[1] = '\0';

I don't think that'd tell the compiler that this couldn't overlap with
str itself? A single 'restrict' can never (?) help, you need *two*
things that are marked as not overlapping in any way.

But what's the difference between:

+       *(char *pg_restrict) (str->data + str->len) = ch;
+       str->len++;
+       *(char *pg_restrict) (str->data + str->len) = '\0';

And:

char *pg_restrict ep = str->data+str->len;
ep[0] = ch;
ep[1] = '\0';
++str->len;

Whether or not str itself is marked restricted is another question;
what I'm talking about is why we need to repeat (char *pg_restrict)
(str->data + str->len).

Ah, I misunderstood. Yea, there's no reason not to do that.

Greetings,

Andres Freund

#22Robert Haas
robertmhaas@gmail.com
In reply to: Andres Freund (#21)
Re: Why is pq_begintypsend so slow?

On Fri, Jul 31, 2020 at 1:00 PM Andres Freund <andres@anarazel.de> wrote:

Perhaps we could add a comment about this, e.g.
Marking these pointers with pg_restrict tells the compiler that str
and str->data can't overlap, which may allow the compiler to optimize
better when this code is inlined. For example, it may be possible to
keep str->data in a register across consecutive appendStringInfoString
operations.

Since pg_restrict is not widely used, I think it's worth adding this
kind of annotation, lest other hackers get confused. I'm probably not
the only one who isn't on top of this.

Would it make more sense to have a bit of an explanation at
pg_restrict's definition, instead of having it at (eventually) multiple
places?

I think, at least for the first few, it might be better to have a more
specific explanation at the point of use, as it may be easier to
understand in specific cases than in general. I imagine this only
really makes sense for places that are pretty hot.

Ah, I misunderstood. Yea, there's no reason not to do that.

OK, then I vote for that version, as I think it looks nicer.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#23Andres Freund
andres@anarazel.de
In reply to: Robert Haas (#22)
10 attachment(s)
Re: Why is pq_begintypsend so slow?

Hi,

I was reminded of the patchset I had posted in this thread by
/messages/by-id/679d5455cbbb0af667ccb753da51a475bae1eaed.camel@cybertec.at

On 2020-07-31 13:35:43 -0400, Robert Haas wrote:

On Fri, Jul 31, 2020 at 1:00 PM Andres Freund <andres@anarazel.de> wrote:

Perhaps we could add a comment about this, e.g.
Marking these pointers with pg_restrict tells the compiler that str
and str->data can't overlap, which may allow the compiler to optimize
better when this code is inlined. For example, it may be possible to
keep str->data in a register across consecutive appendStringInfoString
operations.

Since pg_restrict is not widely used, I think it's worth adding this
kind of annotation, lest other hackers get confused. I'm probably not
the only one who isn't on top of this.

Would it make more sense to have a bit of an explanation at
pg_restrict's definition, instead of having it at (eventually) multiple
places?

I think, at least for the first few, it might be better to have a more
specific explanation at the point of use, as it may be easier to
understand in specific cases than in general. I imagine this only
really makes sense for places that are pretty hot.

Whenever I looked at adding these comments, it felt wrong. We end up with
repetitive boilerplate stuff as quite a few functions use it. I've thus not
addressed this aspect in the attached rebased version. Perhaps a compromise
would be to add such a comment to the top of stringinfo.h?

Ah, I misunderstood. Yea, there's no reason not to do that.

OK, then I vote for that version, as I think it looks nicer.

Done.

Greetings,

Andres Freund

Attachments:

v3-0001-stringinfo-Move-more-functions-inline-provide-ini.patchtext/x-diff; charset=us-asciiDownload
From 3f43bf064afaa3a754fddfb9312256fb45820857 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 6 Feb 2024 17:32:00 -0800
Subject: [PATCH v3 01/10] stringinfo: Move more functions inline, provide
 initStringInfoWithSize()

By moving the whole lifecycle of a stringinfo into inline functions,
a good compiler sometimes can be able to optimize away the existence
of the StringInfoData itself.

initStringInfoWithSize() allows stringinfo users to determine the initial
allocation size, avoiding over/under allocation when known.

Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20200603015559.qc7ry5jqmoxlpu4y@alap3.anarazel.de
---
 src/include/lib/stringinfo.h | 224 ++++++++++++++++++++++++++++-------
 src/common/stringinfo.c      | 179 ++--------------------------
 2 files changed, 191 insertions(+), 212 deletions(-)

diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 2cd636b01cf..409a2365cc3 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -18,6 +18,15 @@
 #ifndef STRINGINFO_H
 #define STRINGINFO_H
 
+#include "common/int.h"
+#include "common/string.h"
+
+#ifdef FRONTEND
+#include "common/fe_memutils.h"
+#else
+#include "utils/palloc.h"
+#endif
+
 /*-------------------------
  * StringInfoData holds information about an extensible string.
  *		data	is the current buffer for the string.
@@ -99,18 +108,6 @@ typedef StringInfoData *StringInfo;
  *-------------------------
  */
 
-/*------------------------
- * makeStringInfo
- * Create an empty 'StringInfoData' & return a pointer to it.
- */
-extern StringInfo makeStringInfo(void);
-
-/*------------------------
- * initStringInfo
- * Initialize a StringInfoData struct (with previously undefined contents)
- * to describe an empty string.
- */
-extern void initStringInfo(StringInfo str);
 
 /*------------------------
  * initReadOnlyStringInfo
@@ -158,8 +155,141 @@ initStringInfoFromString(StringInfo str, char *data, int len)
  * resetStringInfo
  * Clears the current content of the StringInfo, if any. The
  * StringInfo remains valid.
+ *
+ * Read-only StringInfos as initialized by initReadOnlyStringInfo cannot be
+ * reset.
  */
-extern void resetStringInfo(StringInfo str);
+static inline void
+resetStringInfo(StringInfoData *pg_restrict str)
+{
+	/* don't allow resets of read-only StringInfos */
+	Assert(str->maxlen != 0);
+
+	*(char *pg_restrict) (str->data) = '\0';
+	str->len = 0;
+	str->cursor = 0;
+}
+
+/*------------------------
+ * initStringInfo
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ */
+static inline void
+initStringInfo(StringInfoData *pg_restrict str)
+{
+	int			size = 1024;	/* initial default buffer size */
+
+	str->data = (char *) palloc(size);
+	str->maxlen = size;
+	resetStringInfo(str);
+}
+
+/*------------------------
+ * initStringInfoWithSize
+ *
+ * Like initStringInfo(), but allows to specify the size of the initial
+ * allocation.
+ */
+static inline void
+initStringInfoWithSize(StringInfoData *pg_restrict str, int size)
+{
+	/*
+	 * Note that maxlen is increased by 1 to account for the trailing \0 byte.
+	 * Otherwise creating a stringinfo of size N and appending N bytes of data
+	 * to it, would lead to a reallocation, to maintain the invariant that
+	 * there always is space for the trailing \0 byte.
+	 */
+	str->data = (char *) palloc(size + 1);
+	str->maxlen = size + 1;
+	resetStringInfo(str);
+}
+
+/*------------------------
+ * makeStringInfo
+ *
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ */
+static inline StringInfo
+makeStringInfo(void)
+{
+	StringInfo	res;
+
+	res = (StringInfo) palloc(sizeof(StringInfoData));
+
+	initStringInfo(res);
+
+	return res;
+}
+
+/*------------------------
+ * enlargeStringInfoImpl
+ *
+ * Actually enlarge the string, only to be called by enlargeStringInfo().
+ */
+extern void enlargeStringInfoImpl(StringInfo str, int needed);
+
+/*------------------------
+ * enlargeStringInfo
+ * Make sure a StringInfo's buffer can hold at least 'needed' more bytes.
+ *
+ * External callers usually need not concern themselves with this, since
+ * all stringinfo.c routines do it automatically.  However, if a caller
+ * knows that a StringInfo will eventually become X bytes large, it
+ * can save some palloc overhead by enlarging the buffer before starting
+ * to store data in it.
+ *
+ * NB: In the backend, because we use repalloc() to enlarge the buffer, the
+ * string buffer will remain allocated in the same memory context that was
+ * current when initStringInfo was called, even if another context is now
+ * current.  This is the desired and indeed critical behavior!
+ */
+static inline void
+enlargeStringInfo(StringInfoData *pg_restrict str, int datalen)
+{
+	int			res;
+
+	if (unlikely(pg_add_s32_overflow(str->len, datalen, &res)) ||
+		unlikely(res >= str->maxlen))
+		enlargeStringInfoImpl(str, datalen);
+}
+
+/*------------------------
+ * appendBinaryStringInfoNT
+ * Append arbitrary binary data to a StringInfo, allocating more space
+ * if necessary. Does not ensure a trailing null-byte exists.
+ */
+static inline void
+appendBinaryStringInfoNT(StringInfoData *pg_restrict str, const void *pg_restrict data, int datalen)
+{
+	Assert(str != NULL);
+
+	/* Make more room if needed */
+	enlargeStringInfo(str, datalen);
+
+	/* OK, append the data */
+	memcpy((char *pg_restrict) (str->data + str->len), data, datalen);
+	str->len += datalen;
+}
+
+/*------------------------
+ * appendBinaryStringInfo
+ * Append arbitrary binary data to a StringInfo, allocating more space
+ * if necessary. Ensures that a trailing null byte is present.
+ */
+static inline void
+appendBinaryStringInfo(StringInfoData *pg_restrict str, const void *pg_restrict data, int datalen)
+{
+	appendBinaryStringInfoNT(str, data, datalen);
+
+	/*
+	 * Keep a trailing null in place, even though it's probably useless for
+	 * binary data.  (Some callers are dealing with text but call this because
+	 * their input isn't null-terminated.)
+	 */
+	*(char *pg_restrict) (str->data + str->len) = '\0';
+}
+
 
 /*------------------------
  * appendStringInfo
@@ -186,51 +316,55 @@ extern int	appendStringInfoVA(StringInfo str, const char *fmt, va_list args) pg_
  * Append a null-terminated string to str.
  * Like appendStringInfo(str, "%s", s) but faster.
  */
-extern void appendStringInfoString(StringInfo str, const char *s);
+static inline void
+appendStringInfoString(StringInfoData *pg_restrict str, const char *pg_restrict s)
+{
+	appendBinaryStringInfo(str, s, strlen(s));
+}
 
 /*------------------------
  * appendStringInfoChar
  * Append a single byte to str.
  * Like appendStringInfo(str, "%c", ch) but much faster.
  */
-extern void appendStringInfoChar(StringInfo str, char ch);
+static inline void
+appendStringInfoChar(StringInfoData *pg_restrict str, char ch)
+{
+	char	   *pg_restrict ep;
 
-/*------------------------
- * appendStringInfoCharMacro
- * As above, but a macro for even more speed where it matters.
- * Caution: str argument will be evaluated multiple times.
- */
-#define appendStringInfoCharMacro(str,ch) \
-	(((str)->len + 1 >= (str)->maxlen) ? \
-	 appendStringInfoChar(str, ch) : \
-	 (void)((str)->data[(str)->len] = (ch), (str)->data[++(str)->len] = '\0'))
+	/* Make more room if needed */
+	enlargeStringInfo(str, 1);
+
+	/* OK, append the character */
+	ep = str->data + str->len;
+	ep[0] = ch;
+	ep[1] = '\0';
+	str->len++;
+}
+
+/* backward compat for external code */
+#define appendStringInfoCharMacro appendStringInfoChar
 
 /*------------------------
  * appendStringInfoSpaces
  * Append a given number of spaces to str.
  */
-extern void appendStringInfoSpaces(StringInfo str, int count);
+static inline void
+appendStringInfoSpaces(StringInfoData *pg_restrict str, int count)
+{
+	if (count > 0)
+	{
+		char	   *pg_restrict ep;
 
-/*------------------------
- * appendBinaryStringInfo
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary.
- */
-extern void appendBinaryStringInfo(StringInfo str,
-								   const void *data, int datalen);
+		/* Make more room if needed */
+		enlargeStringInfo(str, count);
 
-/*------------------------
- * appendBinaryStringInfoNT
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Does not ensure a trailing null-byte exists.
- */
-extern void appendBinaryStringInfoNT(StringInfo str,
-									 const void *data, int datalen);
-
-/*------------------------
- * enlargeStringInfo
- * Make sure a StringInfo's buffer can hold at least 'needed' more bytes.
- */
-extern void enlargeStringInfo(StringInfo str, int needed);
+		/* OK, append the spaces */
+		ep = str->data + str->len;
+		memset(ep, ' ', count);
+		str->len += count;
+		ep[count] = '\0';
+	}
+}
 
 #endif							/* STRINGINFO_H */
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index c61d5c58f30..92d168e71e5 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -32,59 +32,6 @@
 #include "lib/stringinfo.h"
 
 
-/*
- * makeStringInfo
- *
- * Create an empty 'StringInfoData' & return a pointer to it.
- */
-StringInfo
-makeStringInfo(void)
-{
-	StringInfo	res;
-
-	res = (StringInfo) palloc(sizeof(StringInfoData));
-
-	initStringInfo(res);
-
-	return res;
-}
-
-/*
- * initStringInfo
- *
- * Initialize a StringInfoData struct (with previously undefined contents)
- * to describe an empty string.
- */
-void
-initStringInfo(StringInfo str)
-{
-	int			size = 1024;	/* initial default buffer size */
-
-	str->data = (char *) palloc(size);
-	str->maxlen = size;
-	resetStringInfo(str);
-}
-
-/*
- * resetStringInfo
- *
- * Reset the StringInfo: the data buffer remains valid, but its
- * previous content, if any, is cleared.
- *
- * Read-only StringInfos as initialized by initReadOnlyStringInfo cannot be
- * reset.
- */
-void
-resetStringInfo(StringInfo str)
-{
-	/* don't allow resets of read-only StringInfos */
-	Assert(str->maxlen != 0);
-
-	str->data[0] = '\0';
-	str->len = 0;
-	str->cursor = 0;
-}
-
 /*
  * appendStringInfo
  *
@@ -173,120 +120,18 @@ appendStringInfoVA(StringInfo str, const char *fmt, va_list args)
 }
 
 /*
- * appendStringInfoString
+ * enlargeStringInfoImpl
  *
- * Append a null-terminated string to str.
- * Like appendStringInfo(str, "%s", s) but faster.
+ * Make enough space for 'needed' more bytes ('needed' does not include the
+ * terminating null). This is not for external consumption, it's only to be
+ * called by enlargeStringInfo() when more space is actually needed (including
+ * when we'd overflow the maximum size).
+ *
+ * As this normally shouldn't be the common case, mark as noinline, to avoid
+ * including the function into the fastpath.
  */
-void
-appendStringInfoString(StringInfo str, const char *s)
-{
-	appendBinaryStringInfo(str, s, strlen(s));
-}
-
-/*
- * appendStringInfoChar
- *
- * Append a single byte to str.
- * Like appendStringInfo(str, "%c", ch) but much faster.
- */
-void
-appendStringInfoChar(StringInfo str, char ch)
-{
-	/* Make more room if needed */
-	if (str->len + 1 >= str->maxlen)
-		enlargeStringInfo(str, 1);
-
-	/* OK, append the character */
-	str->data[str->len] = ch;
-	str->len++;
-	str->data[str->len] = '\0';
-}
-
-/*
- * appendStringInfoSpaces
- *
- * Append the specified number of spaces to a buffer.
- */
-void
-appendStringInfoSpaces(StringInfo str, int count)
-{
-	if (count > 0)
-	{
-		/* Make more room if needed */
-		enlargeStringInfo(str, count);
-
-		/* OK, append the spaces */
-		memset(&str->data[str->len], ' ', count);
-		str->len += count;
-		str->data[str->len] = '\0';
-	}
-}
-
-/*
- * appendBinaryStringInfo
- *
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Ensures that a trailing null byte is present.
- */
-void
-appendBinaryStringInfo(StringInfo str, const void *data, int datalen)
-{
-	Assert(str != NULL);
-
-	/* Make more room if needed */
-	enlargeStringInfo(str, datalen);
-
-	/* OK, append the data */
-	memcpy(str->data + str->len, data, datalen);
-	str->len += datalen;
-
-	/*
-	 * Keep a trailing null in place, even though it's probably useless for
-	 * binary data.  (Some callers are dealing with text but call this because
-	 * their input isn't null-terminated.)
-	 */
-	str->data[str->len] = '\0';
-}
-
-/*
- * appendBinaryStringInfoNT
- *
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Does not ensure a trailing null-byte exists.
- */
-void
-appendBinaryStringInfoNT(StringInfo str, const void *data, int datalen)
-{
-	Assert(str != NULL);
-
-	/* Make more room if needed */
-	enlargeStringInfo(str, datalen);
-
-	/* OK, append the data */
-	memcpy(str->data + str->len, data, datalen);
-	str->len += datalen;
-}
-
-/*
- * enlargeStringInfo
- *
- * Make sure there is enough space for 'needed' more bytes
- * ('needed' does not include the terminating null).
- *
- * External callers usually need not concern themselves with this, since
- * all stringinfo.c routines do it automatically.  However, if a caller
- * knows that a StringInfo will eventually become X bytes large, it
- * can save some palloc overhead by enlarging the buffer before starting
- * to store data in it.
- *
- * NB: In the backend, because we use repalloc() to enlarge the buffer, the
- * string buffer will remain allocated in the same memory context that was
- * current when initStringInfo was called, even if another context is now
- * current.  This is the desired and indeed critical behavior!
- */
-void
-enlargeStringInfo(StringInfo str, int needed)
+pg_noinline void
+enlargeStringInfoImpl(StringInfo str, int needed)
 {
 	int			newlen;
 
@@ -326,8 +171,8 @@ enlargeStringInfo(StringInfo str, int needed)
 
 	/* Because of the above test, we now have needed <= MaxAllocSize */
 
-	if (needed <= str->maxlen)
-		return;					/* got enough space already */
+	/* should only be called when needed */
+	Assert(needed > str->maxlen);
 
 	/*
 	 * We don't want to allocate just a little more space with each append;
-- 
2.38.0

v3-0002-stringinfo-Remove-in-core-use-of-appendStringInfo.patchtext/x-diff; charset=us-asciiDownload
From 8e3889d0667b5ce7e8e60066e7e29952d9ba5301 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 6 Feb 2024 17:34:42 -0800
Subject: [PATCH v3 02/10] stringinfo: Remove in-core use of
 appendStringInfoCharMacro()

Kept seperate only to reduce size of patch moving to more inlining for
stringinfo.

Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20200603015559.qc7ry5jqmoxlpu4y@alap3.anarazel.de
---
 src/backend/commands/copyto.c             |  2 +-
 src/backend/commands/explain.c            |  8 +++----
 src/backend/libpq/pqformat.c              | 10 ++++----
 src/backend/utils/adt/json.c              |  6 ++---
 src/backend/utils/adt/jsonb.c             | 10 ++++----
 src/backend/utils/adt/jsonfuncs.c         | 28 +++++++++++------------
 src/backend/utils/adt/jsonpath.c          |  6 ++---
 src/backend/utils/adt/rowtypes.c          |  8 +++----
 src/backend/utils/adt/varlena.c           |  4 ++--
 src/backend/utils/adt/xml.c               |  2 +-
 src/backend/utils/error/csvlog.c          |  8 +++----
 src/backend/utils/error/elog.c            |  4 ++--
 src/backend/utils/mb/stringinfo_mb.c      |  2 +-
 src/bin/pg_combinebackup/write_manifest.c |  6 ++---
 src/bin/pg_rewind/libpq_source.c          |  8 +++----
 contrib/tcn/tcn.c                         | 16 ++++++-------
 16 files changed, 64 insertions(+), 64 deletions(-)

diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 20ffc90363d..b7f8b04ed8d 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -187,7 +187,7 @@ CopySendString(CopyToState cstate, const char *str)
 static void
 CopySendChar(CopyToState cstate, char c)
 {
-	appendStringInfoCharMacro(cstate->fe_msgbuf, c);
+	appendStringInfoChar(cstate->fe_msgbuf, c);
 }
 
 static void
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 83d00a46638..ddae81e5d7a 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -5098,16 +5098,16 @@ ExplainXMLTag(const char *tagname, int flags, ExplainState *es)
 
 	if ((flags & X_NOWHITESPACE) == 0)
 		appendStringInfoSpaces(es->str, 2 * es->indent);
-	appendStringInfoCharMacro(es->str, '<');
+	appendStringInfoChar(es->str, '<');
 	if ((flags & X_CLOSING) != 0)
-		appendStringInfoCharMacro(es->str, '/');
+		appendStringInfoChar(es->str, '/');
 	for (s = tagname; *s; s++)
 		appendStringInfoChar(es->str, strchr(valid, *s) ? *s : '-');
 	if ((flags & X_CLOSE_IMMEDIATE) != 0)
 		appendStringInfoString(es->str, " /");
-	appendStringInfoCharMacro(es->str, '>');
+	appendStringInfoChar(es->str, '>');
 	if ((flags & X_NOWHITESPACE) == 0)
-		appendStringInfoCharMacro(es->str, '\n');
+		appendStringInfoChar(es->str, '\n');
 }
 
 /*
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index a697ccfbbf9..4708a7988a6 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -235,7 +235,7 @@ pq_send_ascii_string(StringInfo buf, const char *str)
 
 		if (IS_HIGHBIT_SET(ch))
 			ch = '?';
-		appendStringInfoCharMacro(buf, ch);
+		appendStringInfoChar(buf, ch);
 	}
 	appendStringInfoChar(buf, '\0');
 }
@@ -330,10 +330,10 @@ pq_begintypsend(StringInfo buf)
 {
 	initStringInfo(buf);
 	/* Reserve four bytes for the bytea length word */
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
+	appendStringInfoChar(buf, '\0');
+	appendStringInfoChar(buf, '\0');
+	appendStringInfoChar(buf, '\0');
+	appendStringInfoChar(buf, '\0');
 }
 
 /* --------------------------------
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index d719a61f16b..a8826fd45c6 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -1550,7 +1550,7 @@ escape_json(StringInfo buf, const char *str)
 {
 	const char *p;
 
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 	for (p = str; *p; p++)
 	{
 		switch (*p)
@@ -1580,11 +1580,11 @@ escape_json(StringInfo buf, const char *str)
 				if ((unsigned char) *p < ' ')
 					appendStringInfo(buf, "\\u%04x", (int) *p);
 				else
-					appendStringInfoCharMacro(buf, *p);
+					appendStringInfoChar(buf, *p);
 				break;
 		}
 	}
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 }
 
 /* Semantic actions for key uniqueness check */
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index c10b3fbedf1..4dc820b92b2 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -533,7 +533,7 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 				if (!v.val.array.rawScalar)
 				{
 					add_indent(out, use_indent && !last_was_key, level);
-					appendStringInfoCharMacro(out, '[');
+					appendStringInfoChar(out, '[');
 				}
 				else
 					raw_scalar = true;
@@ -546,7 +546,7 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 					appendBinaryStringInfo(out, ", ", ispaces);
 
 				add_indent(out, use_indent && !last_was_key, level);
-				appendStringInfoCharMacro(out, '{');
+				appendStringInfoChar(out, '{');
 
 				first = true;
 				level++;
@@ -594,14 +594,14 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 				if (!raw_scalar)
 				{
 					add_indent(out, use_indent, level);
-					appendStringInfoCharMacro(out, ']');
+					appendStringInfoChar(out, ']');
 				}
 				first = false;
 				break;
 			case WJB_END_OBJECT:
 				level--;
 				add_indent(out, use_indent, level);
-				appendStringInfoCharMacro(out, '}');
+				appendStringInfoChar(out, '}');
 				first = false;
 				break;
 			default:
@@ -621,7 +621,7 @@ add_indent(StringInfo out, bool indent, int level)
 {
 	if (indent)
 	{
-		appendStringInfoCharMacro(out, '\n');
+		appendStringInfoChar(out, '\n');
 		appendStringInfoSpaces(out, level * 4);
 	}
 }
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 1b0f4943292..4a81bc15cf2 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -4311,7 +4311,7 @@ sn_object_start(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '{');
+	appendStringInfoChar(_state->strval, '{');
 
 	return JSON_SUCCESS;
 }
@@ -4321,7 +4321,7 @@ sn_object_end(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '}');
+	appendStringInfoChar(_state->strval, '}');
 
 	return JSON_SUCCESS;
 }
@@ -4331,7 +4331,7 @@ sn_array_start(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '[');
+	appendStringInfoChar(_state->strval, '[');
 
 	return JSON_SUCCESS;
 }
@@ -4341,7 +4341,7 @@ sn_array_end(void *state)
 {
 	StripnullState *_state = (StripnullState *) state;
 
-	appendStringInfoCharMacro(_state->strval, ']');
+	appendStringInfoChar(_state->strval, ']');
 
 	return JSON_SUCCESS;
 }
@@ -4363,7 +4363,7 @@ sn_object_field_start(void *state, char *fname, bool isnull)
 	}
 
 	if (_state->strval->data[_state->strval->len - 1] != '{')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 
 	/*
 	 * Unfortunately we don't have the quoted and escaped string any more, so
@@ -4371,7 +4371,7 @@ sn_object_field_start(void *state, char *fname, bool isnull)
 	 */
 	escape_json(_state->strval, fname);
 
-	appendStringInfoCharMacro(_state->strval, ':');
+	appendStringInfoChar(_state->strval, ':');
 
 	return JSON_SUCCESS;
 }
@@ -4382,7 +4382,7 @@ sn_array_element_start(void *state, bool isnull)
 	StripnullState *_state = (StripnullState *) state;
 
 	if (_state->strval->data[_state->strval->len - 1] != '[')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 
 	return JSON_SUCCESS;
 }
@@ -5785,7 +5785,7 @@ transform_string_values_object_start(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '{');
+	appendStringInfoChar(_state->strval, '{');
 
 	return JSON_SUCCESS;
 }
@@ -5795,7 +5795,7 @@ transform_string_values_object_end(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '}');
+	appendStringInfoChar(_state->strval, '}');
 
 	return JSON_SUCCESS;
 }
@@ -5805,7 +5805,7 @@ transform_string_values_array_start(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, '[');
+	appendStringInfoChar(_state->strval, '[');
 
 	return JSON_SUCCESS;
 }
@@ -5815,7 +5815,7 @@ transform_string_values_array_end(void *state)
 {
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
-	appendStringInfoCharMacro(_state->strval, ']');
+	appendStringInfoChar(_state->strval, ']');
 
 	return JSON_SUCCESS;
 }
@@ -5826,14 +5826,14 @@ transform_string_values_object_field_start(void *state, char *fname, bool isnull
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
 	if (_state->strval->data[_state->strval->len - 1] != '{')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 
 	/*
 	 * Unfortunately we don't have the quoted and escaped string any more, so
 	 * we have to re-escape it.
 	 */
 	escape_json(_state->strval, fname);
-	appendStringInfoCharMacro(_state->strval, ':');
+	appendStringInfoChar(_state->strval, ':');
 
 	return JSON_SUCCESS;
 }
@@ -5844,7 +5844,7 @@ transform_string_values_array_element_start(void *state, bool isnull)
 	TransformJsonStringValuesState *_state = (TransformJsonStringValuesState *) state;
 
 	if (_state->strval->data[_state->strval->len - 1] != '[')
-		appendStringInfoCharMacro(_state->strval, ',');
+		appendStringInfoChar(_state->strval, ',');
 
 	return JSON_SUCCESS;
 }
diff --git a/src/backend/utils/adt/jsonpath.c b/src/backend/utils/adt/jsonpath.c
index 258ed8eb117..a760b3ba8de 100644
--- a/src/backend/utils/adt/jsonpath.c
+++ b/src/backend/utils/adt/jsonpath.c
@@ -484,13 +484,13 @@ alignStringInfoInt(StringInfo buf)
 	switch (INTALIGN(buf->len) - buf->len)
 	{
 		case 3:
-			appendStringInfoCharMacro(buf, 0);
+			appendStringInfoChar(buf, 0);
 			/* FALLTHROUGH */
 		case 2:
-			appendStringInfoCharMacro(buf, 0);
+			appendStringInfoChar(buf, 0);
 			/* FALLTHROUGH */
 		case 1:
-			appendStringInfoCharMacro(buf, 0);
+			appendStringInfoChar(buf, 0);
 			/* FALLTHROUGH */
 		default:
 			break;
diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index adc02702fca..d489dab9f54 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -452,17 +452,17 @@ record_out(PG_FUNCTION_ARGS)
 
 		/* And emit the string */
 		if (nq)
-			appendStringInfoCharMacro(&buf, '"');
+			appendStringInfoChar(&buf, '"');
 		for (tmp = value; *tmp; tmp++)
 		{
 			char		ch = *tmp;
 
 			if (ch == '"' || ch == '\\')
-				appendStringInfoCharMacro(&buf, ch);
-			appendStringInfoCharMacro(&buf, ch);
+				appendStringInfoChar(&buf, ch);
+			appendStringInfoChar(&buf, ch);
 		}
 		if (nq)
-			appendStringInfoCharMacro(&buf, '"');
+			appendStringInfoChar(&buf, '"');
 	}
 
 	appendStringInfoChar(&buf, ')');
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e58..01d5d0dbb63 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5689,7 +5689,7 @@ text_format(PG_FUNCTION_ARGS)
 		 */
 		if (*cp != '%')
 		{
-			appendStringInfoCharMacro(&str, *cp);
+			appendStringInfoChar(&str, *cp);
 			continue;
 		}
 
@@ -5698,7 +5698,7 @@ text_format(PG_FUNCTION_ARGS)
 		/* Easy case: %% outputs a single % */
 		if (*cp == '%')
 		{
-			appendStringInfoCharMacro(&str, *cp);
+			appendStringInfoChar(&str, *cp);
 			continue;
 		}
 
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 3e24aba546f..24967a1caf2 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -2655,7 +2655,7 @@ escape_xml(const char *str)
 				appendStringInfoString(&buf, "&#x0d;");
 				break;
 			default:
-				appendStringInfoCharMacro(&buf, *p);
+				appendStringInfoChar(&buf, *p);
 				break;
 		}
 	}
diff --git a/src/backend/utils/error/csvlog.c b/src/backend/utils/error/csvlog.c
index 1b62b07f231..9d6f1f661a7 100644
--- a/src/backend/utils/error/csvlog.c
+++ b/src/backend/utils/error/csvlog.c
@@ -45,14 +45,14 @@ appendCSVLiteral(StringInfo buf, const char *data)
 	if (p == NULL)
 		return;
 
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 	while ((c = *p++) != '\0')
 	{
 		if (c == '"')
-			appendStringInfoCharMacro(buf, '"');
-		appendStringInfoCharMacro(buf, c);
+			appendStringInfoChar(buf, '"');
+		appendStringInfoChar(buf, c);
 	}
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 }
 
 /*
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index bba00a0087f..4dce794a989 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3689,9 +3689,9 @@ append_with_tabs(StringInfo buf, const char *str)
 
 	while ((ch = *str++) != '\0')
 	{
-		appendStringInfoCharMacro(buf, ch);
+		appendStringInfoChar(buf, ch);
 		if (ch == '\n')
-			appendStringInfoCharMacro(buf, '\t');
+			appendStringInfoChar(buf, '\t');
 	}
 }
 
diff --git a/src/backend/utils/mb/stringinfo_mb.c b/src/backend/utils/mb/stringinfo_mb.c
index f4af3909e88..b90aa9830a1 100644
--- a/src/backend/utils/mb/stringinfo_mb.c
+++ b/src/backend/utils/mb/stringinfo_mb.c
@@ -61,7 +61,7 @@ appendStringInfoStringQuoted(StringInfo str, const char *s, int maxlen)
 		ellipsis = false;
 	}
 
-	appendStringInfoCharMacro(str, '\'');
+	appendStringInfoChar(str, '\'');
 
 	while ((chunk_end = strchr(chunk_search_start, '\'')) != NULL)
 	{
diff --git a/src/bin/pg_combinebackup/write_manifest.c b/src/bin/pg_combinebackup/write_manifest.c
index 01deb82cc9f..fae1043d2d0 100644
--- a/src/bin/pg_combinebackup/write_manifest.c
+++ b/src/bin/pg_combinebackup/write_manifest.c
@@ -194,7 +194,7 @@ escape_json(StringInfo buf, const char *str)
 {
 	const char *p;
 
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 	for (p = str; *p; p++)
 	{
 		switch (*p)
@@ -224,11 +224,11 @@ escape_json(StringInfo buf, const char *str)
 				if ((unsigned char) *p < ' ')
 					appendStringInfo(buf, "\\u%04x", (int) *p);
 				else
-					appendStringInfoCharMacro(buf, *p);
+					appendStringInfoChar(buf, *p);
 				break;
 		}
 	}
-	appendStringInfoCharMacro(buf, '"');
+	appendStringInfoChar(buf, '"');
 }
 
 /*
diff --git a/src/bin/pg_rewind/libpq_source.c b/src/bin/pg_rewind/libpq_source.c
index 7d898c3b501..476cc4249cd 100644
--- a/src/bin/pg_rewind/libpq_source.c
+++ b/src/bin/pg_rewind/libpq_source.c
@@ -613,19 +613,19 @@ process_queued_fetch_requests(libpq_source *src)
 static void
 appendArrayEscapedString(StringInfo buf, const char *str)
 {
-	appendStringInfoCharMacro(buf, '\"');
+	appendStringInfoChar(buf, '\"');
 	while (*str)
 	{
 		char		ch = *str;
 
 		if (ch == '"' || ch == '\\')
-			appendStringInfoCharMacro(buf, '\\');
+			appendStringInfoChar(buf, '\\');
 
-		appendStringInfoCharMacro(buf, ch);
+		appendStringInfoChar(buf, ch);
 
 		str++;
 	}
-	appendStringInfoCharMacro(buf, '\"');
+	appendStringInfoChar(buf, '\"');
 }
 
 /*
diff --git a/contrib/tcn/tcn.c b/contrib/tcn/tcn.c
index 8d23c824c1b..754deae54e7 100644
--- a/contrib/tcn/tcn.c
+++ b/contrib/tcn/tcn.c
@@ -32,15 +32,15 @@ PG_MODULE_MAGIC;
 static void
 strcpy_quoted(StringInfo r, const char *s, const char q)
 {
-	appendStringInfoCharMacro(r, q);
+	appendStringInfoChar(r, q);
 	while (*s)
 	{
 		if (*s == q)
-			appendStringInfoCharMacro(r, q);
-		appendStringInfoCharMacro(r, *s);
+			appendStringInfoChar(r, q);
+		appendStringInfoChar(r, *s);
 		s++;
 	}
-	appendStringInfoCharMacro(r, q);
+	appendStringInfoChar(r, q);
 }
 
 /*
@@ -147,17 +147,17 @@ triggered_change_notification(PG_FUNCTION_ARGS)
 				foundPK = true;
 
 				strcpy_quoted(payload, RelationGetRelationName(rel), '"');
-				appendStringInfoCharMacro(payload, ',');
-				appendStringInfoCharMacro(payload, operation);
+				appendStringInfoChar(payload, ',');
+				appendStringInfoChar(payload, operation);
 
 				for (i = 0; i < indnkeyatts; i++)
 				{
 					int			colno = index->indkey.values[i];
 					Form_pg_attribute attr = TupleDescAttr(tupdesc, colno - 1);
 
-					appendStringInfoCharMacro(payload, ',');
+					appendStringInfoChar(payload, ',');
 					strcpy_quoted(payload, NameStr(attr->attname), '"');
-					appendStringInfoCharMacro(payload, '=');
+					appendStringInfoChar(payload, '=');
 					strcpy_quoted(payload, SPI_getvalue(trigtuple, tupdesc, colno), '\'');
 				}
 
-- 
2.38.0

v3-0003-WIP-Annotate-palloc-with-malloc-and-other-compile.patchtext/x-diff; charset=us-asciiDownload
From c877abe961024671beafe55dabdd716df6795578 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:53:33 -0800
Subject: [PATCH v3 03/10] WIP: Annotate palloc() with malloc and other
 compiler attributes

In particularly malloc is useful, allowing the compiler to realize
that the return value does not alias with other data, allowing other
optimizations.

If we were to do this, we'd obviously need to hide these behind
macros to support compilers without those attributes.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/utils/palloc.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h
index 773a5d2c347..642b24b2bd2 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -75,7 +75,11 @@ extern void *MemoryContextAllocExtended(MemoryContext context,
 extern void *MemoryContextAllocAligned(MemoryContext context,
 									   Size size, Size alignto, int flags);
 
-extern void *palloc(Size size);
+extern void *palloc(Size size)
+#if __has_attribute(malloc) && __has_attribute(assume_aligned) && __has_attribute(returns_nonnull) && __has_attribute(warn_unused_result)
+	__attribute__((malloc, assume_aligned(MAXIMUM_ALIGNOF), returns_nonnull, warn_unused_result))
+#endif
+	;
 extern void *palloc0(Size size);
 extern void *palloc_extended(Size size, int flags);
 extern void *palloc_aligned(Size size, Size alignto, int flags);
-- 
2.38.0

v3-0004-pqformat-Move-type-sending-functions-inline-add-p.patchtext/x-diff; charset=us-asciiDownload
From 781f1855202683035ef0c6bddc8a69d4e323b1bc Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 6 Feb 2024 17:40:01 -0800
Subject: [PATCH v3 04/10] pqformat: Move type sending functions inline, add
 pq_begintypsend_with_size()

Combined with the previous commit inlining more functions in
stringinfo this allows many type send functions to be optimized
considerably better, optimizing away the StringInfoData entirely.

The new pq_begintypsend_with_size() is useful to avoid over-allocating for
send functions that only output a small amount of data, e.g. int4send now
allocates 9 bytes, instead of 1024.

Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20200603015559.qc7ry5jqmoxlpu4y@alap3.anarazel.de

<>

Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/libpq/pqformat.h | 53 ++++++++++++++++++++++++++++++++++++
 src/backend/libpq/pqformat.c | 37 -------------------------
 2 files changed, 53 insertions(+), 37 deletions(-)

diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index a710bc18687..62922f58747 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -16,6 +16,7 @@
 #include "lib/stringinfo.h"
 #include "mb/pg_wchar.h"
 #include "port/pg_bswap.h"
+#include "varatt.h"
 
 extern void pq_beginmessage(StringInfo buf, char msgtype);
 extern void pq_beginmessage_reuse(StringInfo buf, char msgtype);
@@ -31,6 +32,58 @@ extern void pq_send_ascii_string(StringInfo buf, const char *str);
 extern void pq_sendfloat4(StringInfo buf, float4 f);
 extern void pq_sendfloat8(StringInfo buf, float8 f);
 
+
+/* --------------------------------
+ *		pq_begintypsend		- initialize for constructing a bytea result
+ * --------------------------------
+ */
+static inline void
+pq_begintypsend(StringInfo buf)
+{
+	initStringInfo(buf);
+	/* Reserve four bytes for the bytea length word */
+	appendStringInfoSpaces(buf, 4);
+}
+
+/* --------------------------------
+ *		pq_begintypsend_with_size - like pq_begintypesend, but with length hint
+ *
+ * This can be used over pq_begintypesend if the caller can cheaply determine
+ * how much data will be sent, reducing the initial size of the
+ * StringInfo. The passed in size need not include the overhead of the length
+ * word.
+ * --------------------------------
+ */
+static inline void
+pq_begintypsend_with_size(StringInfo buf, int size)
+{
+	initStringInfoWithSize(buf, size + 4);
+	/* Reserve four bytes for the bytea length word */
+	appendStringInfoSpaces(buf, 4);
+}
+
+
+/* --------------------------------
+ *		pq_endtypsend	- finish constructing a bytea result
+ *
+ * The data buffer is returned as the palloc'd bytea value.  (We expect
+ * that it will be suitably aligned for this because it has been palloc'd.)
+ * We assume the StringInfoData is just a local variable in the caller and
+ * need not be pfree'd.
+ * --------------------------------
+ */
+static inline bytea *
+pq_endtypsend(StringInfo buf)
+{
+	bytea	   *result = (bytea *) buf->data;
+
+	/* Insert correct length into bytea length word */
+	Assert(buf->len >= VARHDRSZ);
+	SET_VARSIZE(result, buf->len);
+
+	return result;
+}
+
 /*
  * Append a [u]int8 to a StringInfo buffer, which already has enough space
  * preallocated.
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index 4708a7988a6..9275b241333 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -321,43 +321,6 @@ pq_endmessage_reuse(StringInfo buf)
 }
 
 
-/* --------------------------------
- *		pq_begintypsend		- initialize for constructing a bytea result
- * --------------------------------
- */
-void
-pq_begintypsend(StringInfo buf)
-{
-	initStringInfo(buf);
-	/* Reserve four bytes for the bytea length word */
-	appendStringInfoChar(buf, '\0');
-	appendStringInfoChar(buf, '\0');
-	appendStringInfoChar(buf, '\0');
-	appendStringInfoChar(buf, '\0');
-}
-
-/* --------------------------------
- *		pq_endtypsend	- finish constructing a bytea result
- *
- * The data buffer is returned as the palloc'd bytea value.  (We expect
- * that it will be suitably aligned for this because it has been palloc'd.)
- * We assume the StringInfoData is just a local variable in the caller and
- * need not be pfree'd.
- * --------------------------------
- */
-bytea *
-pq_endtypsend(StringInfo buf)
-{
-	bytea	   *result = (bytea *) buf->data;
-
-	/* Insert correct length into bytea length word */
-	Assert(buf->len >= VARHDRSZ);
-	SET_VARSIZE(result, buf->len);
-
-	return result;
-}
-
-
 /* --------------------------------
  *		pq_puttextmessage - generate a character set-converted message in one step
  *
-- 
2.38.0

v3-0005-copy-Use-appendBinaryStringInfoNT-for-sending-bin.patchtext/x-diff; charset=us-asciiDownload
From 953176ae5738758105860868e134858644dfac6c Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 13:00:26 -0800
Subject: [PATCH v3 05/10] copy: Use appendBinaryStringInfoNT() for sending
 binary data

Maintaining the trailing byte is useless overhead.

Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20200603015559.qc7ry5jqmoxlpu4y@alap3.anarazel.de
---
 src/backend/commands/copyto.c                              | 4 ++--
 src/test/modules/test_copy_callbacks/test_copy_callbacks.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index b7f8b04ed8d..5857532f54e 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -175,13 +175,13 @@ SendCopyEnd(CopyToState cstate)
 static void
 CopySendData(CopyToState cstate, const void *databuf, int datasize)
 {
-	appendBinaryStringInfo(cstate->fe_msgbuf, databuf, datasize);
+	appendBinaryStringInfoNT(cstate->fe_msgbuf, databuf, datasize);
 }
 
 static void
 CopySendString(CopyToState cstate, const char *str)
 {
-	appendBinaryStringInfo(cstate->fe_msgbuf, str, strlen(str));
+	appendBinaryStringInfoNT(cstate->fe_msgbuf, str, strlen(str));
 }
 
 static void
diff --git a/src/test/modules/test_copy_callbacks/test_copy_callbacks.c b/src/test/modules/test_copy_callbacks/test_copy_callbacks.c
index 0bbd2aa6bb9..1ba205675af 100644
--- a/src/test/modules/test_copy_callbacks/test_copy_callbacks.c
+++ b/src/test/modules/test_copy_callbacks/test_copy_callbacks.c
@@ -25,8 +25,8 @@ static void
 to_cb(void *data, int len)
 {
 	ereport(NOTICE,
-			(errmsg("COPY TO callback called with data \"%s\" and length %d",
-					(char *) data, len)));
+			(errmsg("COPY TO callback called with data \"%.*s\" and length %d",
+					len, (char *) data, len)));
 }
 
 PG_FUNCTION_INFO_V1(test_copy_to_callback);
-- 
2.38.0

v3-0006-Use-pq_begintypsend_with_size-in-a-number-of-send.patchtext/x-diff; charset=us-asciiDownload
From 1f7ac47f5b6281bd010b5f6451be3f48f8a57151 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 14 Jan 2020 12:58:55 -0800
Subject: [PATCH v3 06/10] Use pq_begintypsend_with_size() in a number of send
 functions

If we were to introduce pq_begintypsend_with_size(), we'd probably want to do
this to quite a few more functions.

Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20200603015559.qc7ry5jqmoxlpu4y@alap3.anarazel.de

<>

Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/utils/adt/bool.c    | 2 +-
 src/backend/utils/adt/float.c   | 4 ++--
 src/backend/utils/adt/int.c     | 4 ++--
 src/backend/utils/adt/int8.c    | 2 +-
 src/backend/utils/adt/uuid.c    | 2 +-
 src/backend/utils/adt/varlena.c | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/backend/utils/adt/bool.c b/src/backend/utils/adt/bool.c
index 85e6786563e..fe188f4f03c 100644
--- a/src/backend/utils/adt/bool.c
+++ b/src/backend/utils/adt/bool.c
@@ -189,7 +189,7 @@ boolsend(PG_FUNCTION_ARGS)
 	bool		arg1 = PG_GETARG_BOOL(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_with_size(&buf, 1);
 	pq_sendbyte(&buf, arg1 ? 1 : 0);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 901edcc8961..4f29c435e2e 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -351,7 +351,7 @@ float4send(PG_FUNCTION_ARGS)
 	float4		num = PG_GETARG_FLOAT4(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_with_size(&buf, 4);
 	pq_sendfloat4(&buf, num);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
@@ -568,7 +568,7 @@ float8send(PG_FUNCTION_ARGS)
 	float8		num = PG_GETARG_FLOAT8(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_with_size(&buf, 8);
 	pq_sendfloat8(&buf, num);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index 234f20796b7..761dc4acce9 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -100,7 +100,7 @@ int2send(PG_FUNCTION_ARGS)
 	int16		arg1 = PG_GETARG_INT16(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_with_size(&buf, 2);
 	pq_sendint16(&buf, arg1);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
@@ -324,7 +324,7 @@ int4send(PG_FUNCTION_ARGS)
 	int32		arg1 = PG_GETARG_INT32(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_with_size(&buf, 4);
 	pq_sendint32(&buf, arg1);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index ede14086aee..5423cb2ecde 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -97,7 +97,7 @@ int8send(PG_FUNCTION_ARGS)
 	int64		arg1 = PG_GETARG_INT64(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_with_size(&buf, 8);
 	pq_sendint64(&buf, arg1);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
diff --git a/src/backend/utils/adt/uuid.c b/src/backend/utils/adt/uuid.c
index 73dfd711c73..5e04640bce3 100644
--- a/src/backend/utils/adt/uuid.c
+++ b/src/backend/utils/adt/uuid.c
@@ -153,7 +153,7 @@ uuid_send(PG_FUNCTION_ARGS)
 	pg_uuid_t  *uuid = PG_GETARG_UUID_P(0);
 	StringInfoData buffer;
 
-	pq_begintypsend(&buffer);
+	pq_begintypsend_with_size(&buffer, UUID_LEN);
 	pq_sendbytes(&buffer, uuid->data, UUID_LEN);
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buffer));
 }
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 01d5d0dbb63..ea696c1dc71 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -621,7 +621,7 @@ textsend(PG_FUNCTION_ARGS)
 	text	   *t = PG_GETARG_TEXT_PP(0);
 	StringInfoData buf;
 
-	pq_begintypsend(&buf);
+	pq_begintypsend_with_size(&buf, VARSIZE_ANY_EXHDR(t));
 	pq_sendtext(&buf, VARDATA_ANY(t), VARSIZE_ANY_EXHDR(t));
 	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
 }
-- 
2.38.0

v3-0007-wip-make-send-calls-in-printtup.c-cheaper.patchtext/x-diff; charset=us-asciiDownload
From bf6ae654768063f95e381145e9cf44969138c5dd Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 2 Jun 2020 16:47:26 -0700
Subject: [PATCH v3 07/10] wip: make send calls in printtup.c cheaper

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/access/common/printtup.c | 40 +++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index 8a4953ea033..b4c031f67e7 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -49,6 +49,15 @@ typedef struct
 	bool		typisvarlena;	/* is it varlena (ie possibly toastable)? */
 	int16		format;			/* format code for this column */
 	FmgrInfo	finfo;			/* Precomputed call info for output fn */
+
+	/* use union with FunctionCallInfoBaseData to guarantee alignment */
+	union
+	{
+		FunctionCallInfoBaseData fcinfo;
+		/* ensure enough space for nargs args is available */
+		char		fcinfo_data[SizeForFunctionCallInfo(1)];
+	}			fcinfo_data;
+
 } PrinttupAttrInfo;
 
 typedef struct
@@ -278,6 +287,9 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
 							  &thisState->typoutput,
 							  &thisState->typisvarlena);
 			fmgr_info(thisState->typoutput, &thisState->finfo);
+			InitFunctionCallInfoData(thisState->fcinfo_data.fcinfo,
+									 &thisState->finfo, 1, InvalidOid,
+									 NULL, NULL);
 		}
 		else if (format == 1)
 		{
@@ -285,6 +297,9 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
 									&thisState->typsend,
 									&thisState->typisvarlena);
 			fmgr_info(thisState->typsend, &thisState->finfo);
+			InitFunctionCallInfoData(thisState->fcinfo_data.fcinfo,
+									 &thisState->finfo, 1, InvalidOid,
+									 NULL, NULL);
 		}
 		else
 			ereport(ERROR,
@@ -361,11 +376,28 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 		{
 			/* Binary output */
 			bytea	   *outputbytes;
+			int			outputlen;
+			Datum		result;
+			FunctionCallInfo fcinfo = &thisState->fcinfo_data.fcinfo;
 
-			outputbytes = SendFunctionCall(&thisState->finfo, attr);
-			pq_sendint32(buf, VARSIZE(outputbytes) - VARHDRSZ);
-			pq_sendbytes(buf, VARDATA(outputbytes),
-						 VARSIZE(outputbytes) - VARHDRSZ);
+			fcinfo->args[0].value = attr;
+			fcinfo->args[0].isnull = false;
+			result = FunctionCallInvoke(fcinfo);
+
+			/*
+			 * Check for null result, since caller is clearly not expecting
+			 * one
+			 */
+			if (unlikely(fcinfo->isnull))
+				elog(ERROR, "send function return null");
+
+			outputbytes = DatumGetByteaP(result);
+			outputlen = VARSIZE(outputbytes) - VARHDRSZ;
+
+			Assert(outputlen > 0);
+
+			pq_sendint32(buf, outputlen);
+			pq_sendbytes(buf, VARDATA(outputbytes), outputlen);
 		}
 	}
 
-- 
2.38.0

v3-0008-wip-make-in-out-send-recv-calls-in-copy.c-cheaper.patchtext/x-diff; charset=us-asciiDownload
From e0b35e463349a8dc8bdce3139ba7b20d4d4b3ca1 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 2 Jun 2020 16:47:26 -0700
Subject: [PATCH v3 08/10] wip: make in/out send/recv calls in copy.c cheaper

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/commands/copyfrom_internal.h | 22 +++++-
 src/backend/access/common/printtup.c     |  2 +-
 src/backend/commands/copyfrom.c          | 35 +++++----
 src/backend/commands/copyfromparse.c     | 96 ++++++++++++++++--------
 src/backend/commands/copyto.c            | 83 ++++++++++++++++----
 src/tools/pgindent/typedefs.list         | 70 ++++++++---------
 6 files changed, 212 insertions(+), 96 deletions(-)

diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index cad52fcc783..1bc51d8a0fc 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -52,6 +52,23 @@ typedef enum CopyInsertMethod
 								 * ExecForeignBatchInsert only if valid */
 } CopyInsertMethod;
 
+typedef struct CopyInAttributeInfo
+{
+	AttrNumber	num;
+	const char *name;
+
+	Oid			typioparam;
+	int32		typmod;
+
+	FmgrInfo	in_finfo;
+	union
+	{
+		FunctionCallInfoBaseData fcinfo;
+		char		fcinfo_data[SizeForFunctionCallInfo(3)];
+	}			in_fcinfo;
+
+} CopyInAttributeInfo;
+
 /*
  * This struct contains all the state variables used throughout a COPY FROM
  * operation.
@@ -93,9 +110,8 @@ typedef struct CopyFromStateData
 
 	AttrNumber	num_defaults;	/* count of att that are missing and have
 								 * default value */
-	FmgrInfo   *in_functions;	/* array of input functions for each attrs */
-	Oid		   *typioparams;	/* array of element types for in_functions */
-	ErrorSaveContext *escontext;	/* soft error trapper during in_functions
+	CopyInAttributeInfo *in_attributes;
+	ErrorSaveContext *escontext;	/* soft error trapper during in_attributes
 									 * execution */
 	uint64		num_errors;		/* total number of rows which contained soft
 								 * errors */
diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index b4c031f67e7..bee3fc26220 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -394,7 +394,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 			outputbytes = DatumGetByteaP(result);
 			outputlen = VARSIZE(outputbytes) - VARHDRSZ;
 
-			Assert(outputlen > 0);
+			Assert(outputlen >= 0);
 
 			pq_sendint32(buf, outputlen);
 			pq_sendbytes(buf, VARDATA(outputbytes), outputlen);
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 1fe70b91338..96e56d3c128 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1384,8 +1384,7 @@ BeginCopyFrom(ParseState *pstate,
 	TupleDesc	tupDesc;
 	AttrNumber	num_phys_attrs,
 				num_defaults;
-	FmgrInfo   *in_functions;
-	Oid		   *typioparams;
+	CopyInAttributeInfo *in_attributes;
 	Oid			in_func_oid;
 	int		   *defmap;
 	ExprState **defexprs;
@@ -1608,27 +1607,36 @@ BeginCopyFrom(ParseState *pstate,
 	 * the input function), and info about defaults and constraints. (Which
 	 * input function we use depends on text/binary format choice.)
 	 */
-	in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
-	typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));
+	in_attributes = (CopyInAttributeInfo *)
+		palloc0(num_phys_attrs * sizeof(CopyInAttributeInfo));
 	defmap = (int *) palloc(num_phys_attrs * sizeof(int));
 	defexprs = (ExprState **) palloc(num_phys_attrs * sizeof(ExprState *));
 
 	for (int attnum = 1; attnum <= num_phys_attrs; attnum++)
 	{
-		Form_pg_attribute att = TupleDescAttr(tupDesc, attnum - 1);
+		CopyInAttributeInfo *attr = &in_attributes[attnum - 1];
+		Form_pg_attribute pgatt = TupleDescAttr(tupDesc, attnum - 1);
 
 		/* We don't need info for dropped attributes */
-		if (att->attisdropped)
+		if (pgatt->attisdropped)
 			continue;
 
+		attr->num = attnum;
+		attr->typmod = pgatt->atttypmod;
+		attr->name = NameStr(pgatt->attname);
+
 		/* Fetch the input function and typioparam info */
 		if (cstate->opts.binary)
-			getTypeBinaryInputInfo(att->atttypid,
-								   &in_func_oid, &typioparams[attnum - 1]);
+			getTypeBinaryInputInfo(pgatt->atttypid,
+								   &in_func_oid, &attr->typioparam);
 		else
-			getTypeInputInfo(att->atttypid,
-							 &in_func_oid, &typioparams[attnum - 1]);
-		fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+			getTypeInputInfo(pgatt->atttypid,
+							 &in_func_oid, &attr->typioparam);
+		fmgr_info(in_func_oid, &attr->in_finfo);
+		InitFunctionCallInfoData(attr->in_fcinfo.fcinfo, &attr->in_finfo, 3,
+								 InvalidOid, (fmNodePtr) cstate->escontext,
+								 NULL);
+
 
 		/* Get default info if available */
 		defexprs[attnum - 1] = NULL;
@@ -1640,7 +1648,7 @@ BeginCopyFrom(ParseState *pstate,
 		 */
 		if ((cstate->opts.default_print != NULL ||
 			 !list_member_int(cstate->attnumlist, attnum)) &&
-			!att->attgenerated)
+			!pgatt->attgenerated)
 		{
 			Expr	   *defexpr = (Expr *) build_column_default(cstate->rel,
 																attnum);
@@ -1689,8 +1697,7 @@ BeginCopyFrom(ParseState *pstate,
 	cstate->bytes_processed = 0;
 
 	/* We keep those variables in cstate. */
-	cstate->in_functions = in_functions;
-	cstate->typioparams = typioparams;
+	cstate->in_attributes = in_attributes;
 	cstate->defmap = defmap;
 	cstate->defexprs = defexprs;
 	cstate->volatile_defexprs = volatile_defexprs;
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 7cacd0b752c..7cf9d10e7ae 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -154,8 +154,8 @@ static bool CopyReadLine(CopyFromState cstate);
 static bool CopyReadLineText(CopyFromState cstate);
 static int	CopyReadAttributesText(CopyFromState cstate);
 static int	CopyReadAttributesCSV(CopyFromState cstate);
-static Datum CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
-									 Oid typioparam, int32 typmod,
+static Datum CopyReadBinaryAttribute(CopyFromState cstate,
+									 CopyInAttributeInfo *att,
 									 bool *isnull);
 
 
@@ -859,8 +859,7 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 	AttrNumber	num_phys_attrs,
 				attr_count,
 				num_defaults = cstate->num_defaults;
-	FmgrInfo   *in_functions = cstate->in_functions;
-	Oid		   *typioparams = cstate->typioparams;
+	CopyInAttributeInfo *in_attributes = cstate->in_attributes;
 	int			i;
 	int		   *defmap = cstate->defmap;
 	ExprState **defexprs = cstate->defexprs;
@@ -899,13 +898,14 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 		{
 			int			attnum = lfirst_int(cur);
 			int			m = attnum - 1;
-			Form_pg_attribute att = TupleDescAttr(tupDesc, m);
+			CopyInAttributeInfo *attr = &in_attributes[m];
+			FunctionCallInfo fcinfo = &attr->in_fcinfo.fcinfo;
 
 			if (fieldno >= fldct)
 				ereport(ERROR,
 						(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
 						 errmsg("missing data for column \"%s\"",
-								NameStr(att->attname))));
+								attr->name)));
 			string = field_strings[fieldno++];
 
 			if (cstate->convert_select_flags &&
@@ -939,7 +939,7 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 				}
 			}
 
-			cstate->cur_attname = NameStr(att->attname);
+			cstate->cur_attname = attr->name;
 			cstate->cur_attval = string;
 
 			if (string != NULL)
@@ -956,20 +956,47 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 
 				values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
 			}
-
-			/*
-			 * If ON_ERROR is specified with IGNORE, skip rows with soft
-			 * errors
-			 */
-			else if (!InputFunctionCallSafe(&in_functions[m],
-											string,
-											typioparams[m],
-											att->atttypmod,
-											(Node *) cstate->escontext,
-											&values[m]))
+			else if (string == NULL && fcinfo->flinfo->fn_strict)
 			{
-				cstate->num_errors++;
-				return true;
+				/* FIXME: shouldn't be reachable */
+				nulls[m] = true;
+			}
+			else
+			{
+				fcinfo->args[0].value = CStringGetDatum(string);
+				fcinfo->args[0].isnull = false;
+				fcinfo->args[1].value = ObjectIdGetDatum(attr->typioparam);
+				fcinfo->args[1].isnull = false;
+				fcinfo->args[2].value = Int32GetDatum(attr->typmod);
+				fcinfo->args[2].isnull = false;
+
+				values[m] = FunctionCallInvoke(fcinfo);
+
+				if (SOFT_ERROR_OCCURRED(cstate->escontext))
+				{
+					/*
+					 * If ON_ERROR is specified with IGNORE, skip rows with
+					 * soft errors
+					 */
+					cstate->num_errors++;
+					return true;
+				}
+
+				/* Should get null result if and only if str is NULL */
+				if (string == NULL)
+				{
+					if (unlikely(!fcinfo->isnull))
+						elog(ERROR, "input function %u returned non-NULL",
+							 fcinfo->flinfo->fn_oid);
+				}
+				else
+				{
+					if (unlikely(fcinfo->isnull))
+						elog(ERROR, "input function %u returned NULL",
+							 fcinfo->flinfo->fn_oid);
+				}
+
+				nulls[m] = string == NULL;
 			}
 
 			cstate->cur_attname = NULL;
@@ -1021,13 +1048,11 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 		{
 			int			attnum = lfirst_int(cur);
 			int			m = attnum - 1;
-			Form_pg_attribute att = TupleDescAttr(tupDesc, m);
+			CopyInAttributeInfo *attr = &in_attributes[m];
 
-			cstate->cur_attname = NameStr(att->attname);
+			cstate->cur_attname = attr->name;
 			values[m] = CopyReadBinaryAttribute(cstate,
-												&in_functions[m],
-												typioparams[m],
-												att->atttypmod,
+												attr,
 												&nulls[m]);
 			cstate->cur_attname = NULL;
 		}
@@ -1952,12 +1977,13 @@ endfield:
  * Read a binary attribute
  */
 static Datum
-CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
-						Oid typioparam, int32 typmod,
+CopyReadBinaryAttribute(CopyFromState cstate,
+						CopyInAttributeInfo *attr,
 						bool *isnull)
 {
 	int32		fld_size;
 	Datum		result;
+	FunctionCallInfo fcinfo = &attr->in_fcinfo.fcinfo;
 
 	if (!CopyGetInt32(cstate, &fld_size))
 		ereport(ERROR,
@@ -1966,7 +1992,7 @@ CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
 	if (fld_size == -1)
 	{
 		*isnull = true;
-		return ReceiveFunctionCall(flinfo, NULL, typioparam, typmod);
+		return ReceiveFunctionCall(fcinfo->flinfo, NULL, attr->typioparam, attr->typmod);
 	}
 	if (fld_size < 0)
 		ereport(ERROR,
@@ -1987,8 +2013,18 @@ CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
 	cstate->attribute_buf.data[fld_size] = '\0';
 
 	/* Call the column type's binary input converter */
-	result = ReceiveFunctionCall(flinfo, &cstate->attribute_buf,
-								 typioparam, typmod);
+	fcinfo->args[0].value = PointerGetDatum(&cstate->attribute_buf);
+	fcinfo->args[0].isnull = false;
+	fcinfo->args[1].value = ObjectIdGetDatum(attr->typioparam);
+	fcinfo->args[1].isnull = false;
+	fcinfo->args[2].value = Int32GetDatum(attr->typmod);
+	fcinfo->args[2].isnull = false;
+
+	result = FunctionCallInvoke(fcinfo);
+
+	if (unlikely(fcinfo->isnull))
+		elog(ERROR, "receive function %u returned NULL",
+			 fcinfo->flinfo->fn_oid);
 
 	/* Trouble if it didn't eat the whole buffer */
 	if (cstate->attribute_buf.cursor != cstate->attribute_buf.len)
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 5857532f54e..b5cada5cb75 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -54,6 +54,20 @@ typedef enum CopyDest
 	COPY_CALLBACK,				/* to callback function */
 } CopyDest;
 
+typedef struct CopyOutAttributeInfo
+{
+	AttrNumber	num;
+	const char *name;
+	int32		typid;
+
+	FmgrInfo	out_finfo;
+	union
+	{
+		FunctionCallInfoBaseData fcinfo;
+		char		fcinfo_data[SizeForFunctionCallInfo(1)];
+	}			out_fcinfo;
+} CopyOutAttributeInfo;
+
 /*
  * This struct contains all the state variables used throughout a COPY TO
  * operation.
@@ -96,7 +110,8 @@ typedef struct CopyToStateData
 	 */
 	MemoryContext copycontext;	/* per-copy execution context */
 
-	FmgrInfo   *out_functions;	/* lookup info for output functions */
+	CopyOutAttributeInfo *out_attributes;
+
 	MemoryContext rowcontext;	/* per-row evaluation context */
 	uint64		bytes_processed;	/* number of bytes processed so far */
 } CopyToStateData;
@@ -768,23 +783,34 @@ DoCopyTo(CopyToState cstate)
 	cstate->fe_msgbuf = makeStringInfo();
 
 	/* Get info about the columns we need to process. */
-	cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
+	cstate->out_attributes =
+		(CopyOutAttributeInfo *) palloc(num_phys_attrs * sizeof(CopyOutAttributeInfo));
 	foreach(cur, cstate->attnumlist)
 	{
 		int			attnum = lfirst_int(cur);
+		CopyOutAttributeInfo *attr = &cstate->out_attributes[attnum - 1];
+		Form_pg_attribute pgatt;
 		Oid			out_func_oid;
 		bool		isvarlena;
-		Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
+
+		pgatt = TupleDescAttr(tupDesc, attnum - 1);
+		attr->num = attnum;
+		attr->name = NameStr(pgatt->attname);
+		attr->typid = pgatt->atttypid;
 
 		if (cstate->opts.binary)
-			getTypeBinaryOutputInfo(attr->atttypid,
+			getTypeBinaryOutputInfo(attr->typid,
 									&out_func_oid,
 									&isvarlena);
 		else
-			getTypeOutputInfo(attr->atttypid,
+			getTypeOutputInfo(attr->typid,
 							  &out_func_oid,
 							  &isvarlena);
-		fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+
+		fmgr_info(out_func_oid, &attr->out_finfo);
+		InitFunctionCallInfoData(attr->out_fcinfo.fcinfo, &attr->out_finfo,
+								 1, InvalidOid,
+								 NULL, NULL);
 	}
 
 	/*
@@ -908,7 +934,7 @@ static void
 CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
 {
 	bool		need_delim = false;
-	FmgrInfo   *out_functions = cstate->out_functions;
+	CopyOutAttributeInfo *out_attributes = cstate->out_attributes;
 	MemoryContext oldcontext;
 	ListCell   *cur;
 	char	   *string;
@@ -928,6 +954,8 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
 	foreach(cur, cstate->attnumlist)
 	{
 		int			attnum = lfirst_int(cur);
+		CopyOutAttributeInfo *attr = &out_attributes[attnum - 1];
+		FunctionCallInfo fcinfo = &attr->out_fcinfo.fcinfo;
 		Datum		value = slot->tts_values[attnum - 1];
 		bool		isnull = slot->tts_isnull[attnum - 1];
 
@@ -949,8 +977,22 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
 		{
 			if (!cstate->opts.binary)
 			{
-				string = OutputFunctionCall(&out_functions[attnum - 1],
-											value);
+				Datum		result;
+
+				fcinfo->args[0].value = value;
+				fcinfo->args[0].isnull = false;
+
+				result = FunctionCallInvoke(fcinfo);
+
+				/*
+				 * Check for null result, since caller is clearly not
+				 * expecting one
+				 */
+				if (unlikely(fcinfo->isnull))
+					elog(ERROR, "send function return null");
+
+				string = DatumGetCString(result);
+
 				if (cstate->opts.csv_mode)
 					CopyAttributeOutCSV(cstate, string,
 										cstate->opts.force_quote_flags[attnum - 1]);
@@ -959,13 +1001,26 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
 			}
 			else
 			{
+				int			outputlen;
+				Datum		result;
 				bytea	   *outputbytes;
 
-				outputbytes = SendFunctionCall(&out_functions[attnum - 1],
-											   value);
-				CopySendInt32(cstate, VARSIZE(outputbytes) - VARHDRSZ);
-				CopySendData(cstate, VARDATA(outputbytes),
-							 VARSIZE(outputbytes) - VARHDRSZ);
+				fcinfo->args[0].value = value;
+				fcinfo->args[0].isnull = false;
+				result = FunctionCallInvoke(fcinfo);
+
+				/*
+				 * Check for null result, since caller is clearly not
+				 * expecting one
+				 */
+				if (unlikely(fcinfo->isnull))
+					elog(ERROR, "send function return null");
+
+				outputbytes = DatumGetByteaP(result);
+				outputlen = VARSIZE(outputbytes) - VARHDRSZ;
+
+				CopySendInt32(cstate, outputlen);
+				CopySendData(cstate, VARDATA(outputbytes), outputlen);
 			}
 		}
 	}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d808aad8b05..2bd1028a5f4 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -273,6 +273,13 @@ BlockId
 BlockIdData
 BlockInfoRecord
 BlockNumber
+BlockRefTable
+BlockRefTableBuffer
+BlockRefTableEntry
+BlockRefTableKey
+BlockRefTableReader
+BlockRefTableSerializedEntry
+BlockRefTableWriter
 BlockSampler
 BlockSamplerData
 BlockedProcData
@@ -373,7 +380,6 @@ CatalogId
 CatalogIdMapEntry
 CatalogIndexState
 ChangeVarNodes_context
-ReplaceVarnoContext
 CheckPoint
 CheckPointStmt
 CheckpointStatsData
@@ -476,10 +482,12 @@ CopyFormatOptions
 CopyFromState
 CopyFromStateData
 CopyHeaderChoice
+CopyInAttributeInfo
 CopyInsertMethod
 CopyMultiInsertBuffer
 CopyMultiInsertInfo
 CopyOnErrorChoice
+CopyOutAttributeInfo
 CopySource
 CopyStmt
 CopyToState
@@ -552,6 +560,8 @@ DR_intorel
 DR_printtup
 DR_sqlfunction
 DR_transientrel
+DSMRegistryCtxStruct
+DSMRegistryEntry
 DWORD
 DataDirSyncMethod
 DataDumperPtr
@@ -612,8 +622,6 @@ DropSubscriptionStmt
 DropTableSpaceStmt
 DropUserMappingStmt
 DropdbStmt
-DSMRegistryCtxStruct
-DSMRegistryEntry
 DumpComponents
 DumpId
 DumpOptions
@@ -748,6 +756,7 @@ FetchStmt
 FieldSelect
 FieldStore
 File
+FileBackupMethod
 FileFdwExecutionState
 FileFdwPlanState
 FileNameMap
@@ -1157,6 +1166,7 @@ InProgressEnt
 IncludeWal
 InclusionOpaque
 IncrementVarSublevelsUp_context
+IncrementalBackupInfo
 IncrementalSort
 IncrementalSortExecutionStatus
 IncrementalSortGroupInfo
@@ -1291,9 +1301,9 @@ JsonManifestWALRangeField
 JsonObjectAgg
 JsonObjectConstructor
 JsonOutput
-JsonParseExpr
 JsonParseContext
 JsonParseErrorType
+JsonParseExpr
 JsonPath
 JsonPathBool
 JsonPathExecContext
@@ -2008,6 +2018,7 @@ PathClauseUsage
 PathCostComparison
 PathHashStack
 PathKey
+PathKeyInfo
 PathKeysComparison
 PathTarget
 PatternInfo
@@ -2359,6 +2370,7 @@ ReorderBufferUpdateProgressTxnCB
 ReorderTuple
 RepOriginId
 ReparameterizeForeignPathByChild_function
+ReplaceVarnoContext
 ReplaceVarsFromTargetList_context
 ReplaceVarsNoMatchOption
 ReplicaIdentityStmt
@@ -2697,6 +2709,7 @@ SubscriptingRefState
 Subscription
 SubscriptionInfo
 SubscriptionRelState
+SummarizerReadLocalXLogPrivate
 SupportRequestCost
 SupportRequestIndexCondition
 SupportRequestOptimizeWindowClause
@@ -2846,7 +2859,6 @@ TocEntry
 TokenAuxData
 TokenizedAuthLine
 TrackItem
-TransamVariablesData
 TransApplyAction
 TransInvalidationInfo
 TransState
@@ -2855,6 +2867,7 @@ TransactionState
 TransactionStateData
 TransactionStmt
 TransactionStmtKind
+TransamVariablesData
 TransformInfo
 TransformJsonStringValuesState
 TransitionCaptureState
@@ -2898,8 +2911,8 @@ TupleTableSlotOps
 TuplesortClusterArg
 TuplesortDatumArg
 TuplesortIndexArg
-TuplesortIndexBrinArg
 TuplesortIndexBTreeArg
+TuplesortIndexBrinArg
 TuplesortIndexHashArg
 TuplesortInstrumentation
 TuplesortMethod
@@ -2945,12 +2958,14 @@ UnicodeNormalizationQC
 Unique
 UniquePath
 UniquePathMethod
+UniqueRelInfo
 UniqueState
 UnlistenStmt
 UnresolvedTup
 UnresolvedTupData
 UpdateContext
 UpdateStmt
+UploadManifestCmd
 UpperRelationKind
 UpperUniquePath
 UserAuth
@@ -2999,7 +3014,6 @@ VolatileFunctionStatus
 Vsrt
 WAIT_ORDER
 WALAvailability
-WalInsertClass
 WALInsertLock
 WALInsertLockPadded
 WALOpenSegment
@@ -3032,6 +3046,7 @@ WaitEventTimeout
 WaitPMResult
 WalCloseMethod
 WalCompression
+WalInsertClass
 WalLevel
 WalRcvData
 WalRcvExecResult
@@ -3045,6 +3060,9 @@ WalSnd
 WalSndCtlData
 WalSndSendDataCallback
 WalSndState
+WalSummarizerData
+WalSummaryFile
+WalSummaryIO
 WalSyncMethod
 WalTimeSample
 WalUsage
@@ -3201,8 +3219,10 @@ avl_node
 avl_tree
 avw_dbase
 backslashResult
+backup_file_entry
 backup_manifest_info
 backup_manifest_option
+backup_wal_range
 base_yy_extra_type
 basebackup_options
 bbsink
@@ -3245,6 +3265,10 @@ cached_re_str
 canonicalize_state
 cashKEY
 catalogid_hash
+cb_cleanup_dir
+cb_options
+cb_tablespace
+cb_tablespace_mapping
 check_agg_arguments_context
 check_function_callback
 check_network_data
@@ -3498,10 +3522,12 @@ macKEY
 macaddr
 macaddr8
 macaddr_sortsupport_state
+manifest_data
 manifest_file
 manifest_files_hash
 manifest_files_iterator
 manifest_wal_range
+manifest_writer
 map_variable_attnos_context
 max_parallel_hazard_context
 mb2wchar_with_len_converter
@@ -3733,6 +3759,7 @@ ret_type
 rewind_source
 rewrite_event
 rf_context
+rfile
 rm_detail_t
 role_auth_extra
 rolename_hash
@@ -3866,7 +3893,6 @@ unicodeStyleColumnFormat
 unicodeStyleFormat
 unicodeStyleRowFormat
 unicode_linestyle
-UniqueRelInfo
 unit_conversion
 unlogged_relation_entry
 utf_local_conversion_func
@@ -3907,6 +3933,8 @@ worker_spi_state
 worker_state
 worktable
 wrap
+ws_file_info
+ws_options
 xl_brin_createidx
 xl_brin_desummarize
 xl_brin_insert
@@ -4026,29 +4054,3 @@ yyscan_t
 z_stream
 z_streamp
 zic_t
-BlockRefTable
-BlockRefTableBuffer
-BlockRefTableEntry
-BlockRefTableKey
-BlockRefTableReader
-BlockRefTableSerializedEntry
-BlockRefTableWriter
-SummarizerReadLocalXLogPrivate
-WalSummarizerData
-WalSummaryFile
-WalSummaryIO
-FileBackupMethod
-IncrementalBackupInfo
-UploadManifestCmd
-backup_file_entry
-backup_wal_range
-cb_cleanup_dir
-cb_options
-cb_tablespace
-cb_tablespace_mapping
-manifest_data
-manifest_writer
-rfile
-ws_options
-ws_file_info
-PathKeyInfo
-- 
2.38.0

v3-0009-inline-pq_sendbytes-stop-maintaining-trailing-nul.patchtext/x-diff; charset=us-asciiDownload
From ce78f7462b933ba9d648280ed34e2f90eb0c17e6 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sat, 17 Feb 2024 13:17:56 -0800
Subject: [PATCH v3 09/10] inline pq_sendbytes, stop maintaining trailing null
 byte

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/libpq/pqformat.h | 12 +++++++++++-
 src/backend/libpq/pqformat.c | 11 -----------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index 62922f58747..bc35bf3f28a 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -23,7 +23,6 @@ extern void pq_beginmessage_reuse(StringInfo buf, char msgtype);
 extern void pq_endmessage(StringInfo buf);
 extern void pq_endmessage_reuse(StringInfo buf);
 
-extern void pq_sendbytes(StringInfo buf, const void *data, int datalen);
 extern void pq_sendcountedtext(StringInfo buf, const char *str, int slen,
 							   bool countincludesself);
 extern void pq_sendtext(StringInfo buf, const char *str, int slen);
@@ -216,6 +215,17 @@ pq_sendbyte(StringInfo buf, uint8 byt)
 	pq_sendint8(buf, byt);
 }
 
+static inline void
+pq_sendbytes(StringInfo buf, const void *data, int datalen)
+{
+	/*
+	 * FIXME: used to say: use variant that maintains a trailing null-byte,
+	 * out of caution but that doesn't make much sense to me, and proves to be
+	 * a performance issue.
+	 */
+	appendBinaryStringInfoNT(buf, data, datalen);
+}
+
 /*
  * Append a binary integer to a StringInfo buffer
  *
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index 9275b241333..807e143cef2 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -118,17 +118,6 @@ pq_beginmessage_reuse(StringInfo buf, char msgtype)
 	buf->cursor = msgtype;
 }
 
-/* --------------------------------
- *		pq_sendbytes	- append raw data to a StringInfo buffer
- * --------------------------------
- */
-void
-pq_sendbytes(StringInfo buf, const void *data, int datalen)
-{
-	/* use variant that maintains a trailing null-byte, out of caution */
-	appendBinaryStringInfo(buf, data, datalen);
-}
-
 /* --------------------------------
  *		pq_sendcountedtext - append a counted text string (with character set conversion)
  *
-- 
2.38.0

v3-0010-heavy-wip-Allow-string-buffer-reuse-in-send-funct.patchtext/x-diff; charset=us-asciiDownload
From c06aa2636a47812ef17d8be7d7e43a8a49e88403 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sat, 17 Feb 2024 17:38:47 -0800
Subject: [PATCH v3 10/10] heavy-wip: Allow string buffer reuse in send
 functions

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/libpq/pqformat.h         | 19 +++++++++++++++++--
 src/backend/access/common/printtup.c |  9 +++++++--
 src/backend/commands/copyto.c        |  8 +++++++-
 src/backend/utils/adt/int.c          | 10 ++++++----
 src/backend/utils/adt/int8.c         |  9 +++++----
 src/backend/utils/adt/varlena.c      | 10 ++++++----
 src/backend/utils/fmgr/fmgr.c        | 17 ++++++++++++++++-
 7 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index bc35bf3f28a..7792691b4cf 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -40,8 +40,16 @@ static inline void
 pq_begintypsend(StringInfo buf)
 {
 	initStringInfo(buf);
-	/* Reserve four bytes for the bytea length word */
-	appendStringInfoSpaces(buf, 4);
+
+	/*
+	 * Reserve four bytes for the bytea length word.  We don't need to fill
+	 * them with anything (pq_endtypsend will do that), and this function is
+	 * enough of a hot spot that it's worth cheating to save some cycles. Note
+	 * in particular that we don't bother to guarantee that the buffer is
+	 * null-terminated.
+	 */
+	Assert(buf->maxlen > 4);
+	buf->len = 4;
 }
 
 /* --------------------------------
@@ -61,6 +69,13 @@ pq_begintypsend_with_size(StringInfo buf, int size)
 	appendStringInfoSpaces(buf, 4);
 }
 
+static inline void
+pq_begintypsend_res(StringInfo buf)
+{
+	Assert(buf && buf->data && buf->len == 0);
+
+	buf->len = 4;
+}
 
 /* --------------------------------
  *		pq_endtypsend	- finish constructing a bytea result
diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index bee3fc26220..d7d6d12f242 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -69,6 +69,7 @@ typedef struct
 	int			nattrs;
 	PrinttupAttrInfo *myinfo;	/* Cached info about each attr */
 	StringInfoData buf;			/* output buffer (*not* in tmpcontext) */
+	StringInfoData fieldbuf;	/* FIXME */
 	MemoryContext tmpcontext;	/* Memory context for per-row workspace */
 } DR_printtup;
 
@@ -128,6 +129,8 @@ printtup_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 	 */
 	initStringInfo(&myState->buf);
 
+	initStringInfo(&myState->fieldbuf);
+
 	/*
 	 * Create a temporary memory context that we can reset once per row to
 	 * recover palloc'd memory.  This avoids any problems with leaks inside
@@ -289,7 +292,7 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
 			fmgr_info(thisState->typoutput, &thisState->finfo);
 			InitFunctionCallInfoData(thisState->fcinfo_data.fcinfo,
 									 &thisState->finfo, 1, InvalidOid,
-									 NULL, NULL);
+									 (Node *) &myState->fieldbuf, NULL);
 		}
 		else if (format == 1)
 		{
@@ -299,7 +302,7 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
 			fmgr_info(thisState->typsend, &thisState->finfo);
 			InitFunctionCallInfoData(thisState->fcinfo_data.fcinfo,
 									 &thisState->finfo, 1, InvalidOid,
-									 NULL, NULL);
+									 (Node *) &myState->fieldbuf, NULL);
 		}
 		else
 			ereport(ERROR,
@@ -319,6 +322,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 	DR_printtup *myState = (DR_printtup *) self;
 	MemoryContext oldcontext;
 	StringInfo	buf = &myState->buf;
+	StringInfo	fieldbuf = &myState->fieldbuf;
 	int			natts = typeinfo->natts;
 	int			i;
 
@@ -398,6 +402,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 
 			pq_sendint32(buf, outputlen);
 			pq_sendbytes(buf, VARDATA(outputbytes), outputlen);
+			resetStringInfo(fieldbuf);
 		}
 	}
 
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index b5cada5cb75..dd552db4828 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -110,6 +110,8 @@ typedef struct CopyToStateData
 	 */
 	MemoryContext copycontext;	/* per-copy execution context */
 
+	StringInfoData attribute_buf;
+
 	CopyOutAttributeInfo *out_attributes;
 
 	MemoryContext rowcontext;	/* per-row evaluation context */
@@ -782,6 +784,8 @@ DoCopyTo(CopyToState cstate)
 	/* We use fe_msgbuf as a per-row buffer regardless of copy_dest */
 	cstate->fe_msgbuf = makeStringInfo();
 
+	initStringInfo(&cstate->attribute_buf);
+
 	/* Get info about the columns we need to process. */
 	cstate->out_attributes =
 		(CopyOutAttributeInfo *) palloc(num_phys_attrs * sizeof(CopyOutAttributeInfo));
@@ -810,7 +814,7 @@ DoCopyTo(CopyToState cstate)
 		fmgr_info(out_func_oid, &attr->out_finfo);
 		InitFunctionCallInfoData(attr->out_fcinfo.fcinfo, &attr->out_finfo,
 								 1, InvalidOid,
-								 NULL, NULL);
+								 (Node *) &cstate->attribute_buf, NULL);
 	}
 
 	/*
@@ -938,6 +942,7 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
 	MemoryContext oldcontext;
 	ListCell   *cur;
 	char	   *string;
+	StringInfo	attribute_buf = &cstate->attribute_buf;
 
 	MemoryContextReset(cstate->rowcontext);
 	oldcontext = MemoryContextSwitchTo(cstate->rowcontext);
@@ -1021,6 +1026,7 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
 
 				CopySendInt32(cstate, outputlen);
 				CopySendData(cstate, VARDATA(outputbytes), outputlen);
+				resetStringInfo(attribute_buf);
 			}
 		}
 	}
diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index 761dc4acce9..2fdf2b247de 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -322,11 +322,13 @@ Datum
 int4send(PG_FUNCTION_ARGS)
 {
 	int32		arg1 = PG_GETARG_INT32(0);
-	StringInfoData buf;
+	StringInfo	buf;
 
-	pq_begintypsend_with_size(&buf, 4);
-	pq_sendint32(&buf, arg1);
-	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
+	buf = (StringInfo) fcinfo->context;
+
+	pq_begintypsend_res(buf);
+	pq_sendint32(buf, arg1);
+	PG_RETURN_BYTEA_P(pq_endtypsend(buf));
 }
 
 
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index 5423cb2ecde..a171125b110 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -95,11 +95,12 @@ Datum
 int8send(PG_FUNCTION_ARGS)
 {
 	int64		arg1 = PG_GETARG_INT64(0);
-	StringInfoData buf;
+	StringInfo	buf;
 
-	pq_begintypsend_with_size(&buf, 8);
-	pq_sendint64(&buf, arg1);
-	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
+	buf = (StringInfo) fcinfo->context;
+	pq_begintypsend_res(buf);
+	pq_sendint64(buf, arg1);
+	PG_RETURN_BYTEA_P(pq_endtypsend(buf));
 }
 
 
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index ea696c1dc71..387e368726e 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -619,11 +619,13 @@ Datum
 textsend(PG_FUNCTION_ARGS)
 {
 	text	   *t = PG_GETARG_TEXT_PP(0);
-	StringInfoData buf;
+	StringInfo	buf = (StringInfo) fcinfo->context;
 
-	pq_begintypsend_with_size(&buf, VARSIZE_ANY_EXHDR(t));
-	pq_sendtext(&buf, VARDATA_ANY(t), VARSIZE_ANY_EXHDR(t));
-	PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
+	Assert(fcinfo->context);
+
+	pq_begintypsend_res(buf);
+	pq_sendtext(buf, VARDATA_ANY(t), VARSIZE_ANY_EXHDR(t));
+	PG_RETURN_BYTEA_P(pq_endtypsend(buf));
 }
 
 
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index e48a86be54b..5677a9a6bdf 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -1743,7 +1743,22 @@ ReceiveFunctionCall(FmgrInfo *flinfo, StringInfo buf,
 bytea *
 SendFunctionCall(FmgrInfo *flinfo, Datum val)
 {
-	return DatumGetByteaP(FunctionCall1(flinfo, val));
+	StringInfoData buf;
+	Datum		result;
+
+	LOCAL_FCINFO(fcinfo, 1);
+
+	initStringInfo(&buf);
+
+	InitFunctionCallInfoData(*fcinfo, flinfo, 1, InvalidOid, (Node *) &buf, NULL);
+	fcinfo->args[0].value = val;
+	fcinfo->args[0].isnull = false;
+	result = FunctionCallInvoke(fcinfo);
+
+	if (fcinfo->isnull)
+		elog(ERROR, "function %u returned NULL", flinfo->fn_oid);
+
+	return DatumGetByteaP(result);
 }
 
 /*
-- 
2.38.0

#24Sutou Kouhei
kou@clear-code.com
In reply to: Andres Freund (#23)
Re: Why is pq_begintypsend so slow?

Hi,

In <20240218015955.rmw5mcmobt5hbene@awork3.anarazel.de>
"Re: Why is pq_begintypsend so slow?" on Sat, 17 Feb 2024 17:59:55 -0800,
Andres Freund <andres@anarazel.de> wrote:

v3-0008-wip-make-in-out-send-recv-calls-in-copy.c-cheaper.patch

It seems that this is an alternative approach of [1]..

[1]: .

+typedef struct CopyInAttributeInfo
+{
+	AttrNumber	num;
+	const char *name;
+
+	Oid			typioparam;
+	int32		typmod;
+
+	FmgrInfo	in_finfo;
+	union
+	{
+		FunctionCallInfoBaseData fcinfo;
+		char		fcinfo_data[SizeForFunctionCallInfo(3)];
+	}			in_fcinfo;

Do we need one FunctionCallInfoBaseData for each attribute?
How about sharing one FunctionCallInfoBaseData by all
attributes like [1].?

@@ -956,20 +956,47 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,

values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
}
-
- /*
- * If ON_ERROR is specified with IGNORE, skip rows with soft
- * errors
- */
- else if (!InputFunctionCallSafe(&in_functions[m],
- string,
- typioparams[m],
- att->atttypmod,
- (Node *) cstate->escontext,
- &values[m]))

Inlining InputFuncallCallSafe() here to use pre-initialized
fcinfo will decrease maintainability. Because we abstract
InputFunctionCall family in fmgr.c. If we define a
InputFunctionCall variant here, we need to change both of
fmgr.c and here when InputFunctionCall family is changed.
How about defining details in fmgr.c and call it here
instead like [1].?

+				fcinfo->args[0].value = CStringGetDatum(string);
+				fcinfo->args[0].isnull = false;
+				fcinfo->args[1].value = ObjectIdGetDatum(attr->typioparam);
+				fcinfo->args[1].isnull = false;
+				fcinfo->args[2].value = Int32GetDatum(attr->typmod);
+				fcinfo->args[2].isnull = false;

I think that "fcinfo->isnull = false;" is also needed like
[1]: .

@@ -1966,7 +1992,7 @@ CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
 	if (fld_size == -1)
 	{
 		*isnull = true;
-		return ReceiveFunctionCall(flinfo, NULL, typioparam, typmod);
+		return ReceiveFunctionCall(fcinfo->flinfo, NULL, attr->typioparam, attr->typmod);

Why pre-initialized fcinfo isn't used here?

Thanks,
--
kou

#25Andres Freund
andres@anarazel.de
In reply to: Sutou Kouhei (#24)
Re: Why is pq_begintypsend so slow?

Hi,

On 2024-02-18 17:38:09 +0900, Sutou Kouhei wrote:

In <20240218015955.rmw5mcmobt5hbene@awork3.anarazel.de>
"Re: Why is pq_begintypsend so slow?" on Sat, 17 Feb 2024 17:59:55 -0800,
Andres Freund <andres@anarazel.de> wrote:

v3-0008-wip-make-in-out-send-recv-calls-in-copy.c-cheaper.patch

It seems that this is an alternative approach of [1].

Note that what I posted was a very lightly polished rebase of a ~4 year old
patchset.

[1] /messages/by-id/20240215.153421.96888103784986803.kou@clear-code.com

+typedef struct CopyInAttributeInfo
+{
+	AttrNumber	num;
+	const char *name;
+
+	Oid			typioparam;
+	int32		typmod;
+
+	FmgrInfo	in_finfo;
+	union
+	{
+		FunctionCallInfoBaseData fcinfo;
+		char		fcinfo_data[SizeForFunctionCallInfo(3)];
+	}			in_fcinfo;

Do we need one FunctionCallInfoBaseData for each attribute?
How about sharing one FunctionCallInfoBaseData by all
attributes like [1]?

That makes no sense to me. You're throwing away most of the possible gains by
having to update the FunctionCallInfo fields on every call. You're saving
neglegible amounts of memory at a substantial runtime cost.

@@ -956,20 +956,47 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,

values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
}
-
- /*
- * If ON_ERROR is specified with IGNORE, skip rows with soft
- * errors
- */
- else if (!InputFunctionCallSafe(&in_functions[m],
- string,
- typioparams[m],
- att->atttypmod,
- (Node *) cstate->escontext,
- &values[m]))

Inlining InputFuncallCallSafe() here to use pre-initialized
fcinfo will decrease maintainability. Because we abstract
InputFunctionCall family in fmgr.c. If we define a
InputFunctionCall variant here, we need to change both of
fmgr.c and here when InputFunctionCall family is changed.
How about defining details in fmgr.c and call it here
instead like [1]?

I'm not sure I buy that that is a problem. It's not like my approach was
actually bypassing fmgr abstractions alltogether - instead it just used the
lower level APIs, because it's a performance sensitive area.

@@ -1966,7 +1992,7 @@ CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
if (fld_size == -1)
{
*isnull = true;
-		return ReceiveFunctionCall(flinfo, NULL, typioparam, typmod);
+		return ReceiveFunctionCall(fcinfo->flinfo, NULL, attr->typioparam, attr->typmod);

Why pre-initialized fcinfo isn't used here?

Because it's a prototype and because I don't think it's a common path.

Greetings,

Andres Freund

#26Michael Paquier
michael@paquier.xyz
In reply to: Andres Freund (#25)
Re: Why is pq_begintypsend so slow?

On Sun, Feb 18, 2024 at 12:09:06PM -0800, Andres Freund wrote:

On 2024-02-18 17:38:09 +0900, Sutou Kouhei wrote:

@@ -1966,7 +1992,7 @@ CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
if (fld_size == -1)
{
*isnull = true;
-		return ReceiveFunctionCall(flinfo, NULL, typioparam, typmod);
+		return ReceiveFunctionCall(fcinfo->flinfo, NULL, attr->typioparam, attr->typmod);

Why pre-initialized fcinfo isn't used here?

Because it's a prototype and because I don't think it's a common path.

0008 and 0010 (a bit) are the only patches of the set that touch some
of the areas that would be impacted by the refactoring to use
callbacks in the COPY code, still I don't see anything that could not
be changed in what's updated here, the one-row callback in COPY FROM
being the most touched. So I don't quite see why each effort could
not happen on their own?

Or Andres, do you think that any improvements you've been proposing in
this area should happen before we consider refactoring the COPY code
to plug in the callbacks? I'm a bit confused by the situation, TBH.
--
Michael

#27Sutou Kouhei
kou@clear-code.com
In reply to: Andres Freund (#25)
Re: Why is pq_begintypsend so slow?

Hi,

In <20240218200906.zvihkrs46yl2juzf@awork3.anarazel.de>
"Re: Why is pq_begintypsend so slow?" on Sun, 18 Feb 2024 12:09:06 -0800,
Andres Freund <andres@anarazel.de> wrote:

[1] /messages/by-id/20240215.153421.96888103784986803.kou@clear-code.com

Do we need one FunctionCallInfoBaseData for each attribute?
How about sharing one FunctionCallInfoBaseData by all
attributes like [1]?

That makes no sense to me. You're throwing away most of the possible gains by
having to update the FunctionCallInfo fields on every call. You're saving
neglegible amounts of memory at a substantial runtime cost.

The number of updated fields of your approach and [1]provides some optimized abstractions, which are implemented with lower level APIs, without breaking the abstractions. are
same:

Your approach: 6 (I think that "fcinfo->isnull = false" is
needed though.)

+				fcinfo->args[0].value = CStringGetDatum(string);
+				fcinfo->args[0].isnull = false;
+				fcinfo->args[1].value = ObjectIdGetDatum(attr->typioparam);
+				fcinfo->args[1].isnull = false;
+				fcinfo->args[2].value = Int32GetDatum(attr->typmod);
+				fcinfo->args[2].isnull = false;

[1]: provides some optimized abstractions, which are implemented with lower level APIs, without breaking the abstractions.

+	fcinfo->flinfo = flinfo;
+	fcinfo->context = escontext;
+	fcinfo->isnull = false;
+	fcinfo->args[0].value = CStringGetDatum(str);
+	fcinfo->args[1].value = ObjectIdGetDatum(typioparam);
+	fcinfo->args[2].value = Int32GetDatum(typmod);

Inlining InputFuncallCallSafe() here to use pre-initialized
fcinfo will decrease maintainability. Because we abstract
InputFunctionCall family in fmgr.c. If we define a
InputFunctionCall variant here, we need to change both of
fmgr.c and here when InputFunctionCall family is changed.
How about defining details in fmgr.c and call it here
instead like [1]?

I'm not sure I buy that that is a problem. It's not like my approach was
actually bypassing fmgr abstractions alltogether - instead it just used the
lower level APIs, because it's a performance sensitive area.

[1]: provides some optimized abstractions, which are implemented with lower level APIs, without breaking the abstractions.
implemented with lower level APIs, without breaking the
abstractions.

Note that I don't have a strong opinion how to implement
this optimization. If other developers think this approach
makes sense for this optimization, I don't object it.

@@ -1966,7 +1992,7 @@ CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
if (fld_size == -1)
{
*isnull = true;
-		return ReceiveFunctionCall(flinfo, NULL, typioparam, typmod);
+		return ReceiveFunctionCall(fcinfo->flinfo, NULL, attr->typioparam, attr->typmod);

Why pre-initialized fcinfo isn't used here?

Because it's a prototype and because I don't think it's a common path.

How about adding a comment why we don't need to optimize
this case?

I don't have a strong opinion how to implement this
optimization as I said above. It seems that you like your
approach. So I withdraw [1]provides some optimized abstractions, which are implemented with lower level APIs, without breaking the abstractions.. Could you complete this
optimization? Can we proceed making COPY format extensible
without this optimization? It seems that it'll take a little
time to complete this optimization because your patch is
still WIP. And it seems that you can work on it after making
COPY format extensible.

Thanks,
--
kou

#28Andres Freund
andres@anarazel.de
In reply to: Sutou Kouhei (#27)
Re: Why is pq_begintypsend so slow?

Hi,

On 2024-02-19 10:02:52 +0900, Sutou Kouhei wrote:

In <20240218200906.zvihkrs46yl2juzf@awork3.anarazel.de>
"Re: Why is pq_begintypsend so slow?" on Sun, 18 Feb 2024 12:09:06 -0800,
Andres Freund <andres@anarazel.de> wrote:

[1] /messages/by-id/20240215.153421.96888103784986803.kou@clear-code.com

Do we need one FunctionCallInfoBaseData for each attribute?
How about sharing one FunctionCallInfoBaseData by all
attributes like [1]?

That makes no sense to me. You're throwing away most of the possible gains by
having to update the FunctionCallInfo fields on every call. You're saving
neglegible amounts of memory at a substantial runtime cost.

The number of updated fields of your approach and [1] are
same:

Your approach: 6 (I think that "fcinfo->isnull = false" is
needed though.)

+				fcinfo->args[0].value = CStringGetDatum(string);
+				fcinfo->args[0].isnull = false;
+				fcinfo->args[1].value = ObjectIdGetDatum(attr->typioparam);
+				fcinfo->args[1].isnull = false;
+				fcinfo->args[2].value = Int32GetDatum(attr->typmod);
+				fcinfo->args[2].isnull = false;

[1]: 6 (including "fcinfo->isnull = false")

+	fcinfo->flinfo = flinfo;
+	fcinfo->context = escontext;
+	fcinfo->isnull = false;
+	fcinfo->args[0].value = CStringGetDatum(str);
+	fcinfo->args[1].value = ObjectIdGetDatum(typioparam);
+	fcinfo->args[2].value = Int32GetDatum(typmod);

If you want to do so you can elide the isnull assignments in my approach just
as well as yours. Assigning not just the value but also flinfo and context is
overhead. But you can't elide assigning flinfo and context, which is why
reusing one FunctionCallInfo isn't going to win

I don't think you necessarily need to assign fcinfo->isnull on every call,
these functions aren't allowed to return NULL IIRC. And if they do we'd error
out, so it could only happen once.

I don't have a strong opinion how to implement this
optimization as I said above. It seems that you like your
approach. So I withdraw [1]. Could you complete this
optimization? Can we proceed making COPY format extensible
without this optimization? It seems that it'll take a little
time to complete this optimization because your patch is
still WIP. And it seems that you can work on it after making
COPY format extensible.

I don't think optimizing this aspect needs to block making copy extensible.

I don't know how much time/energy I'll have to focus on this in the near
term. I really just reposted this because the earlier patches were relevant
for the discussion in another thread. If you want to pick the COPY part up,
feel free.

Greetings,

Andres Freund

#29Sutou Kouhei
kou@clear-code.com
In reply to: Andres Freund (#28)
Re: Why is pq_begintypsend so slow?

Hi,

In <20240219195351.5vy7cdl3wxia66kg@awork3.anarazel.de>
"Re: Why is pq_begintypsend so slow?" on Mon, 19 Feb 2024 11:53:51 -0800,
Andres Freund <andres@anarazel.de> wrote:

I don't have a strong opinion how to implement this
optimization as I said above. It seems that you like your
approach. So I withdraw [1]. Could you complete this
optimization? Can we proceed making COPY format extensible
without this optimization? It seems that it'll take a little
time to complete this optimization because your patch is
still WIP. And it seems that you can work on it after making
COPY format extensible.

I don't think optimizing this aspect needs to block making copy extensible.

OK. I'll work on making copy extensible without this
optimization.

I don't know how much time/energy I'll have to focus on this in the near
term. I really just reposted this because the earlier patches were relevant
for the discussion in another thread. If you want to pick the COPY part up,
feel free.

OK. I may work on this after I complete making copy
extensible if you haven't completed this yet.

Thanks,
--
kou