Simplify the way of appending comma to stringInfo
Hi Hackers,
In a lot places, there are logic of appending comma separators in a pattern
like:
```
for (int i = 0; i < len; i ++)
{
if (i > 0)
appendStringInfoString(", ");
appendStringInfo(some-item);
}
```
This pattern uses an "if" check and two appendStringInfoString() to build a
comma-delimited string.
This can be simplified as:
```
const char *sep = "";
for (int i = 0; i < len; i ++)
{
appendStringInfo("%s%s", sep, some-item);
sep = ", ";
}
```
The new pattern avoids the "if" check, and combines two
appendStringInfoString() into a single appendStringInfo(). I think the new
pattern is neater and faster.
The old patterns are used in a lot of places, and there are some usages of
the new pattern as well. Instead of creating a big cleanup patch, I
just applied the new pattern to a single file for now to see if the hacker
group likes this change.
Best regards,
==
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v1-0001-Simplify-EXPLAIN-for-how-to-append-comma-separato.patchapplication/octet-stream; name=v1-0001-Simplify-EXPLAIN-for-how-to-append-comma-separato.patchDownload
From b5e0947808d0c858ee324e1d1f5fa96775e0903b Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <lic@highgo.com>
Date: Mon, 8 Dec 2025 16:20:40 +0800
Subject: [PATCH v1] Simplify EXPLAIN for how to append comma separators
Streamline how we build comma-separated lists in EXPLAIN text
output by prefixing the separator instead of appending it after
each item. This is a cosmetic simplification; behavior is unchanged.
Author: Chao Li <lic@highgo.com>
---
src/backend/commands/explain.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 7e699f8595e..55b60afaf98 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -718,6 +718,7 @@ ExplainPrintSettings(ExplainState *es)
else
{
StringInfoData str;
+ const char *sep = "";
/* In TEXT mode, print nothing if there are no options */
if (num <= 0)
@@ -730,15 +731,13 @@ ExplainPrintSettings(ExplainState *es)
char *setting;
struct config_generic *conf = gucs[i];
- if (i > 0)
- appendStringInfoString(&str, ", ");
-
setting = GetConfigOptionByName(conf->name, NULL, true);
if (setting)
- appendStringInfo(&str, "%s = '%s'", conf->name, setting);
+ appendStringInfo(&str, "%s%s = '%s'", sep, conf->name, setting);
else
- appendStringInfo(&str, "%s = NULL", conf->name);
+ appendStringInfo(&str, "%s%s = NULL", sep, conf->name);
+ sep = ", ";
}
ExplainPropertyText("Settings", str.data, es);
@@ -2961,6 +2960,7 @@ show_window_keys(StringInfo buf, PlanState *planstate,
Plan *plan = planstate->plan;
List *context;
bool useprefix;
+ const char *sep = "";
/* Set up deparsing context */
context = set_deparse_context_plan(es->deparse_cxt,
@@ -2981,9 +2981,8 @@ show_window_keys(StringInfo buf, PlanState *planstate,
/* Deparse the expression, showing any top-level cast */
exprstr = deparse_expression((Node *) target->expr, context,
useprefix, true);
- if (keyno > 0)
- appendStringInfoString(buf, ", ");
- appendStringInfoString(buf, exprstr);
+ appendStringInfo(buf, "%s%s", sep, exprstr);
+ sep = ", ";
pfree(exprstr);
/*
@@ -3058,16 +3057,14 @@ show_tablesample(TableSampleClause *tsc, PlanState *planstate,
/* Print results */
if (es->format == EXPLAIN_FORMAT_TEXT)
{
- bool first = true;
+ const char *sep = "";
ExplainIndentText(es);
appendStringInfo(es->str, "Sampling: %s (", method_name);
foreach(lc, params)
{
- if (!first)
- appendStringInfoString(es->str, ", ");
- appendStringInfoString(es->str, (const char *) lfirst(lc));
- first = false;
+ appendStringInfo(es->str, "%s%s", sep, (const char *) lfirst(lc));
+ sep = ", ";
}
appendStringInfoChar(es->str, ')');
if (repeatable)
@@ -4769,6 +4766,7 @@ static void
show_result_replacement_info(Result *result, ExplainState *es)
{
StringInfoData buf;
+ const char *sep = "";
int nrels = 0;
int rti = -1;
bool found_non_result = false;
@@ -4828,9 +4826,8 @@ show_result_replacement_info(Result *result, ExplainState *es)
refname = (char *) list_nth(es->rtable_names, rti - 1);
if (refname == NULL)
refname = rte->eref->aliasname;
- if (buf.len > 0)
- appendStringInfoString(&buf, ", ");
- appendStringInfoString(&buf, refname);
+ appendStringInfo(&buf, "%s%s", sep, refname);
+ sep = ", ";
/* Keep track of whether we see anything other than RTE_RESULT. */
if (rte->rtekind != RTE_RESULT)
--
2.39.5 (Apple Git-154)
Hi
po 8. 12. 2025 v 9:37 odesílatel Chao Li <li.evan.chao@gmail.com> napsal:
Hi Hackers,
In a lot places, there are logic of appending comma separators in a
pattern like:```
for (int i = 0; i < len; i ++)
{
if (i > 0)
appendStringInfoString(", ");
appendStringInfo(some-item);
}```
This pattern uses an "if" check and two appendStringInfoString() to build
a comma-delimited string.This can be simplified as:
```
const char *sep = "";
for (int i = 0; i < len; i ++)
{
appendStringInfo("%s%s", sep, some-item);
sep = ", ";
}
```
The new pattern avoids the "if" check, and combines two
appendStringInfoString() into a single appendStringInfo(). I think the new
pattern is neater and faster.The old patterns are used in a lot of places, and there are some usages of
the new pattern as well. Instead of creating a big cleanup patch, I
just applied the new pattern to a single file for now to see if the hacker
group likes this change.
It doesn't look like a simplification.
Regards
Pavel
Show quoted text
Best regards,
==
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/
On 08/12/2025 10:37, Chao Li wrote:
Hi Hackers,
In a lot places, there are logic of appending comma separators in a
pattern like:```
for (int i = 0; i < len; i ++)
{
if (i > 0)
appendStringInfoString(", ");
appendStringInfo(some-item);
}```
This pattern uses an "if" check and two appendStringInfoString() to
build a comma-delimited string.This can be simplified as:
```
const char *sep = "";
for (int i = 0; i < len; i ++)
{
appendStringInfo("%s%s", sep, some-item);
sep = ", ";
}
```
The new pattern avoids the "if" check, and combines two
appendStringInfoString() into a single appendStringInfo(). I think the
new pattern is neater and faster.The old patterns are used in a lot of places, and there are some usages
of the new pattern as well. Instead of creating a big cleanup patch, I
just applied the new pattern to a single file for now to see if the
hacker group likes this change.
It's a matter of taste, but I personally prefer the "old" pattern with
an explicit if() statement more. And I don't think it's worth the code
churn to change existing code either way.
- Heikki
On 08.12.2025 09:43, Heikki Linnakangas wrote:
On 08/12/2025 10:37, Chao Li wrote:
Hi Hackers,
In a lot places, there are logic of appending comma separators in a
pattern like:```
for (int i = 0; i < len; i ++)
{
if (i > 0)
appendStringInfoString(", ");
appendStringInfo(some-item);
}```
This pattern uses an "if" check and two appendStringInfoString() to
build a comma-delimited string.This can be simplified as:
```
const char *sep = "";
for (int i = 0; i < len; i ++)
{
appendStringInfo("%s%s", sep, some-item);
sep = ", ";
}
```
The new pattern avoids the "if" check, and combines two
appendStringInfoString() into a single appendStringInfo(). I think the
new pattern is neater and faster.The old patterns are used in a lot of places, and there are some
usages of the new pattern as well. Instead of creating a big cleanup
patch, I just applied the new pattern to a single file for now to see
if the hacker group likes this change.It's a matter of taste, but I personally prefer the "old" pattern with
an explicit if() statement more. And I don't think it's worth the code
churn to change existing code either way.- Heikki
It's also very likely not faster because now appendStringInfo() is
called one more time and appendStringInfo() also needs to check if the
string is empty or not, which requires an if in some form or shape (e.g.
a loop that bails immediately).
--
David Geier
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 08/12/2025 10:37, Chao Li wrote:
The old patterns are used in a lot of places, and there are some usages
of the new pattern as well. Instead of creating a big cleanup patch, I
just applied the new pattern to a single file for now to see if the
hacker group likes this change.
It's a matter of taste, but I personally prefer the "old" pattern with
an explicit if() statement more. And I don't think it's worth the code
churn to change existing code either way.
Yeah, the "code churn" objection is a big one in my mind. Any
cosmetic change like this creates merge hazards for back-patching
bug fixes in nearby code. Of course any one such change has only a
very low risk of causing back-patching pain, but if you are proposing
to do it in dozens or hundreds of places then the risk becomes much
greater. Cosmetic changes also make it more painful to identify the
origin of code segments via "git blame". I don't think this is such a
huge improvement as to justify those costs.
regards, tom lane