On 2019-Sep-20, Andres Freund wrote:
> > > > + appendStringInfoCharMacro(¶m_str, '\'');
> > > > + for (p = pstring; *p; p++)
> > > > + {
> > > > + if (*p == '\'') /* double single quotes */
> > > > + appendStringInfoCharMacro(¶m_str,
> > > > *p);
> > > > + appendStringInfoCharMacro(¶m_str, *p);
> > > > + }
> > > > + appendStringInfoCharMacro(¶m_str, '\'');
> > >
> > > I know this is old code, but: This is really inefficient. Will cause a
> > > lot of unnecessary memory-reallocations for large text outputs, because
> > > we don't immediately grow to at least close to the required size.
> I'd probably just count the ' in one pass, enlarge the stringinfo to the
> required size, and then put the string directly into he stringbuffer.
> Possibly just putting the necessary code into stringinfo.c. We already
> have multiple copies of this inefficient logic...
I stole Alexey's code for this from downthread and tried to apply to all
these places. However, I did not put it to use in all these places you
mention, because each of them has slightly different requirements;
details below.
Now, I have to say that this doesn't make me terribly happy, because I
would like the additional ability to limit the printed values to N
bytes. This means the new function would have to have an additional
argument to indicate the maximum length (pass 0 to print all args
whole) ... and the logic then because more involved because we need
logic to stop printing early.
I think the current callers should all use the early termination logic;
having plpgsql potentially produce 1 MB of log output because of a long
argument seems silly. So I see no use for this function *without* the
length-limiting logic.
> But even if not, I don't think writing data into the stringbuf directly
> is that ugly, we do that in enough places that I'd argue that that's
> basically part of how it's expected to be used.
>
> In HEAD there's at least
> - postgres.c:errdetail_params(),
> - pl_exec.c:format_preparedparamsdata()
> pl_exec.c:format_expr_params()
These are changed in the patch; they're all alike.
> - dblink.c:escape_param_str()
This one wants to use \' and \\ instead of just doubling each '.
The resulting string is used as libpq conninfo, so using doubled ' does
not work.
> - deparse.c:deparseStringLiteral()
This one wants to use E''-style strings that ignore std_conforming_strings;
the logic would need to detect two chars ' and \ instead of just ' so
we'd need to use strcspn instead of strchr(); that seems a lot slower.
> - xlog.c:do_pg_start_backup() (after "Add the escape character"),
This one wants to escape \n chars.
> - tsearchcmds.c:serialize_deflist()
This wants E''-style strings, like deparse.c.
> - ruleutils.c:simple_quote_literal()
Produce an escaped string according to the prevailing
std_conforming_strings.
I think it's possible to produce a function that would satisfy all of
these, but it would be another function similar to the one I'm writing
here, not the same one.
--
Álvaro Herrera https://www.2ndQuadrant.com/
ggPostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 99cf1d122bc935a4060c6b359fb2c262035039df Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 3 Dec 2019 10:08:35 -0300
Subject: [PATCH] Add appendStringInfoStringQuoted
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Simplifies some coding that prints parameters, as well as optimize to do
it per non-quote chunks instead of per byte.
Author: Alexey Bashtanov and Álvaro Herrera, after a suggestion from Andres Freund
Discussion: https://postgr.es/m/20190920203905.xkv5udsd5dxfs...@alap3.anarazel.de
---
src/backend/tcop/postgres.c | 10 +--------
src/common/stringinfo.c | 40 ++++++++++++++++++++++++++++++++++++
src/include/lib/stringinfo.h | 7 +++++++
src/pl/plpgsql/src/pl_exec.c | 34 ++++++++----------------------
4 files changed, 56 insertions(+), 35 deletions(-)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3b85e48333..0bff20ad67 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2348,7 +2348,6 @@ errdetail_params(ParamListInfo params)
Oid typoutput;
bool typisvarlena;
char *pstring;
- char *p;
appendStringInfo(¶m_str, "%s$%d = ",
paramno > 0 ? ", " : "",
@@ -2364,14 +2363,7 @@ errdetail_params(ParamListInfo params)
pstring = OidOutputFunctionCall(typoutput, prm->value);
- appendStringInfoCharMacro(¶m_str, '\'');
- for (p = pstring; *p; p++)
- {
- if (*p == '\'') /* double single quotes */
- appendStringInfoCharMacro(¶m_str, *p);
- appendStringInfoCharMacro(¶m_str, *p);
- }
- appendStringInfoCharMacro(¶m_str, '\'');
+ appendStringInfoStringQuoted(¶m_str, pstring);
pfree(pstring);
}
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index a50e587da9..f4dddf8223 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -178,6 +178,46 @@ appendStringInfoString(StringInfo str, const char *s)
appendBinaryStringInfo(str, s, strlen(s));
}
+/*
+ * appendStringInfoStringQuoted
+ *
+ * Append a null-terminated string to str, adding single quotes around it
+ * and doubling all single quotes.
+ */
+void
+appendStringInfoStringQuoted(StringInfo str, const char *s)
+{
+ const char *chunk_search_start = s,
+ *chunk_copy_start = s,
+ *chunk_end;
+ int len;
+
+ Assert(str != NULL);
+
+ appendStringInfoCharMacro(str, '\'');
+
+ while ((chunk_end = strchr(chunk_search_start, '\'')) != NULL)
+ {
+ /* copy including the found delimiting ' */
+ appendBinaryStringInfoNT(str,
+ chunk_copy_start,
+ chunk_end - chunk_copy_start + 1);
+
+ /* in order to double it, include this ' into the next chunk as well */
+ chunk_copy_start = chunk_end;
+ chunk_search_start = chunk_end + 1;
+ }
+
+ /* copy the last chunk, ensuring sufficient space for final bytes */
+ len = strlen(chunk_copy_start);
+ enlargeStringInfo(str, len + 2);
+ appendBinaryStringInfoNT(str, chunk_copy_start, len);
+
+ /* add terminators */
+ appendStringInfoCharMacro(str, '\'');
+ str->data[str->len] = '\0';
+}
+
/*
* appendStringInfoChar
*
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index e27942728e..7de2773e1f 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -113,6 +113,13 @@ extern int appendStringInfoVA(StringInfo str, const char *fmt, va_list args) pg_
*/
extern void appendStringInfoString(StringInfo str, const char *s);
+/*------------------------
+ * appendStringInfoStringQuoted
+ * Append a null-terminated string to str, adding single quotes around it
+ * and doubling all single quotes.
+ */
+extern void appendStringInfoStringQuoted(StringInfo str, const char *s);
+
/*------------------------
* appendStringInfoChar
* Append a single byte to str.
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 4f0de7a811..23553a6948 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -8611,19 +8611,10 @@ format_expr_params(PLpgSQL_execstate *estate,
if (paramisnull)
appendStringInfoString(¶mstr, "NULL");
else
- {
- char *value = convert_value_to_string(estate, paramdatum, paramtypeid);
- char *p;
-
- appendStringInfoCharMacro(¶mstr, '\'');
- for (p = value; *p; p++)
- {
- if (*p == '\'') /* double single quotes */
- appendStringInfoCharMacro(¶mstr, *p);
- appendStringInfoCharMacro(¶mstr, *p);
- }
- appendStringInfoCharMacro(¶mstr, '\'');
- }
+ appendStringInfoStringQuoted(¶mstr,
+ convert_value_to_string(estate,
+ paramdatum,
+ paramtypeid));
paramno++;
}
@@ -8661,19 +8652,10 @@ format_preparedparamsdata(PLpgSQL_execstate *estate,
if (ppd->nulls[paramno] == 'n')
appendStringInfoString(¶mstr, "NULL");
else
- {
- char *value = convert_value_to_string(estate, ppd->values[paramno], ppd->types[paramno]);
- char *p;
-
- appendStringInfoCharMacro(¶mstr, '\'');
- for (p = value; *p; p++)
- {
- if (*p == '\'') /* double single quotes */
- appendStringInfoCharMacro(¶mstr, *p);
- appendStringInfoCharMacro(¶mstr, *p);
- }
- appendStringInfoCharMacro(¶mstr, '\'');
- }
+ appendStringInfoStringQuoted(¶mstr,
+ convert_value_to_string(estate,
+ ppd->values[paramno],
+ ppd->types[paramno]));
}
MemoryContextSwitchTo(oldcontext);
--
2.20.1
