Date: Tue, 10 Dec 2019 15:59:13 -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: <20191210185913.GA28890@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="9amGYk9869ThD9tj"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20191210144847.GA6183@alvherre.pgsql>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


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

On 2019-Dec-10, Alvaro Herrera wrote:

> On 2019-Dec-09, Tom Lane wrote:
> 
> > Some quick review of v19:
> 
> Here's v20 with all these comments hopefully addressed.

Grr, I had forgotten to git add the stringinfo.h -> pg_wchar.h changes,
so the prototype isn't anywhere in v20.  However, after looking at it
again, I'm not very happy with how that turned out, because pg_wchar.h
is a frontend-exposed header.  So I instead propose to put it in a
separate file src/include/mb/stringinfo_mb.h.

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

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

From bd8df7f46556eb8a5de5e64f04b29f4d7a174d0e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 10 Dec 2019 14:40:56 -0300
Subject: [PATCH v21] Add backend-only appendStringInfoStringQuoted
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This provides a mechanism to emit literal values in informative
messages, such as query parameters.  The new code is more complex than
what it replaces, primarily because it wants to be more efficient.
It also has the (currently unused) additional optional capability of
specifying a maximum size to print.

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           | 11 +---
 src/backend/utils/mb/Makefile         |  1 +
 src/backend/utils/mb/README           |  1 +
 src/backend/utils/mb/stringinfo_mb.c  | 92 +++++++++++++++++++++++++++
 src/include/mb/stringinfo_mb.h        | 24 +++++++
 src/pl/plpgsql/src/pl_exec.c          | 37 ++++-------
 src/test/regress/expected/plpgsql.out | 14 ++++
 src/test/regress/sql/plpgsql.sql      | 13 ++++
 8 files changed, 158 insertions(+), 35 deletions(-)
 create mode 100644 src/backend/utils/mb/stringinfo_mb.c
 create mode 100644 src/include/mb/stringinfo_mb.h

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3b85e48333..512209a38c 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -48,6 +48,7 @@
 #include "libpq/pqformat.h"
 #include "libpq/pqsignal.h"
 #include "mb/pg_wchar.h"
+#include "mb/stringinfo_mb.h"
 #include "miscadmin.h"
 #include "nodes/print.h"
 #include "optimizer/optimizer.h"
@@ -2348,7 +2349,6 @@ errdetail_params(ParamListInfo params)
 			Oid			typoutput;
 			bool		typisvarlena;
 			char	   *pstring;
-			char	   *p;
 
 			appendStringInfo(&param_str, "%s$%d = ",
 							 paramno > 0 ? ", " : "",
@@ -2364,14 +2364,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..e84b9ad36f
--- /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 "mb/stringinfo_mb.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/include/mb/stringinfo_mb.h b/src/include/mb/stringinfo_mb.h
new file mode 100644
index 0000000000..e90ba8bc51
--- /dev/null
+++ b/src/include/mb/stringinfo_mb.h
@@ -0,0 +1,24 @@
+/*-------------------------------------------------------------------------
+ *
+ * stringinfo_mb.h
+ *	  multibyte support for StringInfo
+ *
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/mb/stringinfo_mb.h
+ *-------------------------------------------------------------------------
+ */
+#ifndef STRINGINFO_MB_H
+#define STRINGINFO_MB_H
+
+
+#include "lib/stringinfo.h"
+
+/*
+ * Multibyte-aware StringInfo support function.
+ */
+extern void appendStringInfoStringQuoted(StringInfo str,
+										 const char *s, int maxlen);
+
+#endif							/* STRINGINFO_MB_H */
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 4f0de7a811..2deb7c0b12 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -28,6 +28,7 @@
 #include "executor/spi.h"
 #include "executor/spi_priv.h"
 #include "funcapi.h"
+#include "mb/stringinfo_mb.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -8611,19 +8612,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 +8654,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


--9amGYk9869ThD9tj--