Subject: Re: explain plans with information about (modified) gucs
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <e1791b4c-df9c-be02-edc5-7c8874944be0@2ndquadrant.com>
 <10561.1544800904@sss.pgh.pa.us>
 <c1282df1-43a8-0854-dfee-790d7dd9c93a@2ndquadrant.com>
Message-ID: <631ae5c8-5581-6c5e-382c-2dcdb2e5c956@2ndquadrant.com>
Date: Sat, 15 Dec 2018 16:44:48 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.3.1
MIME-Version: 1.0
In-Reply-To: <c1282df1-43a8-0854-dfee-790d7dd9c93a@2ndquadrant.com>
Content-Type: multipart/mixed;
 boundary="------------AEC5569485AC5EAFBC437091"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------AEC5569485AC5EAFBC437091
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


On 12/14/18 4:32 PM, Tomas Vondra wrote:
> 
> 
> On 12/14/18 4:21 PM, Tom Lane wrote:
>> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>>> ... I propose to extend EXPLAIN output with an additional option, which
>>> would include information about modified GUCs in the execution plan
>>> (disabled by default, of course):
>>
>> I'm a bit suspicious about whether this'll have any actual value,
>> if it's disabled by default (which I agree it needs to be, if only for
>> compatibility reasons).  The problem you're trying to solve is basically
>> "I forgot that this might have an effect", but stuff that isn't shown
>> by default will not help you un-forget.  It certainly won't fix the
>> form of the problem that I run into, which is people sending in EXPLAIN
>> plans and not mentioning their weird local settings.
>>
> 
> Not quite.
> 
> I agree we'll still have to deal with plans from users without this
> info, but it's easier to ask for explain with this extra option (just
> like we regularly ask for explain analyze instead of just plain
> explain). I'd expect the output to be more complete than trying to
> figure out which of the GUCs might have effect / been modified here.
> 
> But more importantly - my personal primary use case here is explains
> from application connections generated using auto_explain, with some
> application-level GUC magic. And there I can easily tweak auto_explain
> config to do (auto_explain.log_gucs = true) of course.
> 
>>> We certainly don't want to include all GUCs, so the question is how to
>>> decide which GUCs are interesting. The simplest approach would be to
>>> look for GUCs that changed in the session (source == PGC_S_SESSION), but
>>> that does not help with SET SESSION AUTHORIZATION / ROLE cases. So we
>>> probably want (source > PGC_S_ARGV), but probably not PGC_S_OVERRIDE
>>> because that includes irrelevant options like wal_buffers etc.
>>
>> Don't you want to show anything that's not the built-in default?
>> (I agree OVERRIDE could be excluded, but that's irrelevant for query
>> tuning parameters.)  Just because somebody injected a damfool setting
>> of, say, random_page_cost via the postmaster command line or
>> environment settings doesn't make it not damfool :-(
>>
> 
> Probably. My assumption here was that I can do
> 
>    select * from pg_settings
> 
> and then combine it with whatever is included in the plan. But you're
> right comparing it with the built-in default may be a better option.
> 

FWIW here is a v3 of the patch, using the built-in default, and fixing a
silly thinko resulting in the code not being executed from auto_explain.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--------------AEC5569485AC5EAFBC437091
Content-Type: text/x-patch;
 name="explain-with-gucs-v3.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="explain-with-gucs-v3.patch"

diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index 646cd0d42c..ec44c59b7f 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -28,6 +28,7 @@ static bool auto_explain_log_verbose = false;
 static bool auto_explain_log_buffers = false;
 static bool auto_explain_log_triggers = false;
 static bool auto_explain_log_timing = true;
+static bool auto_explain_log_gucs = false;
 static int	auto_explain_log_format = EXPLAIN_FORMAT_TEXT;
 static int	auto_explain_log_level = LOG;
 static bool auto_explain_log_nested_statements = false;
@@ -112,6 +113,17 @@ _PG_init(void)
 							 NULL,
 							 NULL);
 
+	DefineCustomBoolVariable("auto_explain.log_gucs",
+							 "Print modified GUC values.",
+							 NULL,
+							 &auto_explain_log_gucs,
+							 false,
+							 PGC_SUSET,
+							 0,
+							 NULL,
+							 NULL,
+							 NULL);
+
 	DefineCustomBoolVariable("auto_explain.log_verbose",
 							 "Use EXPLAIN VERBOSE for plan logging.",
 							 NULL,
@@ -356,6 +368,7 @@ explain_ExecutorEnd(QueryDesc *queryDesc)
 			es->timing = (es->analyze && auto_explain_log_timing);
 			es->summary = es->analyze;
 			es->format = auto_explain_log_format;
+			es->gucs = auto_explain_log_gucs;
 
 			ExplainBeginOutput(es);
 			ExplainQueryText(es, queryDesc);
diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml
index 120b168d45..852c69b7bb 100644
--- a/doc/src/sgml/auto-explain.sgml
+++ b/doc/src/sgml/auto-explain.sgml
@@ -169,6 +169,23 @@ LOAD 'auto_explain';
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term>
+     <varname>auto_explain.log_gucs</varname> (<type>boolean</type>)
+     <indexterm>
+      <primary><varname>auto_explain.log_gucs</varname> configuration parameter</primary>
+     </indexterm>
+    </term>
+    <listitem>
+     <para>
+      <varname>auto_explain.log_gucs</varname> controls whether information
+      about modified configuration options are logged with the execution
+      plan.  Only options modified at the database, user, client or session
+      level are considered modified.  This parameter is off by default.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term>
      <varname>auto_explain.log_format</varname> (<type>enum</type>)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index de09ded65b..2e92690453 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -31,6 +31,7 @@
 #include "storage/bufmgr.h"
 #include "tcop/tcopprot.h"
 #include "utils/builtins.h"
+#include "utils/guc_tables.h"
 #include "utils/json.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
@@ -164,6 +165,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
 			es->costs = defGetBoolean(opt);
 		else if (strcmp(opt->defname, "buffers") == 0)
 			es->buffers = defGetBoolean(opt);
+		else if (strcmp(opt->defname, "gucs") == 0)
+			es->gucs = defGetBoolean(opt);
 		else if (strcmp(opt->defname, "timing") == 0)
 		{
 			timing_set = true;
@@ -635,6 +638,41 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
 	if (IsA(ps, GatherState) &&((Gather *) ps->plan)->invisible)
 		ps = outerPlanState(ps);
 	ExplainNode(ps, NIL, NULL, NULL, es);
+
+	/*
+	 * If requested, include information about GUC parameters that don't
+	 * match the built-in defaults.
+	 */
+	if (es->gucs)
+	{
+		int		i;
+		int		num;
+		StringInfoData	str;
+		struct config_generic **gucs;
+
+		gucs = get_modified_guc_options(&num);
+
+		for (i = 0; i < num; i++)
+		{
+			char *setting;
+			struct config_generic *conf = gucs[i];
+
+			if (i == 0)
+				initStringInfo(&str);
+			else
+				appendStringInfoString(&str, ", ");
+
+			setting = GetConfigOptionByName(conf->name, NULL, true);
+
+			if (setting)
+				appendStringInfo(&str, "%s = '%s'", conf->name, setting);
+			else
+				appendStringInfo(&str, "%s = NULL", conf->name);
+		}
+
+		if (num > 0)
+			ExplainPropertyText("GUCs", str.data, es);
+	}
 }
 
 /*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6fe1939881..8223c041f7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8556,6 +8556,90 @@ ShowAllGUCConfig(DestReceiver *dest)
 	end_tup_output(tstate);
 }
 
+struct config_generic **
+get_modified_guc_options(int *num)
+{
+	int		i;
+	struct config_generic **result;
+
+	*num = 0;
+	result = palloc(sizeof(struct config_generic *) * num_guc_variables);
+
+	for (i = 0; i < num_guc_variables; i++)
+	{
+		bool modified;
+		struct config_generic *conf = guc_variables[i];
+
+		/* return only options visible to the user */
+		if ((conf->flags & GUC_NO_SHOW_ALL) ||
+			((conf->flags & GUC_SUPERUSER_ONLY) &&
+			 !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS)))
+			continue;
+
+		/* only parameters related to query tuning */
+		if ((conf->group != QUERY_TUNING) &&
+			(conf->group != QUERY_TUNING_METHOD) &&
+			(conf->group != QUERY_TUNING_COST) &&
+			(conf->group != QUERY_TUNING_GEQO) &&
+			(conf->group != QUERY_TUNING_OTHER))
+			continue;
+
+		/* return only options that were modified (w.r.t. config file) */
+		modified = false;
+
+		switch (conf->vartype)
+		{
+			case PGC_BOOL:
+			{
+				struct config_bool *lconf = (struct config_bool *) conf;
+				modified = (lconf->boot_val != *(lconf->variable));
+			}
+			break;
+
+			case PGC_INT:
+				{
+					struct config_int *lconf = (struct config_int *) conf;
+					modified = (lconf->boot_val != *(lconf->variable));
+				}
+				break;
+
+			case PGC_REAL:
+				{
+					struct config_real *lconf = (struct config_real *) conf;
+					modified = (lconf->boot_val != *(lconf->variable));
+				}
+				break;
+
+			case PGC_STRING:
+				{
+					struct config_string *lconf = (struct config_string *) conf;
+					modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+				}
+				break;
+
+			case PGC_ENUM:
+				{
+					struct config_enum *lconf = (struct config_enum *) conf;
+					modified = (lconf->boot_val != *(lconf->variable));
+				}
+				break;
+
+			default:
+				elog(ERROR, "unexcpected GUC type: %d", conf->vartype);
+		}
+
+		/* skip GUC variables that match the built-in default */
+		if (!modified)
+			continue;
+
+		/* assign to the values array */
+		result[*num] = conf;
+		*num = *num + 1;
+	}
+
+	return result;
+}
+
 /*
  * Return GUC variable value by name; optionally return canonical form of
  * name.  If the GUC is unset, then throw an error unless missing_ok is true,
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index d3f70fda08..05afca22aa 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -35,6 +35,7 @@ typedef struct ExplainState
 	bool		buffers;		/* print buffer usage */
 	bool		timing;			/* print detailed node timing */
 	bool		summary;		/* print total planning and execution timing */
+	bool		gucs;			/* print modified GUCs */
 	ExplainFormat format;		/* output format */
 	/* state for output formatting --- not reset for each new plan tree */
 	int			indent;			/* current indentation level */
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 6f9fdb6a5f..4942e192d6 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -267,5 +267,6 @@ extern void build_guc_variables(void);
 extern const char *config_enum_lookup_by_value(struct config_enum *record, int val);
 extern bool config_enum_lookup_by_name(struct config_enum *record,
 						   const char *value, int *retval);
+extern struct config_generic **get_modified_guc_options(int *num);
 
 #endif							/* GUC_TABLES_H */

--------------AEC5569485AC5EAFBC437091--