Date: Tue, 10 Dec 2019 11:48:47 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>,
	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: <20191210144847.GA6183@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="liOOAslEiF7prFVr"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9900.1575925825@sss.pgh.pa.us>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


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

On 2019-Dec-09, Tom Lane wrote:

> Some quick review of v19:

Here's v20 with all these comments hopefully addressed.


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

From 62277fcd3f63ae68495300c98f77c7e4b4713614 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 v20] Add backend-only 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.

This lives separetely from common/stringinfo.c so that frontend users of
that file need not pull in unnecessary multibyte-encoding support code.

Author: Álvaro Herrera and Alexey Bashtanov, after a suggestion from Andres Freund
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/20190920203905.xkv5udsd5dxfs6tr@alap3.anarazel.de
---
 src/backend/tcop/postgres.c           | 10 +--
 src/backend/utils/mb/Makefile         |  1 +
 src/backend/utils/mb/README           |  1 +
 src/backend/utils/mb/stringinfo_mb.c  | 92 +++++++++++++++++++++++++++
 src/pl/plpgsql/src/pl_exec.c          | 36 +++--------
 src/test/regress/expected/plpgsql.out | 14 ++++
 src/test/regress/sql/plpgsql.sql      | 13 ++++
 7 files changed, 132 insertions(+), 35 deletions(-)
 create mode 100644 src/backend/utils/mb/stringinfo_mb.c

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3b85e48333..3d3172e83d 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, 0);
 
 			pfree(pstring);
 		}
diff --git a/src/backend/utils/mb/Makefile b/src/backend/utils/mb/Makefile
index 18dd758cfe..cd4a016449 100644
--- a/src/backend/utils/mb/Makefile
+++ b/src/backend/utils/mb/Makefile
@@ -16,6 +16,7 @@ OBJS = \
 	conv.o \
 	encnames.o \
 	mbutils.o \
+	stringinfo_mb.o \
 	wchar.o \
 	wstrcmp.o \
 	wstrncmp.o
diff --git a/src/backend/utils/mb/README b/src/backend/utils/mb/README
index c9bc6e6f8d..7495ca5db2 100644
--- a/src/backend/utils/mb/README
+++ b/src/backend/utils/mb/README
@@ -9,6 +9,7 @@ wchar.c:	mostly static functions and a public table for mb string and
 		multibyte conversion
 mbutils.c:	public functions for the backend only.
 		requires conv.c and wchar.c
+stringinfo_mb.c: public backend-only multibyte-aware stringinfo functions
 wstrcmp.c:	strcmp for mb
 wstrncmp.c:	strncmp for mb
 win866.c:	a tool to generate KOI8 <--> CP866 conversion table
diff --git a/src/backend/utils/mb/stringinfo_mb.c b/src/backend/utils/mb/stringinfo_mb.c
new file mode 100644
index 0000000000..baef1aa39a
--- /dev/null
+++ b/src/backend/utils/mb/stringinfo_mb.c
@@ -0,0 +1,92 @@
+/*-------------------------------------------------------------------------
+ *
+ * stringinfo_mb.c
+ *		Multibyte encoding-aware additional StringInfo facilites
+ *
+ * This is separate from common/stringinfo.c so that frontend users
+ * of that file need not pull in unnecessary multibyte-encoding support
+ * code.
+ *
+ *
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/backend/utils/mb/stringinfo_mb.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "lib/stringinfo.h"
+#include "mb/pg_wchar.h"
+
+
+/*
+ * appendStringInfoStringQuoted
+ *
+ * Append up to maxlen characters from s to str, or the whole input string if
+ * maxlen <= 0, adding single quotes around it and doubling all single quotes.
+ * Add an ellipsis if the copy is incomplete.
+ */
+void
+appendStringInfoStringQuoted(StringInfo str, const char *s, int maxlen)
+{
+	char	   *copy = NULL;
+	const char *chunk_search_start,
+			   *chunk_copy_start,
+			   *chunk_end;
+	bool		ellipsis;
+	int			slen;
+
+	Assert(str != NULL);
+
+	slen = strlen(s);
+	if (maxlen > 0 && maxlen < slen)
+	{
+		int		finallen = pg_mbcliplen(s, slen, maxlen);
+
+		copy = pnstrdup(s, finallen);
+		chunk_search_start = copy;
+		chunk_copy_start = copy;
+
+		ellipsis = true;
+	}
+	else
+	{
+		chunk_search_start = s;
+		chunk_copy_start = s;
+
+		ellipsis = false;
+	}
+
+	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 and terminate */
+	if (ellipsis)
+		appendStringInfo(str, "%s...'", chunk_copy_start);
+	else
+	{
+		int		len = strlen(chunk_copy_start);
+
+		/* ensure sufficient space for terminators */
+		appendBinaryStringInfoNT(str, chunk_copy_start, len);
+		appendStringInfoCharMacro(str, '\'');
+	}
+
+	if (copy)
+		pfree(copy);
+}
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 4f0de7a811..c3ae409f39 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -8611,19 +8611,11 @@ 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),
+										 0);
 
 		paramno++;
 	}
@@ -8661,19 +8653,11 @@ 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]),
+										 0);
 	}
 
 	MemoryContextSwitchTo(oldcontext);
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index e85b29455e..cd2c79f4d5 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -2656,6 +2656,20 @@ create or replace function stricttest() returns void as $$
 declare
 x record;
 p1 int := 2;
+p3 text := $a$'Valame Dios!' dijo Sancho; 'no le dije yo a vuestra merced que mirase bien lo que hacia?'$a$;
+begin
+  -- no rows
+  select * from foo where f1 = p1 and f1::text = p3 into strict x;
+  raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
+end$$ language plpgsql;
+select stricttest();
+ERROR:  query returned no rows
+DETAIL:  parameters: p1 = '2', p3 = '''Valame Dios!'' dijo Sancho; ''no le dije yo a vuestra merced que mirase bien lo que hacia?'''
+CONTEXT:  PL/pgSQL function stricttest() line 8 at SQL statement
+create or replace function stricttest() returns void as $$
+declare
+x record;
+p1 int := 2;
 p3 text := 'foo';
 begin
   -- too many rows
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 70deadfbea..d841d8c0f9 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2280,6 +2280,19 @@ end$$ language plpgsql;
 
 select stricttest();
 
+create or replace function stricttest() returns void as $$
+declare
+x record;
+p1 int := 2;
+p3 text := $a$'Valame Dios!' dijo Sancho; 'no le dije yo a vuestra merced que mirase bien lo que hacia?'$a$;
+begin
+  -- no rows
+  select * from foo where f1 = p1 and f1::text = p3 into strict x;
+  raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
+end$$ language plpgsql;
+
+select stricttest();
+
 create or replace function stricttest() returns void as $$
 declare
 x record;
-- 
2.20.1


--liOOAslEiF7prFVr--