From: Andy Fan <zhihuifan1213@163.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
  pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Make printtup a bit faster
In-Reply-To: 
 <CAApHDvoE1EiHeQaHKh+NnB8GqhSVDUTjkHovTcXzp0W-d0yK0g@mail.gmail.com>
	(David Rowley's message of "Fri, 30 Aug 2024 12:31:07 +1200")
References: <87wmjzfz0h.fsf@163.com>
	<CAApHDvrBNA-QRsbn-SJyRAsywjHCNenfcbi14c3O3c7=OimQ8Q@mail.gmail.com>
	<3601908.1724945588@sss.pgh.pa.us>
	<CAApHDvoE1EiHeQaHKh+NnB8GqhSVDUTjkHovTcXzp0W-d0yK0g@mail.gmail.com>
Date: Fri, 30 Aug 2024 13:00:50 +0800
Message-ID: <87v7zihaf1.fsf@163.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: <https://www.postgresql.org/message-id/87v7zihaf1.fsf%40163.com>
Precedence: bulk

--=-=-=
Content-Type: text/plain

David Rowley <dgrowleyml@gmail.com> writes:

> On Fri, 30 Aug 2024 at 03:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> David Rowley <dgrowleyml@gmail.com> writes:
>> > [ redesign I/O function APIs ]
>> > I had planned to work on this for PG18, but I'd be happy for some
>> > assistance if you're willing.
>>
>> I'm skeptical that such a thing will ever be practical.  To avoid
>> breaking un-converted data types, all the call sites would have to
>> support both old and new APIs.  To avoid breaking non-core callers,
>> all the I/O functions would have to support both old and new APIs.
>> That probably adds enough overhead to negate whatever benefit you'd
>> get.
>
> So, we currently return cstrings in our output functions. Let's take
> jsonb_out() as an example, to build that cstring, we make a *new*
> StringInfoData on *every call* inside JsonbToCStringWorker(). That
> gives you 1024 bytes before you need to enlarge it. However, it's
> maybe not all bad as we have some size estimations there to call
> enlargeStringInfo(), only that's a bit wasteful as it does a
> repalloc() which memcpys the freshly allocated 1024 bytes allocated in
> initStringInfo() when it doesn't yet contain any data. After
> jsonb_out() has returned and we have the cstring, only we forgot the
> length of the string, so most places will immediately call strlen() or
> do that indirectly via appendStringInfoString(). For larger JSON
> documents, that'll likely require pulling cachelines back into L1
> again. I don't know how modern CPU cacheline eviction works, but if it
> was as simple as FIFO then the strlen() would flush all those
> cachelines only for memcpy() to have to read them back again for
> output strings larger than L1.

The attached is PoC of this idea, not matter which method are adopted
(rewrite all the outfunction or a optional print function), I think the
benefit will be similar. In the blew test case, it shows us 10%+
improvements. (0.134ms vs 0.110ms)

create table demo as select oid as oid1, relname::text as text1, relam,
relname::text as text2 from pg_class; 

pgbench:  
select * from demo;

-- 
Best Regards
Andy Fan


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=v20240830-0001-Avoiding-some-memcpy-strlen-palloc-in-prin.patch

From 5ace763a5126478da1c3cb68d5221d83e45d2f34 Mon Sep 17 00:00:00 2001
From: Andy Fan <zhihuifan1213@163.com>
Date: Fri, 30 Aug 2024 12:50:54 +0800
Subject: [PATCH v20240830 1/1] Avoiding some memcpy, strlen, palloc in
 printtup.

https://www.postgresql.org/message-id/87wmjzfz0h.fsf%40163.com
---
 src/backend/access/common/printtup.c | 40 ++++++++++++++++++++++------
 src/backend/utils/adt/oid.c          | 20 ++++++++++++++
 src/backend/utils/adt/varlena.c      | 17 ++++++++++++
 src/include/catalog/pg_proc.dat      |  9 ++++++-
 4 files changed, 77 insertions(+), 9 deletions(-)

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index c78cc39308..ecba4a7113 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -19,6 +19,7 @@
 #include "libpq/pqformat.h"
 #include "libpq/protocol.h"
 #include "tcop/pquery.h"
+#include "utils/fmgroids.h"
 #include "utils/lsyscache.h"
 #include "utils/memdebug.h"
 #include "utils/memutils.h"
@@ -49,6 +50,7 @@ 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 */
+	FmgrInfo	p_finfo;        /* Precomputed call info for print fn if any */
 } PrinttupAttrInfo;
 
 typedef struct
@@ -274,10 +276,25 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
 		thisState->format = format;
 		if (format == 0)
 		{
-			getTypeOutputInfo(attr->atttypid,
-							  &thisState->typoutput,
-							  &thisState->typisvarlena);
-			fmgr_info(thisState->typoutput, &thisState->finfo);
+			/*
+			 * If the type defines a print function, then use it
+			 * rather than outfunction.
+			 *
+			 * XXX: need a generic function to improve the if-elseif.
+			 */
+			if (attr->atttypid == OIDOID)
+				fmgr_info(F_OIDPRINT, &thisState->p_finfo);
+			else if (attr->atttypid == TEXTOID)
+				fmgr_info(F_TEXTPRINT, &thisState->p_finfo);
+			else
+			{
+				getTypeOutputInfo(attr->atttypid,
+								  &thisState->typoutput,
+								  &thisState->typisvarlena);
+				fmgr_info(thisState->typoutput, &thisState->finfo);
+				/* mark print function is invalid */
+				thisState->p_finfo.fn_oid = InvalidOid;
+			}
 		}
 		else if (format == 1)
 		{
@@ -355,10 +372,17 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 		if (thisState->format == 0)
 		{
 			/* Text output */
-			char	   *outputstr;
-
-			outputstr = OutputFunctionCall(&thisState->finfo, attr);
-			pq_sendcountedtext(buf, outputstr, strlen(outputstr));
+			if (thisState->p_finfo.fn_oid)
+			{
+				FunctionCall2(&thisState->p_finfo, attr, PointerGetDatum(buf));
+			}
+			else
+			{
+				char	   *outputstr;
+
+				outputstr = OutputFunctionCall(&thisState->finfo, attr);
+				pq_sendcountedtext(buf, outputstr, strlen(outputstr));
+			}
 		}
 		else
 		{
diff --git a/src/backend/utils/adt/oid.c b/src/backend/utils/adt/oid.c
index 56fb1fd77c..cc85d920c8 100644
--- a/src/backend/utils/adt/oid.c
+++ b/src/backend/utils/adt/oid.c
@@ -53,6 +53,26 @@ oidout(PG_FUNCTION_ARGS)
 	PG_RETURN_CSTRING(result);
 }
 
+Datum
+oidprint(PG_FUNCTION_ARGS)
+{
+	Oid			o = PG_GETARG_OID(0);
+	StringInfo	buf = (StringInfo) PG_GETARG_POINTER(1);
+	uint32 *lenp;
+	uint32 data_len;
+
+	/* 12 is the max length for an oid's text presentation. */
+	enlargeStringInfo(buf, sizeof(int32) + 12);
+
+	/* note the position for len */
+	lenp = (uint32 *) (buf->data + buf->len);
+	data_len = pg_snprintf(buf->data + buf->len + sizeof(int), 12, "%u", o);
+	*lenp = pg_hton32(data_len);
+	buf->len += sizeof(uint32) + data_len;
+
+	PG_RETURN_VOID();
+}
+
 /*
  *		oidrecv			- converts external binary format to oid
  */
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 7c6391a276..3b7006d54a 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -594,6 +594,23 @@ textout(PG_FUNCTION_ARGS)
 	PG_RETURN_CSTRING(TextDatumGetCString(txt));
 }
 
+
+Datum
+textprint(PG_FUNCTION_ARGS)
+{
+	text		*txt = (text *) pg_detoast_datum((struct varlena *)PG_GETARG_POINTER(0));
+	StringInfo	buf = (StringInfo) PG_GETARG_POINTER(1);
+	uint32 text_len = VARSIZE(txt) - VARHDRSZ;
+	uint32 ni = pg_hton32(text_len);
+
+	enlargeStringInfo(buf, sizeof(int32) + text_len);
+	memcpy((char *pg_restrict) buf->data + buf->len, &ni, sizeof(uint32));
+	memcpy(buf->data + buf->len + sizeof(int), VARDATA(txt), text_len);
+	buf->len += sizeof(uint32) + text_len;
+
+	PG_RETURN_VOID();
+}
+
 /*
  *		textrecv			- converts external binary format to text
  */
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 85f42be1b3..74eeead4de 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -100,6 +100,10 @@
 { oid => '47', descr => 'I/O',
   proname => 'textout', prorettype => 'cstring', proargtypes => 'text',
   prosrc => 'textout' },
+{
+  oid => '8907', descr => 'I/O',
+  proname => 'textprint', prorettype => 'void', proargtypes => 'text internal',
+  prosrc => 'textprint' },
 { oid => '48', descr => 'I/O',
   proname => 'tidin', prorettype => 'tid', proargtypes => 'cstring',
   prosrc => 'tidin' },
@@ -4718,7 +4722,10 @@
 { oid => '1799', descr => 'I/O',
   proname => 'oidout', prorettype => 'cstring', proargtypes => 'oid',
   prosrc => 'oidout' },
-
+{
+  oid => '9771', descr => 'I/O',
+  proname => 'oidprint', prorettype => 'void', proargtypes => 'oid internal',
+  prosrc => 'oidprint'},
 { oid => '3058', descr => 'concatenate values',
   proname => 'concat', provariadic => 'any', proisstrict => 'f',
   provolatile => 's', prorettype => 'text', proargtypes => 'any',
-- 
2.45.1


--=-=-=--