Date: Tue, 3 Dec 2019 12:22:40 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Alexey Bashtanov <bashtanov@imap.cc>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: log bind parameter values on error
Message-ID: <20191203152240.GA21110@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="AqsLC8rIMeq19msA"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190920203905.xkv5udsd5dxfs6tr@alap3.anarazel.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


--AqsLC8rIMeq19msA
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2019-Sep-20, Andres Freund wrote:

> > > > +		appendStringInfoCharMacro(&param_str, '\'');
> > > > +		for (p = pstring; *p; p++)
> > > > +		{
> > > > +			if (*p == '\'') /* double single quotes */
> > > > +				appendStringInfoCharMacro(&param_str, *p);
> > > > +			appendStringInfoCharMacro(&param_str, *p);
> > > > +		}
> > > > +		appendStringInfoCharMacro(&param_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

--AqsLC8rIMeq19msA
Content-Type: text/x-diff; charset=iso-8859-1
Content-Disposition: attachment;
 filename="0001-Add-appendStringInfoStringQuoted.patch"
Content-Transfer-Encoding: 8bit

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.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(&param_str, "%s$%d = ",
 							 paramno > 0 ? ", " : "",
@@ -2364,14 +2363,7 @@ errdetail_params(ParamListInfo params)
 
 			pstring = OidOutputFunctionCall(typoutput, prm->value);
 
-			appendStringInfoCharMacro(&param_str, '\'');
-			for (p = pstring; *p; p++)
-			{
-				if (*p == '\'') /* double single quotes */
-					appendStringInfoCharMacro(&param_str, *p);
-				appendStringInfoCharMacro(&param_str, *p);
-			}
-			appendStringInfoCharMacro(&param_str, '\'');
+			appendStringInfoStringQuoted(&param_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(&paramstr, "NULL");
 		else
-		{
-			char	   *value = convert_value_to_string(estate, paramdatum, paramtypeid);
-			char	   *p;
-
-			appendStringInfoCharMacro(&paramstr, '\'');
-			for (p = value; *p; p++)
-			{
-				if (*p == '\'') /* double single quotes */
-					appendStringInfoCharMacro(&paramstr, *p);
-				appendStringInfoCharMacro(&paramstr, *p);
-			}
-			appendStringInfoCharMacro(&paramstr, '\'');
-		}
+			appendStringInfoStringQuoted(&paramstr,
+										 convert_value_to_string(estate,
+																 paramdatum,
+																 paramtypeid));
 
 		paramno++;
 	}
@@ -8661,19 +8652,10 @@ format_preparedparamsdata(PLpgSQL_execstate *estate,
 		if (ppd->nulls[paramno] == 'n')
 			appendStringInfoString(&paramstr, "NULL");
 		else
-		{
-			char	   *value = convert_value_to_string(estate, ppd->values[paramno], ppd->types[paramno]);
-			char	   *p;
-
-			appendStringInfoCharMacro(&paramstr, '\'');
-			for (p = value; *p; p++)
-			{
-				if (*p == '\'') /* double single quotes */
-					appendStringInfoCharMacro(&paramstr, *p);
-				appendStringInfoCharMacro(&paramstr, *p);
-			}
-			appendStringInfoCharMacro(&paramstr, '\'');
-		}
+			appendStringInfoStringQuoted(&paramstr,
+										 convert_value_to_string(estate,
+																 ppd->values[paramno],
+																 ppd->types[paramno]));
 	}
 
 	MemoryContextSwitchTo(oldcontext);
-- 
2.20.1


--AqsLC8rIMeq19msA--