Improve a few appendStringInfo calls new to v18
Looks like v18 has grown a few appendStringInfo misusages, e.g. using
appendStringInfo() when no formatting is needed or just using format
"%s" instead of using appendStringInfoString().
I've attached a couple of patches. The 0001 is just my method for
finding these, not for commit. 0002 contains the fixes to commit.
Any objections to doing this soonish? Or in a few weeks?
David
Attachments:
v1-0001-Adjust-code-to-highlight-appendStringInfo-misusag.patchapplication/octet-stream; name=v1-0001-Adjust-code-to-highlight-appendStringInfo-misusag.patchDownload
From 3af7a3f179707bd458aaed06a341894fd7e81267 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Thu, 15 Sep 2022 21:27:00 +1200
Subject: [PATCH v1 1/2] Adjust code to highlight appendStringInfo misusages
Not intended for commit
---
src/backend/nodes/outfuncs.c | 40 ++++++++++++++++++------------------
src/backend/utils/adt/xml.c | 2 +-
src/common/stringinfo.c | 6 +++---
src/include/lib/stringinfo.h | 28 ++++++++++++++++++++++---
4 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index ceac3fd8620..52277ec01cd 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -45,92 +45,92 @@ static void outDouble(StringInfo str, double d);
/* Write an integer field (anything written as ":fldname %d") */
#define WRITE_INT_FIELD(fldname) \
- appendStringInfo(str, " :" CppAsString(fldname) " %d", node->fldname)
+ appendStringInfoInternal(str, " :" CppAsString(fldname) " %d", node->fldname)
/* Write an unsigned integer field (anything written as ":fldname %u") */
#define WRITE_UINT_FIELD(fldname) \
- appendStringInfo(str, " :" CppAsString(fldname) " %u", node->fldname)
+ appendStringInfoInternal(str, " :" CppAsString(fldname) " %u", node->fldname)
/* Write an unsigned integer field (anything written with UINT64_FORMAT) */
#define WRITE_UINT64_FIELD(fldname) \
- appendStringInfo(str, " :" CppAsString(fldname) " " UINT64_FORMAT, \
+ appendStringInfoInternal(str, " :" CppAsString(fldname) " " UINT64_FORMAT, \
node->fldname)
/* Write an OID field (don't hard-wire assumption that OID is same as uint) */
#define WRITE_OID_FIELD(fldname) \
- appendStringInfo(str, " :" CppAsString(fldname) " %u", node->fldname)
+ appendStringInfoInternal(str, " :" CppAsString(fldname) " %u", node->fldname)
/* Write a long-integer field */
#define WRITE_LONG_FIELD(fldname) \
- appendStringInfo(str, " :" CppAsString(fldname) " %ld", node->fldname)
+ appendStringInfoInternal(str, " :" CppAsString(fldname) " %ld", node->fldname)
/* Write a char field (ie, one ascii character) */
#define WRITE_CHAR_FIELD(fldname) \
- (appendStringInfo(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
outChar(str, node->fldname))
/* Write an enumerated-type field as an integer code */
#define WRITE_ENUM_FIELD(fldname, enumtype) \
- appendStringInfo(str, " :" CppAsString(fldname) " %d", \
+ appendStringInfoInternal(str, " :" CppAsString(fldname) " %d", \
(int) node->fldname)
/* Write a float field (actually, they're double) */
#define WRITE_FLOAT_FIELD(fldname) \
- (appendStringInfo(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoInternal(str, " :" CppAsString(fldname) " "), \
outDouble(str, node->fldname))
/* Write a boolean field */
#define WRITE_BOOL_FIELD(fldname) \
- appendStringInfo(str, " :" CppAsString(fldname) " %s", \
+ appendStringInfoInternal(str, " :" CppAsString(fldname) " %s", \
booltostr(node->fldname))
/* Write a character-string (possibly NULL) field */
#define WRITE_STRING_FIELD(fldname) \
- (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
outToken(str, node->fldname))
/* Write a parse location field (actually same as INT case) */
#define WRITE_LOCATION_FIELD(fldname) \
- appendStringInfo(str, " :" CppAsString(fldname) " %d", write_location_fields ? node->fldname : -1)
+ appendStringInfoInternal(str, " :" CppAsString(fldname) " %d", write_location_fields ? node->fldname : -1)
/* Write a Node field */
#define WRITE_NODE_FIELD(fldname) \
- (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
outNode(str, node->fldname))
/* Write a bitmapset field */
#define WRITE_BITMAPSET_FIELD(fldname) \
- (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
outBitmapset(str, node->fldname))
/* Write a variable-length array (not a List) of Node pointers */
#define WRITE_NODE_ARRAY(fldname, len) \
- (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
writeNodeArray(str, (const Node * const *) node->fldname, len))
/* Write a variable-length array of AttrNumber */
#define WRITE_ATTRNUMBER_ARRAY(fldname, len) \
- (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
writeAttrNumberCols(str, node->fldname, len))
/* Write a variable-length array of Oid */
#define WRITE_OID_ARRAY(fldname, len) \
- (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
writeOidCols(str, node->fldname, len))
/* Write a variable-length array of Index */
#define WRITE_INDEX_ARRAY(fldname, len) \
- (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
writeIndexCols(str, node->fldname, len))
/* Write a variable-length array of int */
#define WRITE_INT_ARRAY(fldname, len) \
- (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
writeIntCols(str, node->fldname, len))
/* Write a variable-length array of bool */
#define WRITE_BOOL_ARRAY(fldname, len) \
- (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+ (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \
writeBoolCols(str, node->fldname, len))
#define booltostr(x) ((x) ? "true" : "false")
@@ -236,7 +236,7 @@ fnname(StringInfo str, const datatype *arr, int len) \
appendStringInfoChar(str, ')'); \
} \
else \
- appendStringInfoString(str, "<>"); \
+ appendStringInfoStringInternal(str, "<>"); \
}
WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, " %d",)
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index db8d0d6a7e8..08083076f65 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -2198,7 +2198,7 @@ xml_errorHandler(void *data, PgXmlErrorPtr error)
void *errCtxSaved = xmlGenericErrorContext;
xmlSetGenericErrorFunc(errorBuf,
- (xmlGenericErrorFunc) appendStringInfo);
+ (xmlGenericErrorFunc) appendStringInfoInternal);
/* Add context information to errorBuf */
appendStringInfoLineSeparator(errorBuf);
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 22d03807697..1a0ea23d8d5 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -142,7 +142,7 @@ resetStringInfo(StringInfo str)
* strcat.
*/
void
-appendStringInfo(StringInfo str, const char *fmt,...)
+appendStringInfoInternal(StringInfo str, const char *fmt, ...)
{
int save_errno = errno;
@@ -221,13 +221,13 @@ appendStringInfoVA(StringInfo str, const char *fmt, va_list args)
}
/*
- * appendStringInfoString
+ * appendStringInfoStringInternal
*
* Append a null-terminated string to str.
* Like appendStringInfo(str, "%s", s) but faster.
*/
void
-appendStringInfoString(StringInfo str, const char *s)
+appendStringInfoStringInternal(StringInfo str, const char *s)
{
appendBinaryStringInfo(str, s, strlen(s));
}
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index c96df989bb0..24e9bcf68da 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -196,9 +196,20 @@ extern void resetStringInfo(StringInfo str);
* to str if necessary. This is sort of like a combination of sprintf and
* strcat.
*/
-extern void appendStringInfo(StringInfo str, const char *fmt,...) pg_attribute_printf(2, 3);
+#if defined(HAVE__BUILTIN_CONSTANT_P) && defined(USE_ASSERT_CHECKING)
+#define appendStringInfo(str, fmt, ...) \
+ do { \
+ StaticAssertStmt(!__builtin_constant_p(fmt) || strcmp(fmt, "%s") != 0, \
+ "use appendStringInfoString instead of appendStringInfo with %s"); \
+ appendStringInfoInternal(str, fmt, __VA_ARGS__); \
+ } while (0)
+#else
+#define appendStringInfo(str, fmt, ...) appendStringInfoInternal(str, fmt, __VA_ARGS__)
+#endif
-/*------------------------
+extern void appendStringInfoInternal(StringInfo str, const char *fmt,...) pg_attribute_printf(2, 3);
+
+ /*------------------------
* appendStringInfoVA
* Attempt to format text data under the control of fmt (an sprintf-style
* format string) and append it to whatever is already in str. If successful
@@ -214,7 +225,18 @@ 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);
+#if defined(HAVE__BUILTIN_CONSTANT_P) && defined(USE_ASSERT_CHECKING)
+#define appendStringInfoString(str, s) \
+ do { \
+ StaticAssertStmt(!__builtin_constant_p(s) || strlen(s) != 1, \
+ "use appendStringInfoChar to append single characters to a string"); \
+ appendStringInfoStringInternal(str, s); \
+ } while (0)
+#else
+#define appendStringInfoString(str, s) appendStringInfoStringInternal(str, s)
+#endif
+
+extern void appendStringInfoStringInternal(StringInfo str, const char *s);
/*------------------------
* appendStringInfoChar
--
2.43.0
v1-0002-Improve-various-appendStringInfo-calls-that-are-n.patchapplication/octet-stream; name=v1-0002-Improve-various-appendStringInfo-calls-that-are-n.patchDownload
From 558dcc487a06c16d41464a45906c2e6b022fcc6c Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Thu, 10 Apr 2025 15:40:32 +1200
Subject: [PATCH v1 2/2] Improve various appendStringInfo calls that are new to
v18
Here we adjust a few new locations which were not using the most suitable
appendStringInfo* function for the intended purpose.
---
contrib/dblink/dblink.c | 2 +-
contrib/pg_overexplain/pg_overexplain.c | 18 +++++++++---------
src/backend/commands/explain.c | 4 ++--
.../libpqwalreceiver/libpqwalreceiver.c | 2 +-
src/backend/replication/logical/conflict.c | 6 +++---
src/backend/replication/logical/relation.c | 2 +-
src/backend/replication/logical/tablesync.c | 12 ++++++------
src/backend/utils/adt/ri_triggers.c | 12 ++++++------
src/bin/pg_dump/pg_restore.c | 2 +-
9 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index c2e695e364e..092f0753ff5 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -3228,7 +3228,7 @@ appendSCRAMKeysInfo(StringInfo buf)
appendStringInfo(buf, "scram_client_key='%s' ", client_key);
appendStringInfo(buf, "scram_server_key='%s' ", server_key);
- appendStringInfo(buf, "require_auth='scram-sha-256' ");
+ appendStringInfoString(buf, "require_auth='scram-sha-256' ");
pfree(client_key);
pfree(server_key);
diff --git a/contrib/pg_overexplain/pg_overexplain.c b/contrib/pg_overexplain/pg_overexplain.c
index afc34ad5f2f..68c40fc98ea 100644
--- a/contrib/pg_overexplain/pg_overexplain.c
+++ b/contrib/pg_overexplain/pg_overexplain.c
@@ -292,7 +292,7 @@ overexplain_debug(PlannedStmt *plannedstmt, ExplainState *es)
if (es->format == EXPLAIN_FORMAT_TEXT)
{
ExplainIndentText(es);
- appendStringInfo(es->str, "PlannedStmt:\n");
+ appendStringInfoString(es->str, "PlannedStmt:\n");
es->indent++;
}
@@ -329,19 +329,19 @@ overexplain_debug(PlannedStmt *plannedstmt, ExplainState *es)
/* Print various properties as a comma-separated list of flags. */
initStringInfo(&flags);
if (plannedstmt->hasReturning)
- appendStringInfo(&flags, ", hasReturning");
+ appendStringInfoString(&flags, ", hasReturning");
if (plannedstmt->hasModifyingCTE)
- appendStringInfo(&flags, ", hasModifyingCTE");
+ appendStringInfoString(&flags, ", hasModifyingCTE");
if (plannedstmt->canSetTag)
- appendStringInfo(&flags, ", canSetTag");
+ appendStringInfoString(&flags, ", canSetTag");
if (plannedstmt->transientPlan)
- appendStringInfo(&flags, ", transientPlan");
+ appendStringInfoString(&flags, ", transientPlan");
if (plannedstmt->dependsOnRole)
- appendStringInfo(&flags, ", dependsOnRole");
+ appendStringInfoString(&flags, ", dependsOnRole");
if (plannedstmt->parallelModeNeeded)
- appendStringInfo(&flags, ", parallelModeNeeded");
+ appendStringInfoString(&flags, ", parallelModeNeeded");
if (flags.len == 0)
- appendStringInfo(&flags, ", none");
+ appendStringInfoString(&flags, ", none");
ExplainPropertyText("Flags", flags.data + 2, es);
/* Various lists of integers. */
@@ -763,7 +763,7 @@ overexplain_intlist(const char *qlabel, List *list, ExplainState *es)
}
else
{
- appendStringInfo(&buf, " not an integer list");
+ appendStringInfoString(&buf, " not an integer list");
Assert(false);
}
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index ef8aa489af8..786ee865f14 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1848,7 +1848,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
if (es->format == EXPLAIN_FORMAT_TEXT)
{
- appendStringInfo(es->str, " (actual ");
+ appendStringInfoString(es->str, " (actual ");
if (es->timing)
appendStringInfo(es->str, "time=%.3f..%.3f ", startup_ms, total_ms);
@@ -1917,7 +1917,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
if (es->format == EXPLAIN_FORMAT_TEXT)
{
ExplainIndentText(es);
- appendStringInfo(es->str, "actual ");
+ appendStringInfoString(es->str, "actual ");
if (es->timing)
appendStringInfo(es->str, "time=%.3f..%.3f ", startup_ms, total_ms);
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 200e8f1aabd..7b4ddf7a8f5 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -1012,7 +1012,7 @@ libpqrcv_alter_slot(WalReceiverConn *conn, const char *slotname,
*failover ? "true" : "false");
if (failover && two_phase)
- appendStringInfo(&cmd, ", ");
+ appendStringInfoString(&cmd, ", ");
if (two_phase)
appendStringInfo(&cmd, "TWO_PHASE %s",
diff --git a/src/backend/replication/logical/conflict.c b/src/backend/replication/logical/conflict.c
index f1e92f2fc1a..97c4e26b586 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -262,7 +262,7 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo,
break;
case CT_UPDATE_MISSING:
- appendStringInfo(&err_detail, _("Could not find the row to be updated."));
+ appendStringInfoString(&err_detail, _("Could not find the row to be updated."));
break;
case CT_DELETE_ORIGIN_DIFFERS:
@@ -281,7 +281,7 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo,
break;
case CT_DELETE_MISSING:
- appendStringInfo(&err_detail, _("Could not find the row to be deleted."));
+ appendStringInfoString(&err_detail, _("Could not find the row to be deleted."));
break;
}
@@ -304,7 +304,7 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo,
if (err_msg->len > 0)
appendStringInfoChar(err_msg, '\n');
- appendStringInfo(err_msg, "%s", err_detail.data);
+ appendStringInfoString(err_msg, err_detail.data);
}
/*
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 46d15b824e3..f59046ad620 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -238,7 +238,7 @@ logicalrep_get_attrs_str(LogicalRepRelation *remoterel, Bitmapset *atts)
{
attcnt++;
if (attcnt > 1)
- appendStringInfo(&attsbuf, _(", "));
+ appendStringInfoString(&attsbuf, _(", "));
appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
}
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 65b98aa905f..8e1e8762f62 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -940,15 +940,15 @@ fetch_remote_table_info(char *nspname, char *relname, LogicalRepRelation *lrel,
* Now fetch column names and types.
*/
resetStringInfo(&cmd);
- appendStringInfo(&cmd,
- "SELECT a.attnum,"
- " a.attname,"
- " a.atttypid,"
- " a.attnum = ANY(i.indkey)");
+ appendStringInfoString(&cmd,
+ "SELECT a.attnum,"
+ " a.attname,"
+ " a.atttypid,"
+ " a.attnum = ANY(i.indkey)");
/* Generated columns can be replicated since version 18. */
if (server_version >= 180000)
- appendStringInfo(&cmd, ", a.attgenerated != ''");
+ appendStringInfoString(&cmd, ", a.attgenerated != ''");
appendStringInfo(&cmd,
" FROM pg_catalog.pg_attribute a"
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index c4ff18ce65e..6239900fa28 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -428,13 +428,13 @@ RI_FKey_check(TriggerData *trigdata)
{
Oid fk_type = RIAttType(fk_rel, riinfo->fk_attnums[riinfo->nkeys - 1]);
- appendStringInfo(&querybuf, ") x1 HAVING ");
+ appendStringInfoString(&querybuf, ") x1 HAVING ");
sprintf(paramname, "$%d", riinfo->nkeys);
ri_GenerateQual(&querybuf, "",
paramname, fk_type,
riinfo->agged_period_contained_by_oper,
"pg_catalog.range_agg", ANYMULTIRANGEOID);
- appendStringInfo(&querybuf, "(x1.r)");
+ appendStringInfoString(&querybuf, "(x1.r)");
}
/* Prepare and save the plan */
@@ -597,13 +597,13 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
{
Oid fk_type = RIAttType(fk_rel, riinfo->fk_attnums[riinfo->nkeys - 1]);
- appendStringInfo(&querybuf, ") x1 HAVING ");
+ appendStringInfoString(&querybuf, ") x1 HAVING ");
sprintf(paramname, "$%d", riinfo->nkeys);
ri_GenerateQual(&querybuf, "",
paramname, fk_type,
riinfo->agged_period_contained_by_oper,
"pg_catalog.range_agg", ANYMULTIRANGEOID);
- appendStringInfo(&querybuf, "(x1.r)");
+ appendStringInfoString(&querybuf, "(x1.r)");
}
/* Prepare and save the plan */
@@ -838,12 +838,12 @@ ri_restrict(TriggerData *trigdata, bool is_no_action)
/* Intersect the fk with the old pk range */
initStringInfo(&intersectbuf);
- appendStringInfoString(&intersectbuf, "(");
+ appendStringInfoChar(&intersectbuf, '(');
ri_GenerateQual(&intersectbuf, "",
attname, fk_period_type,
riinfo->period_intersect_oper,
paramname, pk_period_type);
- appendStringInfoString(&intersectbuf, ")");
+ appendStringInfoChar(&intersectbuf, ')');
/* Find the remaining history */
initStringInfo(&replacementsbuf);
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 06c28ab3149..a22b3124fb9 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -1310,7 +1310,7 @@ process_global_sql_commands(PGconn *conn, const char *dumpdirpath, const char *o
appendStringInfoString(&user_create, "CREATE ROLE ");
/* should use fmtId here, but we don't know the encoding */
appendStringInfoString(&user_create, PQuser(conn));
- appendStringInfoString(&user_create, ";");
+ appendStringInfoChar(&user_create, ';');
/* Process file till EOF and execute sql statements. */
while (read_one_statement(&sqlstatement, pfile) != EOF)
--
2.43.0
On 10/04/2025 06:51, David Rowley wrote:
Looks like v18 has grown a few appendStringInfo misusages, e.g. using
appendStringInfo() when no formatting is needed or just using format
"%s" instead of using appendStringInfoString().I've attached a couple of patches. The 0001 is just my method for
finding these, not for commit.
Clever!
0002 contains the fixes to commit.
Any objections to doing this soonish? Or in a few weeks?
Sure, let's do it. Why would we wait?
--
Heikki Linnakangas
Neon (https://neon.tech)
On Thu, Apr 10, 2025 at 11:24:36AM +0300, Heikki Linnakangas wrote:
On 10/04/2025 06:51, David Rowley wrote:
Any objections to doing this soonish? Or in a few weeks?
Sure, let's do it. Why would we wait?
+1. You did something similar for v17 (commit 8461424), and it seems like
an entirely reasonable post-feature-freeze routine to me.
This probably isn't v18 material, but this reminds me of my idea to change
appendStringInfoString() into a macro for appendBinaryStringInfo() so that
the compiler can remove the runtime strlen() calls for string literals [0]/messages/by-id/20231218164135.GA530790@nathanxps13.
In most cases, the benefits are probably negligible, but StringInfo is
sometimes used in hot paths.
[0]: /messages/by-id/20231218164135.GA530790@nathanxps13
--
nathan
On Thu, 10 Apr 2025 at 20:24, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 10/04/2025 06:51, David Rowley wrote:
Any objections to doing this soonish? Or in a few weeks?
Sure, let's do it. Why would we wait?
Great. Pushed. Was considering waiting as I didn't know if there was
a revert-fest looming or not, and this may have conflicted a bit. In
hindsight, likely not a good thing to optimise for.
David
On Fri, 11 Apr 2025 at 02:51, Nathan Bossart <nathandbossart@gmail.com> wrote:
This probably isn't v18 material, but this reminds me of my idea to change
appendStringInfoString() into a macro for appendBinaryStringInfo() so that
the compiler can remove the runtime strlen() calls for string literals [0].
In most cases, the benefits are probably negligible, but StringInfo is
sometimes used in hot paths.
That one has come up a few times. The most lengthy discussion I
remember was in [1]/messages/by-id/flat/a0086cfc-ff0f-2827-20fe-52b591d2666c@enterprisedb.com. It didn't come to anything, but I don't think
there were any objections to it, so maybe we should just do it.
In the thread I did some measurements of binary size increases. For
non-compile-time consts, it does mean putting the strlen() call in the
calling function, which is a bit of overhead in terms of size. The
macro trick I suggested should have fixed that, but I admit the macro
is a bit ugly. The macro version also still has the overhead of having
to pass the length of the string when it detects a compile-time const.
David
[1]: /messages/by-id/flat/a0086cfc-ff0f-2827-20fe-52b591d2666c@enterprisedb.com
On 10.04.25 05:51, David Rowley wrote:
Looks like v18 has grown a few appendStringInfo misusages, e.g. using
appendStringInfo() when no formatting is needed or just using format
"%s" instead of using appendStringInfoString().
Would it be useful to augment appendStringInfo() something like this:
if (VA_ARGS_NARGS() == 0)
return appendStringInfoString(str, fmt);
?
Peter Eisentraut <peter@eisentraut.org> writes:
Would it be useful to augment appendStringInfo() something like this:
if (VA_ARGS_NARGS() == 0)
return appendStringInfoString(str, fmt);
That would change the behavior in edge cases, for instance
appendStringInfo(str, "foo%%bar"). Maybe we'd never hit those,
but on the whole I'm not in love with the idea.
regards, tom lane
On Fri, Apr 11, 2025 at 10:40:57AM +1200, David Rowley wrote:
On Fri, 11 Apr 2025 at 02:51, Nathan Bossart <nathandbossart@gmail.com> wrote:
This probably isn't v18 material, but this reminds me of my idea to change
appendStringInfoString() into a macro for appendBinaryStringInfo() so that
the compiler can remove the runtime strlen() calls for string literals [0].
In most cases, the benefits are probably negligible, but StringInfo is
sometimes used in hot paths.That one has come up a few times. The most lengthy discussion I
remember was in [1]. It didn't come to anything, but I don't think
there were any objections to it, so maybe we should just do it.In the thread I did some measurements of binary size increases. For
non-compile-time consts, it does mean putting the strlen() call in the
calling function, which is a bit of overhead in terms of size. The
macro trick I suggested should have fixed that, but I admit the macro
is a bit ugly. The macro version also still has the overhead of having
to pass the length of the string when it detects a compile-time const.
Thanks for the additional context.
--
nathan