On 19.12.22 23:48, David Rowley wrote:
On Tue, 20 Dec 2022 at 11:42, Tom Lane <t...@sss.pgh.pa.us> wrote:
I think Peter is entirely right to question whether *this* type's
output function is performance-critical. Who's got large tables with
jsonpath columns? It seems to me the type would mostly only exist
as constants within queries.
The patch touches code in the path of jsonb's output function too. I
don't think you could claim the same for that.
Ok, let's leave the jsonb output alone. The jsonb output code also
won't change a lot, but there is a bunch of stuff for jsonpath on the
horizon, so having some more robust coding style to imitate there seems
useful. Here is another patch set with the jsonb changes omitted.
From 319c206bec80d326808a211f9edd50346edbeb8c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 19 Dec 2022 07:01:27 +0100
Subject: [PATCH v2 1/2] Use appendStringInfoString instead of
appendBinaryStringInfo where possible
For the jsonpath output, we don't need to squeeze out every bit of
performance, so instead use a more robust coding style. There are
similar calls in jsonb.c, which we leave alone here since there is
indeed a performance impact for bulk exports.
Discussion:
https://www.postgresql.org/message-id/flat/a0086cfc-ff0f-2827-20fe-52b591d2666c%40enterprisedb.com
---
src/backend/utils/adt/jsonpath.c | 42 ++++++++++++++++----------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/src/backend/utils/adt/jsonpath.c b/src/backend/utils/adt/jsonpath.c
index 91af030095..a39eab9c20 100644
--- a/src/backend/utils/adt/jsonpath.c
+++ b/src/backend/utils/adt/jsonpath.c
@@ -213,7 +213,7 @@ jsonPathToCstring(StringInfo out, JsonPath *in, int
estimated_len)
enlargeStringInfo(out, estimated_len);
if (!(in->header & JSONPATH_LAX))
- appendBinaryStringInfo(out, "strict ", 7);
+ appendStringInfoString(out, "strict ");
jspInit(&v, in);
printJsonPathItem(out, &v, false, true);
@@ -510,9 +510,9 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
break;
case jpiBool:
if (jspGetBool(v))
- appendBinaryStringInfo(buf, "true", 4);
+ appendStringInfoString(buf, "true");
else
- appendBinaryStringInfo(buf, "false", 5);
+ appendStringInfoString(buf, "false");
break;
case jpiAnd:
case jpiOr:
@@ -553,13 +553,13 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
operationPriority(elem.type) <=
operationPriority(v->type));
- appendBinaryStringInfo(buf, " like_regex ", 12);
+ appendStringInfoString(buf, " like_regex ");
escape_json(buf, v->content.like_regex.pattern);
if (v->content.like_regex.flags)
{
- appendBinaryStringInfo(buf, " flag \"", 7);
+ appendStringInfoString(buf, " flag \"");
if (v->content.like_regex.flags &
JSP_REGEX_ICASE)
appendStringInfoChar(buf, 'i');
@@ -591,13 +591,13 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
appendStringInfoChar(buf, ')');
break;
case jpiFilter:
- appendBinaryStringInfo(buf, "?(", 2);
+ appendStringInfoString(buf, "?(");
jspGetArg(v, &elem);
printJsonPathItem(buf, &elem, false, false);
appendStringInfoChar(buf, ')');
break;
case jpiNot:
- appendBinaryStringInfo(buf, "!(", 2);
+ appendStringInfoString(buf, "!(");
jspGetArg(v, &elem);
printJsonPathItem(buf, &elem, false, false);
appendStringInfoChar(buf, ')');
@@ -606,10 +606,10 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
appendStringInfoChar(buf, '(');
jspGetArg(v, &elem);
printJsonPathItem(buf, &elem, false, false);
- appendBinaryStringInfo(buf, ") is unknown", 12);
+ appendStringInfoString(buf, ") is unknown");
break;
case jpiExists:
- appendBinaryStringInfo(buf, "exists (", 8);
+ appendStringInfoString(buf, "exists (");
jspGetArg(v, &elem);
printJsonPathItem(buf, &elem, false, false);
appendStringInfoChar(buf, ')');
@@ -623,10 +623,10 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
appendStringInfoChar(buf, '$');
break;
case jpiLast:
- appendBinaryStringInfo(buf, "last", 4);
+ appendStringInfoString(buf, "last");
break;
case jpiAnyArray:
- appendBinaryStringInfo(buf, "[*]", 3);
+ appendStringInfoString(buf, "[*]");
break;
case jpiAnyKey:
if (inKey)
@@ -648,7 +648,7 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
if (range)
{
- appendBinaryStringInfo(buf, " to ", 4);
+ appendStringInfoString(buf, " to ");
printJsonPathItem(buf, &to, false,
false);
}
}
@@ -660,7 +660,7 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
if (v->content.anybounds.first == 0 &&
v->content.anybounds.last == PG_UINT32_MAX)
- appendBinaryStringInfo(buf, "**", 2);
+ appendStringInfoString(buf, "**");
else if (v->content.anybounds.first ==
v->content.anybounds.last)
{
if (v->content.anybounds.first == PG_UINT32_MAX)
@@ -681,25 +681,25 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
v->content.anybounds.last);
break;
case jpiType:
- appendBinaryStringInfo(buf, ".type()", 7);
+ appendStringInfoString(buf, ".type()");
break;
case jpiSize:
- appendBinaryStringInfo(buf, ".size()", 7);
+ appendStringInfoString(buf, ".size()");
break;
case jpiAbs:
- appendBinaryStringInfo(buf, ".abs()", 6);
+ appendStringInfoString(buf, ".abs()");
break;
case jpiFloor:
- appendBinaryStringInfo(buf, ".floor()", 8);
+ appendStringInfoString(buf, ".floor()");
break;
case jpiCeiling:
- appendBinaryStringInfo(buf, ".ceiling()", 10);
+ appendStringInfoString(buf, ".ceiling()");
break;
case jpiDouble:
- appendBinaryStringInfo(buf, ".double()", 9);
+ appendStringInfoString(buf, ".double()");
break;
case jpiDatetime:
- appendBinaryStringInfo(buf, ".datetime(", 10);
+ appendStringInfoString(buf, ".datetime(");
if (v->content.arg)
{
jspGetArg(v, &elem);
@@ -708,7 +708,7 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
appendStringInfoChar(buf, ')');
break;
case jpiKeyValue:
- appendBinaryStringInfo(buf, ".keyvalue()", 11);
+ appendStringInfoString(buf, ".keyvalue()");
break;
default:
elog(ERROR, "unrecognized jsonpath item type: %d",
v->type);
base-commit: f4f2f2b84a0bbf9edbfc4a8684040a941cd6d085
--
2.39.0
From a56c77066e750ca9d7afcf1f16679d8dcd9af06f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 19 Dec 2022 07:01:27 +0100
Subject: [PATCH v2 2/2] Change argument of appendBinaryStringInfo from char *
to void *
There is some code that uses this function to assemble some kind of
packed binary layout, which requires a bunch of casts because of this.
Functions taking binary data plus length should take void * instead,
like memcpy() for example.
Discussion:
https://www.postgresql.org/message-id/flat/a0086cfc-ff0f-2827-20fe-52b591d2666c%40enterprisedb.com
---
src/backend/utils/adt/jsonpath.c | 18 +++++++++---------
src/backend/utils/adt/xid8funcs.c | 4 ++--
src/common/stringinfo.c | 4 ++--
src/include/lib/stringinfo.h | 4 ++--
4 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/backend/utils/adt/jsonpath.c b/src/backend/utils/adt/jsonpath.c
index a39eab9c20..acc8fd97f1 100644
--- a/src/backend/utils/adt/jsonpath.c
+++ b/src/backend/utils/adt/jsonpath.c
@@ -258,18 +258,18 @@ flattenJsonPathParseItem(StringInfo buf,
JsonPathParseItem *item,
case jpiString:
case jpiVariable:
case jpiKey:
- appendBinaryStringInfo(buf, (char *)
&item->value.string.len,
+ appendBinaryStringInfo(buf, &item->value.string.len,
sizeof(item->value.string.len));
appendBinaryStringInfo(buf, item->value.string.val,
item->value.string.len);
appendStringInfoChar(buf, '\0');
break;
case jpiNumeric:
- appendBinaryStringInfo(buf, (char *)
item->value.numeric,
+ appendBinaryStringInfo(buf, item->value.numeric,
VARSIZE(item->value.numeric));
break;
case jpiBool:
- appendBinaryStringInfo(buf, (char *)
&item->value.boolean,
+ appendBinaryStringInfo(buf, &item->value.boolean,
sizeof(item->value.boolean));
break;
case jpiAnd:
@@ -313,11 +313,11 @@ flattenJsonPathParseItem(StringInfo buf,
JsonPathParseItem *item,
int32 offs;
appendBinaryStringInfo(buf,
-
(char *) &item->value.like_regex.flags,
+
&item->value.like_regex.flags,
sizeof(item->value.like_regex.flags));
offs = reserveSpaceForItemPointer(buf);
appendBinaryStringInfo(buf,
-
(char *) &item->value.like_regex.patternlen,
+
&item->value.like_regex.patternlen,
sizeof(item->value.like_regex.patternlen));
appendBinaryStringInfo(buf,
item->value.like_regex.pattern,
item->value.like_regex.patternlen);
@@ -373,7 +373,7 @@ flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem
*item,
int offset;
int i;
- appendBinaryStringInfo(buf, (char *) &nelems,
sizeof(nelems));
+ appendBinaryStringInfo(buf, &nelems,
sizeof(nelems));
offset = buf->len;
@@ -404,10 +404,10 @@ flattenJsonPathParseItem(StringInfo buf,
JsonPathParseItem *item,
break;
case jpiAny:
appendBinaryStringInfo(buf,
- (char *)
&item->value.anybounds.first,
+
&item->value.anybounds.first,
sizeof(item->value.anybounds.first));
appendBinaryStringInfo(buf,
- (char *)
&item->value.anybounds.last,
+
&item->value.anybounds.last,
sizeof(item->value.anybounds.last));
break;
case jpiType:
@@ -464,7 +464,7 @@ reserveSpaceForItemPointer(StringInfo buf)
int32 pos = buf->len;
int32 ptr = 0;
- appendBinaryStringInfo(buf, (char *) &ptr, sizeof(ptr));
+ appendBinaryStringInfo(buf, &ptr, sizeof(ptr));
return pos;
}
diff --git a/src/backend/utils/adt/xid8funcs.c
b/src/backend/utils/adt/xid8funcs.c
index 3baf5f7d90..15d4db4b70 100644
--- a/src/backend/utils/adt/xid8funcs.c
+++ b/src/backend/utils/adt/xid8funcs.c
@@ -260,7 +260,7 @@ buf_init(FullTransactionId xmin, FullTransactionId xmax)
snap.nxip = 0;
buf = makeStringInfo();
- appendBinaryStringInfo(buf, (char *) &snap, PG_SNAPSHOT_SIZE(0));
+ appendBinaryStringInfo(buf, &snap, PG_SNAPSHOT_SIZE(0));
return buf;
}
@@ -272,7 +272,7 @@ buf_add_txid(StringInfo buf, FullTransactionId fxid)
/* do this before possible realloc */
snap->nxip++;
- appendBinaryStringInfo(buf, (char *) &fxid, sizeof(fxid));
+ appendBinaryStringInfo(buf, &fxid, sizeof(fxid));
}
static pg_snapshot *
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 76ff4d3e24..66a64180c9 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -224,7 +224,7 @@ appendStringInfoSpaces(StringInfo str, int count)
* if necessary. Ensures that a trailing null byte is present.
*/
void
-appendBinaryStringInfo(StringInfo str, const char *data, int datalen)
+appendBinaryStringInfo(StringInfo str, const void *data, int datalen)
{
Assert(str != NULL);
@@ -250,7 +250,7 @@ appendBinaryStringInfo(StringInfo str, const char *data,
int datalen)
* if necessary. Does not ensure a trailing null-byte exists.
*/
void
-appendBinaryStringInfoNT(StringInfo str, const char *data, int datalen)
+appendBinaryStringInfoNT(StringInfo str, const void *data, int datalen)
{
Assert(str != NULL);
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 9b755c4883..63ea1eca63 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -142,7 +142,7 @@ extern void appendStringInfoSpaces(StringInfo str, int
count);
* if necessary.
*/
extern void appendBinaryStringInfo(StringInfo str,
- const char
*data, int datalen);
+ const void
*data, int datalen);
/*------------------------
* appendBinaryStringInfoNT
@@ -150,7 +150,7 @@ extern void appendBinaryStringInfo(StringInfo str,
* if necessary. Does not ensure a trailing null-byte exists.
*/
extern void appendBinaryStringInfoNT(StringInfo str,
- const
char *data, int datalen);
+ const
void *data, int datalen);
/*------------------------
* enlargeStringInfo
--
2.39.0
