Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Logging query parmeters in auto_explain
References: <87ee09mohb.fsf@wibble.ilmari.org> <Yp8D4kb2qZnhNAWJ@paquier.xyz>
	<8735ggn2lf.fsf@wibble.ilmari.org> <YqFQbYkFLF4fXuZX@paquier.xyz>
Date: Thu, 09 Jun 2022 23:55:11 +0100
In-Reply-To: <YqFQbYkFLF4fXuZX@paquier.xyz> (Michael Paquier's message of
	"Thu, 9 Jun 2022 10:44:13 +0900")
Message-ID: <87tu8tla5s.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: 
 <https://www.postgresql.org/message-id/87tu8tla5s.fsf%40wibble.ilmari.org>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

Michael Paquier <michael@paquier.xyz> writes:

> On Tue, Jun 07, 2022 at 12:18:52PM +0100, Dagfinn Ilmari Mannsåker wrote:
>> Point, fixed in the attached v2. I've also added a test that truncation
>> and disabling works.
>
> The tests are structured so as all the queries are run first, then the
> full set of logs is slurped and scanned.  With things like tests for
> truncation, it becomes easier to have a given test overlap with
> another one in terms of pattern matching, so we could silently lose
> coverage.  Wouldn't it be better to reorganize things so we save the
> current size of the log file (as of -s $node->logfile), run one query,
> and then slurp the log file based on the position saved previously?
> The GUC updates had better be localized in each sub-section of the
> tests, as well. 

Done (and more tests added), v3 attached.

- ilmari


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=v3-0001-Log-query-parameters-in-auto_explain.patch

From de3627d972f5259c9290109a6d7b08fbfa1faea2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 31 May 2022 21:12:12 +0100
Subject: [PATCH v3] Log query parameters in auto_explain

Add an auto_explain.log_parameter_max_length config setting, similar
to the corresponding core setting, that controls the inclusion of
query parameters in the logged explain output.

Also adjust the tests to only look at the relevant parts of the log
for each query, and tho reset the GUCs after each test.
---
 contrib/auto_explain/auto_explain.c        |  15 +++
 contrib/auto_explain/t/001_auto_explain.pl | 133 ++++++++++++++++++---
 doc/src/sgml/auto-explain.sgml             |  19 +++
 src/backend/commands/explain.c             |  22 ++++
 src/include/commands/explain.h             |   1 +
 5 files changed, 176 insertions(+), 14 deletions(-)

diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index c9a0d947c8..1ba7536879 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -19,12 +19,14 @@
 #include "common/pg_prng.h"
 #include "executor/instrument.h"
 #include "jit/jit.h"
+#include "nodes/params.h"
 #include "utils/guc.h"
 
 PG_MODULE_MAGIC;
 
 /* GUC variables */
 static int	auto_explain_log_min_duration = -1; /* msec or -1 */
+static int	auto_explain_log_parameter_max_length = -1; /* bytes or -1 */
 static bool auto_explain_log_analyze = false;
 static bool auto_explain_log_verbose = false;
 static bool auto_explain_log_buffers = false;
@@ -105,6 +107,18 @@ _PG_init(void)
 							NULL,
 							NULL);
 
+	DefineCustomIntVariable("auto_explain.log_parameter_max_length",
+							"Sets the maximum length of query parameters to log.",
+							"Zero logs no query parameters, -1 logs them in full.",
+							&auto_explain_log_parameter_max_length,
+							-1,
+							-1, INT_MAX,
+							PGC_SUSET,
+							GUC_UNIT_BYTE,
+							NULL,
+							NULL,
+							NULL);
+
 	DefineCustomBoolVariable("auto_explain.log_analyze",
 							 "Use EXPLAIN ANALYZE for plan logging.",
 							 NULL,
@@ -389,6 +403,7 @@ explain_ExecutorEnd(QueryDesc *queryDesc)
 
 			ExplainBeginOutput(es);
 			ExplainQueryText(es, queryDesc);
+			ExplainQueryParameters(es, queryDesc->params, auto_explain_log_parameter_max_length);
 			ExplainPrintPlan(es, queryDesc);
 			if (es->analyze && auto_explain_log_triggers)
 				ExplainPrintTriggers(es, queryDesc);
diff --git a/contrib/auto_explain/t/001_auto_explain.pl b/contrib/auto_explain/t/001_auto_explain.pl
index 82e4d9d15c..4fa1a8457b 100644
--- a/contrib/auto_explain/t/001_auto_explain.pl
+++ b/contrib/auto_explain/t/001_auto_explain.pl
@@ -16,42 +16,147 @@
 $node->append_conf('postgresql.conf', "auto_explain.log_analyze = on");
 $node->start;
 
+sub query_log
+{
+	my ($params, $sql) = @_;
+
+	if (keys %$params)
+	{
+		for my $key (keys %$params)
+		{
+			$node->append_conf('postgresql.conf', "$key = $params->{$key}\n");
+		}
+		$node->reload;
+	}
+
+	my $log    = $node->logfile();
+	my $offset = -s $log;
+
+	$node->safe_psql("postgres", $sql);
+
+	my $log_contents = slurp_file($log, $offset);
+
+	if (keys %$params)
+	{
+		for my $key (keys %$params)
+		{
+			$node->adjust_conf('postgresql.conf', $key, undef);
+		}
+		$node->reload;
+	}
+
+	return $log_contents;
+}
+
 # run a couple of queries
-$node->safe_psql("postgres", "SELECT * FROM pg_class;");
-$node->safe_psql("postgres",
-	"SELECT * FROM pg_proc WHERE proname = 'int4pl';");
+my $log_contents = query_log({}, "SELECT * FROM pg_class;");
 
-# emit some json too
-$node->append_conf('postgresql.conf', "auto_explain.log_format = json");
-$node->reload;
-$node->safe_psql("postgres", "SELECT * FROM pg_proc;");
-$node->safe_psql("postgres",
-	"SELECT * FROM pg_class WHERE relname = 'pg_class';");
+like(
+	$log_contents,
+	qr/Query Text: SELECT \* FROM pg_class;/,
+	"query text logged, text mode");
 
-$node->stop('fast');
-
-my $log = $node->logfile();
-
-my $log_contents = slurp_file($log);
+unlike(
+	$log_contents,
+	qr/Query Parameters:/,
+	"no query parameters logged when none, text mode");
 
 like(
 	$log_contents,
 	qr/Seq Scan on pg_class/,
 	"sequential scan logged, text mode");
 
+# with parameters
+$log_contents = query_log({},
+	q{PREPARE get_proc(name) AS SELECT * FROM pg_proc WHERE proname = $1; EXECUTE get_proc('int4pl');}
+);
+
+like(
+	$log_contents,
+	qr/Query Text: PREPARE get_proc\(name\) AS SELECT \* FROM pg_proc WHERE proname = \$1;/,
+	"prepared query text logged, text mode");
+
+like(
+	$log_contents,
+	qr/Query Parameters: \$1 = 'int4pl'/,
+	"query parameters logged, text mode");
+
 like(
 	$log_contents,
 	qr/Index Scan using pg_proc_proname_args_nsp_index on pg_proc/,
 	"index scan logged, text mode");
 
+# check that logged parameters can be truncated
+$log_contents = query_log(
+	{ "auto_explain.log_parameter_max_length" => 3 },
+	q{PREPARE get_type(name) AS SELECT * FROM pg_type WHERE typname = $1; EXECUTE get_type('float8');},
+);
+
+like(
+	$log_contents,
+	qr/Query Text: PREPARE get_type\(name\) AS SELECT \* FROM pg_type WHERE typname = \$1;/,
+	"prepared query text logged, text mode");
+
+like(
+	$log_contents,
+	qr/Query Parameters: \$1 = 'flo\.\.\.'/,
+	"query parameters truncated, text mode");
+
+# check that parameter logging can be disabled
+$log_contents = query_log(
+	{ "auto_explain.log_parameter_max_length" => 0 },
+	q{PREPARE get_type(name) AS SELECT * FROM pg_type WHERE typname = $1; EXECUTE get_type('float8');},
+);
+
+like(
+	$log_contents,
+	qr/Query Text: PREPARE get_type\(name\) AS SELECT \* FROM pg_type WHERE typname = \$1;/,
+	"prepared query text logged, text mode");
+
+unlike(
+	$log_contents,
+	qr/Query Parameters:/,
+	"query parameters not logged when disabled, text mode");
+
+# emit some json too
+$log_contents = query_log({ "auto_explain.log_format" => "json" },
+	"SELECT * FROM pg_proc;");
+
+like(
+	$log_contents,
+	qr/"Query Text": "SELECT \* FROM pg_proc;"/,
+	"query text logged, json mode");
+
+unlike(
+	$log_contents,
+	qr/"Query Parameters":/,
+	"query parameters not logged when none, json mode");
+
 like(
 	$log_contents,
 	qr/"Node Type": "Seq Scan"[^}]*"Relation Name": "pg_proc"/s,
 	"sequential scan logged, json mode");
 
+$log_contents = query_log(
+	{ "auto_explain.log_format" => "json" },
+	q{PREPARE get_class(name) AS SELECT * FROM pg_class WHERE relname = $1; EXECUTE get_class('pg_class');},
+);
+
+like(
+	$log_contents,
+	qr/"Query Text": "PREPARE get_class\(name\) AS SELECT \* FROM pg_class WHERE relname = \$1;"/,
+	"prepared query text logged, json mode");
+
+like(
+	$log_contents,
+	qr/"Query Parameters": "\$1 = 'pg_class'"/,
+	"query parameters logged, json mode");
+
 like(
 	$log_contents,
 	qr/"Node Type": "Index Scan"[^}]*"Index Name": "pg_class_relname_nsp_index"/s,
 	"index scan logged, json mode");
 
+$node->stop('fast');
+
 done_testing();
diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml
index 30e35a714a..a033c9c685 100644
--- a/doc/src/sgml/auto-explain.sgml
+++ b/doc/src/sgml/auto-explain.sgml
@@ -63,6 +63,25 @@
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term>
+     <varname>auto_explain.log_parameter_max_length</varname> (<type>integer</type>)
+     <indexterm>
+      <primary><varname>auto_explain.log_parameter_max_length</varname> configuration parameter</primary>
+     </indexterm>
+    </term>
+    <listitem>
+     <para>
+      <varname>auto_explain.log_parameter_max_length</varname> controls the
+      logging of query parameter values.  A value of<literal>-1</literal> (the
+      default) logs the parameter values in full. <literal>0</literal> disables
+      logging of parameter values.  A value greater than zero truncates each
+      parameter value to that many bytes. Only superusers can change this
+      setting.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term>
      <varname>auto_explain.log_analyze</varname> (<type>boolean</type>)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 5d1f7089da..8584e3585d 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -972,6 +972,28 @@ ExplainQueryText(ExplainState *es, QueryDesc *queryDesc)
 		ExplainPropertyText("Query Text", queryDesc->sourceText, es);
 }
 
+/*
+ * ExplainQueryParameters -
+ *	  add a "Query Parameters" node that describes the parameters of the query,
+ *	  if any
+ *
+ * The caller should have set up the options fields of *es, as well as
+ * initializing the output buffer es->str.
+ *
+ */
+void
+ExplainQueryParameters(ExplainState *es, ParamListInfo params, int maxlen)
+{
+	char	   *str;
+
+	if (params == NULL || params->numParams == 0 || maxlen == 0)
+		return;
+
+	str = BuildParamLogString(params, NULL, maxlen);
+	if (str && str[0] != '\0')
+		ExplainPropertyText("Query Parameters", str, es);
+}
+
 /*
  * report_triggers -
  *		report execution stats for a single relation's triggers
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 666977fb1f..9ebde089ae 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -99,6 +99,7 @@ extern void ExplainPrintTriggers(ExplainState *es, QueryDesc *queryDesc);
 extern void ExplainPrintJITSummary(ExplainState *es, QueryDesc *queryDesc);
 
 extern void ExplainQueryText(ExplainState *es, QueryDesc *queryDesc);
+extern void ExplainQueryParameters(ExplainState *es, ParamListInfo params, int maxlen);
 
 extern void ExplainBeginOutput(ExplainState *es);
 extern void ExplainEndOutput(ExplainState *es);
-- 
2.30.2


--=-=-=--