David Rowley <dgrowle...@gmail.com> writes: > On Fri, 30 Aug 2024 at 03:33, Tom Lane <t...@sss.pgh.pa.us> wrote: >> >> David Rowley <dgrowle...@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
>From 5ace763a5126478da1c3cb68d5221d83e45d2f34 Mon Sep 17 00:00:00 2001 From: Andy Fan <zhihuifan1...@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
