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
From ab6ffda29c9d1d258a8918190dc3b84cbe5efecf Mon Sep 17 00:00:00 2001 From: David Rowley <dgrow...@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
From d615eb695d426f72f94dc57c421bbc1aa10ade83 Mon Sep 17 00:00:00 2001 From: David Rowley <dgrow...@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
git blame contrib/postgres_fdw/deparse.c | grep "appendStringInfo(&str, \")\");" git blame contrib/postgres_fdw/deparse.c | grep "appendStringInfo(buf, \"%s\", join_sql_o.data);" git blame src/backend/replication/logical/slotsync.c | grep "appendStringInfo(&app_name, \"%s\", \"slotsync worker\");" git blame src/backend/replication/logical/tablesync.c | grep "appendStringInfoString(&cmd, \")\");" git blame src/backend/utils/adt/ri_triggers.c | grep "appendStringInfo(&querybuf, \") x1 HAVING \");" git blame src/backend/utils/adt/ri_triggers.c | grep "appendStringInfo(&querybuf, \"(x1.r)\");" git blame src/backend/utils/adt/ri_triggers.c | grep "appendStringInfo(&querybuf, \") x1 HAVING \");" git blame src/backend/utils/adt/ri_triggers.c | grep "appendStringInfo(&querybuf, \") x1 HAVING \");" git blame src/backend/utils/adt/ri_triggers.c | grep "appendStringInfo(&querybuf, \"(x1.r)\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfo(buf, \"MATCHED\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfo(buf, \"NOT MATCHED BY SOURCE\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfo(buf, \"NOT MATCHED BY TARGET\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfo(buf, \"NOT MATCHED\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfo(context->buf, \" WITH CONDITIONAL WRAPPER\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfo(context->buf, \" WITH UNCONDITIONAL WRAPPER\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfo(context->buf, \" WITHOUT WRAPPER\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfo(context->buf, \" OMIT QUOTES\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfo(context->buf, \" KEEP QUOTES\");" git blame src/backend/utils/adt/ruleutils.c | grep "appendStringInfoString(buf, \")\");"