Fixup some StringInfo usages
Started by David Rowleyalmost 2 years ago4 messages
Similar to f736e188c, I've attached a patch that fixes up a few
misusages of the StringInfo functions. These just swap one function
call for another function that is more suited to the use case.
I've also attached the patch that I used to find these. That's not
intended for commit.
I feel like it's a good idea to fix these soon while they're new
rather than wait N years and make backpatching bug fixes possibly
harder.
I've attached a file with git blame commands to identify where all of
these were introduced. All are new to PG17.
Any objections?
David
Attachments:
v1-0001-Fixup-various-StringInfo-function-usages.patchtext/plain; charset=US-ASCII; name=v1-0001-Fixup-various-StringInfo-function-usages.patchDownload
From ab6ffda29c9d1d258a8918190dc3b84cbe5efecf Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 5 Mar 2024 09:07:05 +1300
Subject: [PATCH v1] Fixup various StringInfo function usages
---
contrib/postgres_fdw/deparse.c | 4 ++--
src/backend/replication/logical/slotsync.c | 2 +-
src/backend/replication/logical/tablesync.c | 2 +-
src/backend/utils/adt/ri_triggers.c | 8 ++++----
src/backend/utils/adt/ruleutils.c | 20 ++++++++++----------
5 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index ce765ecc90..fb590c87e6 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -1887,7 +1887,7 @@ deparseFromExprForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
}
/* Close parentheses for EXISTS subquery */
- appendStringInfo(&str, ")");
+ appendStringInfoChar(&str, ')');
*additional_conds = lappend(*additional_conds, str.data);
}
@@ -1921,7 +1921,7 @@ deparseFromExprForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
*/
if (fpinfo->jointype == JOIN_SEMI)
{
- appendStringInfo(buf, "%s", join_sql_o.data);
+ appendBinaryStringInfo(buf, join_sql_o.data, join_sql_o.len);
}
else
{
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index d18e2c7342..97440cb6bf 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -1311,7 +1311,7 @@ ReplSlotSyncWorkerMain(char *startup_data, size_t startup_data_len)
if (cluster_name[0])
appendStringInfo(&app_name, "%s_%s", cluster_name, "slotsync worker");
else
- appendStringInfo(&app_name, "%s", "slotsync worker");
+ appendStringInfoString(&app_name, "slotsync worker");
/*
* Establish the connection to the primary server for slot
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 1061d5b61b..f1a3ad5459 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1154,7 +1154,7 @@ copy_table(Relation rel)
appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
}
- appendStringInfoString(&cmd, ")");
+ appendStringInfoChar(&cmd, ')');
}
appendStringInfoString(&cmd, " TO STDOUT");
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index a2cc837ebf..bbc2e3e2f0 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -426,13 +426,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 */
@@ -594,13 +594,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 */
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 52bf87ac55..24e3514b00 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -7176,16 +7176,16 @@ get_merge_query_def(Query *query, deparse_context *context,
switch (action->matchKind)
{
case MERGE_WHEN_MATCHED:
- appendStringInfo(buf, "MATCHED");
+ appendStringInfoString(buf, "MATCHED");
break;
case MERGE_WHEN_NOT_MATCHED_BY_SOURCE:
- appendStringInfo(buf, "NOT MATCHED BY SOURCE");
+ appendStringInfoString(buf, "NOT MATCHED BY SOURCE");
break;
case MERGE_WHEN_NOT_MATCHED_BY_TARGET:
if (haveNotMatchedBySource)
- appendStringInfo(buf, "NOT MATCHED BY TARGET");
+ appendStringInfoString(buf, "NOT MATCHED BY TARGET");
else
- appendStringInfo(buf, "NOT MATCHED");
+ appendStringInfoString(buf, "NOT MATCHED");
break;
default:
elog(ERROR, "unrecognized matchKind: %d",
@@ -8850,18 +8850,18 @@ get_json_expr_options(JsonExpr *jsexpr, deparse_context *context,
if (jsexpr->op == JSON_QUERY_OP)
{
if (jsexpr->wrapper == JSW_CONDITIONAL)
- appendStringInfo(context->buf, " WITH CONDITIONAL WRAPPER");
+ appendStringInfoString(context->buf, " WITH CONDITIONAL WRAPPER");
else if (jsexpr->wrapper == JSW_UNCONDITIONAL)
- appendStringInfo(context->buf, " WITH UNCONDITIONAL WRAPPER");
+ appendStringInfoString(context->buf, " WITH UNCONDITIONAL WRAPPER");
/* The default */
else if (jsexpr->wrapper == JSW_NONE || jsexpr->wrapper == JSW_UNSPEC)
- appendStringInfo(context->buf, " WITHOUT WRAPPER");
+ appendStringInfoString(context->buf, " WITHOUT WRAPPER");
if (jsexpr->omit_quotes)
- appendStringInfo(context->buf, " OMIT QUOTES");
+ appendStringInfoString(context->buf, " OMIT QUOTES");
/* The default */
else
- appendStringInfo(context->buf, " KEEP QUOTES");
+ appendStringInfoString(context->buf, " KEEP QUOTES");
}
if (jsexpr->on_empty && jsexpr->on_empty->btype != default_behavior)
@@ -10206,7 +10206,7 @@ get_rule_expr(Node *node, deparse_context *context,
JSON_BEHAVIOR_NULL :
JSON_BEHAVIOR_FALSE);
- appendStringInfoString(buf, ")");
+ appendStringInfoChar(buf, ')');
}
break;
--
2.40.1.windows.1
v1-0001-Adjust-code-to-highlight-appendStringInfo-misusag.patch.txttext/plain; charset=US-ASCII; name=v1-0001-Adjust-code-to-highlight-appendStringInfo-misusag.patch.txtDownload
From d615eb695d426f72f94dc57c421bbc1aa10ade83 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] 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 3337b77ae6..f65f08b0e2 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 3e4ca874d8..09c515501d 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -2135,7 +2135,7 @@ xml_errorHandler(void *data, PgXmlErrorPtr error)
void *errCtxSaved = xmlGenericErrorContext;
xmlSetGenericErrorFunc((void *) 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 ec5fc2422d..a6c2939a73 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -94,7 +94,7 @@ resetStringInfo(StringInfo str)
* strcat.
*/
void
-appendStringInfo(StringInfo str, const char *fmt,...)
+appendStringInfoInternal(StringInfo str, const char *fmt, ...)
{
int save_errno = errno;
@@ -173,13 +173,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 cd9632e3fc..882614416c 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -169,9 +169,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
@@ -187,7 +198,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.40.1.windows.1
Re: Fixup some StringInfo usages
On Tue, Apr 09, 2024 at 12:53:21PM +1200, David Rowley wrote:
Similar to f736e188c, I've attached a patch that fixes up a few
misusages of the StringInfo functions. These just swap one function
call for another function that is more suited to the use case.I've also attached the patch that I used to find these. That's not
intended for commit.I feel like it's a good idea to fix these soon while they're new
rather than wait N years and make backpatching bug fixes possibly
harder.I've attached a file with git blame commands to identify where all of
these were introduced. All are new to PG17.Any objections?
Looks reasonable to me.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: Fixup some StringInfo usages
David Rowley <dgrowleyml@gmail.com> writes:
Similar to f736e188c, I've attached a patch that fixes up a few
misusages of the StringInfo functions. These just swap one function
call for another function that is more suited to the use case.
I feel like it's a good idea to fix these soon while they're new
rather than wait N years and make backpatching bug fixes possibly
harder.
+1. Looks good in a quick eyeball scan.
regards, tom lane
Re: Fixup some StringInfo usages
On Tue, 9 Apr 2024 at 14:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
Similar to f736e188c, I've attached a patch that fixes up a few
misusages of the StringInfo functions. These just swap one function
call for another function that is more suited to the use case.I feel like it's a good idea to fix these soon while they're new
rather than wait N years and make backpatching bug fixes possibly
harder.+1. Looks good in a quick eyeball scan.
Thank you to both of you for having a look. Pushed.
David