I found a couple of adjacent weird things:
There are a bunch of places in the json code that use
appendBinaryStringInfo() where appendStringInfoString() could be used, e.g.,
appendBinaryStringInfo(buf, ".size()", 7);
Is there a reason for this? Are we that stretched for performance? I
find this kind of code very fragile.
Also, the argument type of appendBinaryStringInfo() is char *. 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. I think
functions taking binary data plus length should take void * instead,
like memcpy() for example.
Attached are two patches that illustrate these issues and show proposed
changes.From 15b103edec2f89a63596d112fd62cfc2367576bf Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 19 Dec 2022 07:01:27 +0100
Subject: [PATCH 1/2] Use appendStringInfoString instead of
appendBinaryStringInfo where possible
---
src/backend/utils/adt/jsonb.c | 10 ++++----
src/backend/utils/adt/jsonpath.c | 42 ++++++++++++++++----------------
2 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 7c1e5e6144..c153c87ba6 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -361,7 +361,7 @@ jsonb_put_escaped_value(StringInfo out, JsonbValue
*scalarVal)
switch (scalarVal->type)
{
case jbvNull:
- appendBinaryStringInfo(out, "null", 4);
+ appendStringInfoString(out, "null");
break;
case jbvString:
escape_json(out, pnstrdup(scalarVal->val.string.val,
scalarVal->val.string.len));
@@ -373,9 +373,9 @@ jsonb_put_escaped_value(StringInfo out, JsonbValue
*scalarVal)
break;
case jbvBool:
if (scalarVal->val.boolean)
- appendBinaryStringInfo(out, "true", 4);
+ appendStringInfoString(out, "true");
else
- appendBinaryStringInfo(out, "false", 5);
+ appendStringInfoString(out, "false");
break;
default:
elog(ERROR, "unknown jsonb scalar type");
@@ -565,7 +565,7 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in,
int estimated_len, bool
/* json rules guarantee this is a string */
jsonb_put_escaped_value(out, &v);
- appendBinaryStringInfo(out, ": ", 2);
+ appendStringInfoString(out, ": ");
type = JsonbIteratorNext(&it, &v, false);
if (type == WJB_VALUE)
@@ -630,7 +630,7 @@ add_indent(StringInfo out, bool indent, int level)
appendStringInfoCharMacro(out, '\n');
for (i = 0; i < level; i++)
- appendBinaryStringInfo(out, " ", 4);
+ appendStringInfoString(out, " ");
}
}
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: 7122f9d5437789312cb0a7e26e853bb8d2e57add
--
2.39.0
From 2e765008d1314eb6d8a3bf5e7fb9c00e93a86461 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 19 Dec 2022 07:01:27 +0100
Subject: [PATCH 2/2] Change argument of appendBinaryStringInfo from char * to
void *
---
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
