From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Alexey Bashtanov <bashtanov@imap.cc>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: log bind parameter values on error
Date: Tue, 3 Dec 2019 12:22:40 -0300
Message-ID: <20191203152240.GA21110@alvherre.pgsql> (raw)
In-Reply-To: <20190920203905.xkv5udsd5dxfs6tr@alap3.anarazel.de>
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
Attachments:
[text/x-diff] 0001-Add-appendStringInfoStringQuoted.patch (5.4K, ../20191203152240.GA21110@alvherre.pgsql/2-0001-Add-appendStringInfoStringQuoted.patch)
download | inline diff:
From 99cf1d122bc935a4060c6b359fb2c262035039df Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@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.xkv5udsd5dxfs6tr@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.cindex 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.cindex 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.hindex 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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, andres@anarazel.de, bashtanov@imap.cc, peter.eisentraut@2ndquadrant.com
Subject: Re: log bind parameter values on error
In-Reply-To: <20191203152240.GA21110@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox