Re: explain plans with information about (modified) gucs
Hi, I've committed this, with some minor documentation tweaks. I've also fixed a minor bug in the last patch, where the group with settings was not properly labeled in some formats (e.g. json). Thanks to all the reviewers! regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
On Fri, 29 Mar 2019 at 22:07, Tomas Vondra wrote: > On Wed, Mar 27, 2019 at 09:06:04AM +0100, Rafia Sabih wrote: > >On Tue, 26 Mar 2019 at 21:04, Tomas Vondra > >wrote: > > > >> On Mon, Mar 18, 2019 at 11:31:48AM +0100, Rafia Sabih wrote: > >> >On Sun, 24 Feb 2019 at 00:06, Tomas Vondra < > tomas.von...@2ndquadrant.com> > >> wrote: > >> >> > >> >> Hi, > >> >> > >> >> attached is an updated patch, fixing and slightly tweaking the docs. > >> >> > >> >> > >> >> Barring objections, I'll get this committed later next week. > >> >> > >> >I was having a look at this patch, and this kept me wondering, > >> > > >> >+static void > >> >+ExplainShowSettings(ExplainState *es) > >> >+{ > >> >Is there some reason for not providing any explanation above this > >> >function just like the rest of the functions in this file? > >> > > >> >Similarly, for > >> > > >> >struct config_generic ** > >> >get_explain_guc_options(int *num) > >> >{ > >> > > >> >/* also bail out of there are no options */ > >> >+ if (!num) > >> >+ return; > >> >I think you meant 'if' instead if 'of' here. > >> > >> Thanks for the review - attached is a patch adding the missing comments, > >> and doing two additional minor improvements: > >> > >> 1) Renaming ExplainShowSettings to ExplainPrintSettings, to make it more > >> consistent with naming of the other functions in explain.c. > >> > >> 2) Actually counting GUC_EXPLAIN options, and only allocating space for > >> those in get_explain_guc_options, instead of using num_guc_variables. > The > >> diffrence is quite significant (~50 vs. ~300), and considering each > entry > >> is 8B it makes a difference because such large chunks tend to have > higher > >> palloc overhed (due to ALLOCSET_SEPARATE_THRESHOLD). > >> > >> > > Looks like the patch is in need of a rebase. > >At commit: 1983af8e899389187026cb34c1ca9d89ea986120 > > > >P.S. reject files attached. > > > > D'oh! That was a stupid mistake - I apparently attched just the delta > against > the previous patch version, i.e. the improvements I described. Attaches is > a > correct (and complete) patch. > > I planned to get this committed today, but considering this I'll wait until > early next week to allow for feedback. > > > The patch looks good to me. -- Regards, Rafia Sabih
Re: explain plans with information about (modified) gucs
On Wed, Mar 27, 2019 at 09:06:04AM +0100, Rafia Sabih wrote: On Tue, 26 Mar 2019 at 21:04, Tomas Vondra wrote: On Mon, Mar 18, 2019 at 11:31:48AM +0100, Rafia Sabih wrote: >On Sun, 24 Feb 2019 at 00:06, Tomas Vondra wrote: >> >> Hi, >> >> attached is an updated patch, fixing and slightly tweaking the docs. >> >> >> Barring objections, I'll get this committed later next week. >> >I was having a look at this patch, and this kept me wondering, > >+static void >+ExplainShowSettings(ExplainState *es) >+{ >Is there some reason for not providing any explanation above this >function just like the rest of the functions in this file? > >Similarly, for > >struct config_generic ** >get_explain_guc_options(int *num) >{ > >/* also bail out of there are no options */ >+ if (!num) >+ return; >I think you meant 'if' instead if 'of' here. Thanks for the review - attached is a patch adding the missing comments, and doing two additional minor improvements: 1) Renaming ExplainShowSettings to ExplainPrintSettings, to make it more consistent with naming of the other functions in explain.c. 2) Actually counting GUC_EXPLAIN options, and only allocating space for those in get_explain_guc_options, instead of using num_guc_variables. The diffrence is quite significant (~50 vs. ~300), and considering each entry is 8B it makes a difference because such large chunks tend to have higher palloc overhed (due to ALLOCSET_SEPARATE_THRESHOLD). Looks like the patch is in need of a rebase. At commit: 1983af8e899389187026cb34c1ca9d89ea986120 P.S. reject files attached. D'oh! That was a stupid mistake - I apparently attched just the delta against the previous patch version, i.e. the improvements I described. Attaches is a correct (and complete) patch. I planned to get this committed today, but considering this I'll wait until early next week to allow for feedback. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index 7b22927674..edc50f9368 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_settings = 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_settings", +"Log modified configuration parameters affecting query planning.", +NULL, + _explain_log_settings, +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->settings = auto_explain_log_settings; ExplainBeginOutput(es); ExplainQueryText(es, queryDesc); diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml index 120b168d45..296ae2de80 100644 --- a/doc/src/sgml/auto-explain.sgml +++ b/doc/src/sgml/auto-explain.sgml @@ -169,6 +169,24 @@ LOAD 'auto_explain'; + + + auto_explain.log_settings (boolean) + + auto_explain.log_settings configuration parameter + + + + + auto_explain.log_settings controls whether information + about modified configuration options affecting query planning are logged + with the execution plan. Only options affecting query planning with value + different from the built-in default value are considered. This parameter is + off by default. Only superusers can change this setting. + + + + auto_explain.log_format
Re: explain plans with information about (modified) gucs
On Tue, 26 Mar 2019 at 21:04, Tomas Vondra wrote: > On Mon, Mar 18, 2019 at 11:31:48AM +0100, Rafia Sabih wrote: > >On Sun, 24 Feb 2019 at 00:06, Tomas Vondra > wrote: > >> > >> Hi, > >> > >> attached is an updated patch, fixing and slightly tweaking the docs. > >> > >> > >> Barring objections, I'll get this committed later next week. > >> > >I was having a look at this patch, and this kept me wondering, > > > >+static void > >+ExplainShowSettings(ExplainState *es) > >+{ > >Is there some reason for not providing any explanation above this > >function just like the rest of the functions in this file? > > > >Similarly, for > > > >struct config_generic ** > >get_explain_guc_options(int *num) > >{ > > > >/* also bail out of there are no options */ > >+ if (!num) > >+ return; > >I think you meant 'if' instead if 'of' here. > > Thanks for the review - attached is a patch adding the missing comments, > and doing two additional minor improvements: > > 1) Renaming ExplainShowSettings to ExplainPrintSettings, to make it more > consistent with naming of the other functions in explain.c. > > 2) Actually counting GUC_EXPLAIN options, and only allocating space for > those in get_explain_guc_options, instead of using num_guc_variables. The > diffrence is quite significant (~50 vs. ~300), and considering each entry > is 8B it makes a difference because such large chunks tend to have higher > palloc overhed (due to ALLOCSET_SEPARATE_THRESHOLD). > > > Looks like the patch is in need of a rebase. At commit: 1983af8e899389187026cb34c1ca9d89ea986120 P.S. reject files attached. -- Regards, Rafia Sabih --- src/backend/utils/misc/guc.c +++ src/backend/utils/misc/guc.c @@ -8837,6 +8855,11 @@ ShowAllGUCConfig(DestReceiver *dest) end_tup_output(tstate); } +/* + * Returns an array of modified GUC options to show in EXPLAIN. Only options + * related to query planning (marked with GUC_EXPLAIN), with values different + * from built-in defaults. + */ struct config_generic ** get_explain_guc_options(int *num) { @@ -8844,7 +8867,13 @@ get_explain_guc_options(int *num) struct config_generic **result; *num = 0; - result = palloc(sizeof(struct config_generic *) * num_guc_variables); + + /* + * Allocate enough space to fit all GUC_EXPLAIN options. We may not + * need all the space, but there are fairly few such options so we + * don't waste a lot of memory. + */ + result = palloc(sizeof(struct config_generic *) * num_guc_explain_variables); for (i = 0; i < num_guc_variables; i++) { @@ -8912,6 +8941,8 @@ get_explain_guc_options(int *num) /* assign to the values array */ result[*num] = conf; *num = *num + 1; + + Assert(*num <= num_guc_explain_variables); } return result; --- src/backend/commands/explain.c +++ src/backend/commands/explain.c @@ -599,8 +599,12 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es, ExplainCloseGroup("Query", NULL, true, es); } +/* + * ExplainPrintSettings - + *Print summary of modified settings affecting query planning. + */ static void -ExplainShowSettings(ExplainState *es) +ExplainPrintSettings(ExplainState *es) { int num; struct config_generic **gucs; @@ -700,10 +704,10 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc) ExplainNode(ps, NIL, NULL, NULL, es); /* - * If requested, include information about GUC parameters that don't - * match the built-in defaults. + * If requested, include information about GUC parameters with values + * that don't match the built-in defaults. */ - ExplainShowSettings(es); + ExplainPrintSettings(es); } /*
Re: explain plans with information about (modified) gucs
On Mon, Mar 18, 2019 at 11:31:48AM +0100, Rafia Sabih wrote: On Sun, 24 Feb 2019 at 00:06, Tomas Vondra wrote: Hi, attached is an updated patch, fixing and slightly tweaking the docs. Barring objections, I'll get this committed later next week. I was having a look at this patch, and this kept me wondering, +static void +ExplainShowSettings(ExplainState *es) +{ Is there some reason for not providing any explanation above this function just like the rest of the functions in this file? Similarly, for struct config_generic ** get_explain_guc_options(int *num) { /* also bail out of there are no options */ + if (!num) + return; I think you meant 'if' instead if 'of' here. Thanks for the review - attached is a patch adding the missing comments, and doing two additional minor improvements: 1) Renaming ExplainShowSettings to ExplainPrintSettings, to make it more consistent with naming of the other functions in explain.c. 2) Actually counting GUC_EXPLAIN options, and only allocating space for those in get_explain_guc_options, instead of using num_guc_variables. The diffrence is quite significant (~50 vs. ~300), and considering each entry is 8B it makes a difference because such large chunks tend to have higher palloc overhed (due to ALLOCSET_SEPARATE_THRESHOLD). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index 8e48b94d4a..2a289b8b94 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -599,8 +599,12 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es, ExplainCloseGroup("Query", NULL, true, es); } +/* + * ExplainPrintSettings - + *Print summary of modified settings affecting query planning. + */ static void -ExplainShowSettings(ExplainState *es) +ExplainPrintSettings(ExplainState *es) { int num; struct config_generic **gucs; @@ -700,10 +704,10 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc) ExplainNode(ps, NIL, NULL, NULL, es); /* -* If requested, include information about GUC parameters that don't -* match the built-in defaults. +* If requested, include information about GUC parameters with values +* that don't match the built-in defaults. */ - ExplainShowSettings(es); + ExplainPrintSettings(es); } /* diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 7306d7c232..39f844ebc5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4532,6 +4532,7 @@ static struct config_generic **guc_variables; /* Current number of variables contained in the vector */ static int num_guc_variables; +static int num_guc_explain_variables; /* Vector capacity */ static int size_guc_variables; @@ -4796,6 +4797,7 @@ build_guc_variables(void) { int size_vars; int num_vars = 0; + int num_explain_vars = 0; struct config_generic **guc_vars; int i; @@ -4806,6 +4808,9 @@ build_guc_variables(void) /* Rather than requiring vartype to be filled in by hand, do this: */ conf->gen.vartype = PGC_BOOL; num_vars++; + + if (conf->gen.flags & GUC_EXPLAIN) + num_explain_vars++; } for (i = 0; ConfigureNamesInt[i].gen.name; i++) @@ -4814,6 +4819,9 @@ build_guc_variables(void) conf->gen.vartype = PGC_INT; num_vars++; + + if (conf->gen.flags & GUC_EXPLAIN) + num_explain_vars++; } for (i = 0; ConfigureNamesReal[i].gen.name; i++) @@ -4822,6 +4830,9 @@ build_guc_variables(void) conf->gen.vartype = PGC_REAL; num_vars++; + + if (conf->gen.flags & GUC_EXPLAIN) + num_explain_vars++; } for (i = 0; ConfigureNamesString[i].gen.name; i++) @@ -4830,6 +4841,9 @@ build_guc_variables(void) conf->gen.vartype = PGC_STRING; num_vars++; + + if (conf->gen.flags & GUC_EXPLAIN) + num_explain_vars++; } for (i = 0; ConfigureNamesEnum[i].gen.name; i++) @@ -4838,6 +4852,9 @@ build_guc_variables(void) conf->gen.vartype = PGC_ENUM; num_vars++; + + if (conf->gen.flags & GUC_EXPLAIN) + num_explain_vars++; } /* @@ -4869,6 +4886,7 @@ build_guc_variables(void) free(guc_variables); guc_variables = guc_vars; num_guc_variables = num_vars; + num_guc_explain_variables = num_explain_vars; size_guc_variables = size_vars; qsort((void *)
Re: explain plans with information about (modified) gucs
On Sun, 24 Feb 2019 at 00:06, Tomas Vondra wrote: > > Hi, > > attached is an updated patch, fixing and slightly tweaking the docs. > > > Barring objections, I'll get this committed later next week. > I was having a look at this patch, and this kept me wondering, +static void +ExplainShowSettings(ExplainState *es) +{ Is there some reason for not providing any explanation above this function just like the rest of the functions in this file? Similarly, for struct config_generic ** get_explain_guc_options(int *num) { /* also bail out of there are no options */ + if (!num) + return; I think you meant 'if' instead if 'of' here.
Re: explain plans with information about (modified) gucs
Hi, attached is an updated patch, fixing and slightly tweaking the docs. Barring objections, I'll get this committed later next week. -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >From ba0dc2578351e9c3c29cbd00860b31e3fd0acd6a Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sat, 23 Feb 2019 23:38:30 +0100 Subject: [PATCH] Allow including configuration parameters in explain output When analyzing execution plans, it's useful to know which configuration parameters affecting the planning were modified, and how. This commit adds SETTINGS option for EXPLAIN command, which includes GUC parameters with non-default value and marked with GUC_EXPLAIN flag. auto_explain is extended with log_settings option, doing the same thing. --- contrib/auto_explain/auto_explain.c | 13 ++ doc/src/sgml/auto-explain.sgml | 18 +++ doc/src/sgml/ref/explain.sgml | 12 ++ src/backend/commands/explain.c | 71 + src/backend/utils/misc/guc.c| 229 +--- src/include/commands/explain.h | 1 + src/include/utils/guc.h | 2 + src/include/utils/guc_tables.h | 1 + 8 files changed, 295 insertions(+), 52 deletions(-) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index 7b22927674..edc50f9368 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_settings = 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_settings", + "Log modified configuration parameters affecting query planning.", + NULL, + _explain_log_settings, + 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->settings = auto_explain_log_settings; ExplainBeginOutput(es); ExplainQueryText(es, queryDesc); diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml index 120b168d45..296ae2de80 100644 --- a/doc/src/sgml/auto-explain.sgml +++ b/doc/src/sgml/auto-explain.sgml @@ -169,6 +169,24 @@ LOAD 'auto_explain'; + + + auto_explain.log_settings (boolean) + + auto_explain.log_settings configuration parameter + + + + + auto_explain.log_settings controls whether information + about modified configuration options affecting query planning are logged + with the execution plan. Only options affecting query planning with value + different from the built-in default value are considered. This parameter is + off by default. Only superusers can change this setting. + + + + auto_explain.log_format (enum) diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml index 8dc0d7038a..385d10411f 100644 --- a/doc/src/sgml/ref/explain.sgml +++ b/doc/src/sgml/ref/explain.sgml @@ -39,6 +39,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ] VERBOSE [ boolean ] COSTS [ boolean ] +SETTINGS [ boolean ] BUFFERS [ boolean ] TIMING [ boolean ] SUMMARY [ boolean ] @@ -152,6 +153,17 @@ ROLLBACK; + +SETTINGS + + + Include information on configuration parameters. Specifically, include + options affecting query planning with value different from the built-in + default value. This parameter defaults to FALSE. + + + + BUFFERS diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index 1831ea81cf..8e48b94d4a 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -29,6 +29,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" @@ -162,6 +163,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, "settings") == 0) + es->settings = defGetBoolean(opt); else if (strcmp(opt->defname, "timing") == 0) {
Re: explain plans with information about (modified) gucs
On 2019-02-15 17:10:28 +0100, Tomas Vondra wrote: > True. So you agree printing the values as text (as the patch currently > does) seems enough? I guess I'm fine with that. I do agree, yes.
Re: explain plans with information about (modified) gucs
On 2/14/19 8:55 PM, Andres Freund wrote: > Hi, > > On 2019-01-15 02:39:49 +0100, Tomas Vondra wrote: >> >> >> On 1/14/19 11:13 PM, Alvaro Herrera wrote: >>> On 2019-Jan-14, Tomas Vondra wrote: >>> The one slightly annoying issue is that currently all the options are formatted as text, including e.g. cpu_tuple_cost. That's because the GUC options may have show hook, so I can't access the value directly (not sure if there's an option around it). >>> >>> I think the problem is that you'd have to know how to print the value, >>> which can be in one of several different C types. You'd have to grow >>> some more infrastructure in the GUC tables, I think, and that doesn't >>> seem worth the trouble. Printing as text seems enough. >>> >> >> I don't think the number of formats is such a big issue - the range of >> formats is quite limited: PGC_BOOL, PGC_INT, PGC_REAL, PGC_STRING and >> PGC_ENUM. But the show hook simply returns string, and I'm not sure it's >> guaranteed it matches the raw value (afaik the assign/show hooks can do >> all kinds of funny stuff). > > Yea, but the underlying values are quite useless for > humans. E.g. counting shared_buffers, wal_buffers etc in weird units is > not helpful. So you'd need to support the different units for each. > True. So you agree printing the values as text (as the patch currently does) seems enough? I guess I'm fine with that. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
Hi, On 2019-01-15 02:39:49 +0100, Tomas Vondra wrote: > > > On 1/14/19 11:13 PM, Alvaro Herrera wrote: > > On 2019-Jan-14, Tomas Vondra wrote: > > > >> The one slightly annoying issue is that currently all the options are > >> formatted as text, including e.g. cpu_tuple_cost. That's because the GUC > >> options may have show hook, so I can't access the value directly (not > >> sure if there's an option around it). > > > > I think the problem is that you'd have to know how to print the value, > > which can be in one of several different C types. You'd have to grow > > some more infrastructure in the GUC tables, I think, and that doesn't > > seem worth the trouble. Printing as text seems enough. > > > > I don't think the number of formats is such a big issue - the range of > formats is quite limited: PGC_BOOL, PGC_INT, PGC_REAL, PGC_STRING and > PGC_ENUM. But the show hook simply returns string, and I'm not sure it's > guaranteed it matches the raw value (afaik the assign/show hooks can do > all kinds of funny stuff). Yea, but the underlying values are quite useless for humans. E.g. counting shared_buffers, wal_buffers etc in weird units is not helpful. So you'd need to support the different units for each. Greetings, Andres Freund
Re: explain plans with information about (modified) gucs
On Tue, Jan 22, 2019 at 02:29:06AM +0100, Tomas Vondra wrote: > Yes, that's an omission in the docs. Will fix. Could you fix your patch then? I am moving it to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: explain plans with information about (modified) gucs
On 1/22/19 1:35 AM, John Naylor wrote: > On Sun, Jan 20, 2019 at 2:31 PM Tomas Vondra > wrote: >> Attached is v6 of the patch, adopting "settings" instead of "guc". > > Ok, looks good. I tried changing config values with the .conf file, > alter system, and alter database, and tried a few queries with > auto_explain. I made a pass through the config parameters, and don't > see anything obviously left out. I have no comments on the source > code. > > One thing stands out: For the docs on auto_explain, all other options > have "Only superusers can change this setting.", but log_settings > doesn't. Yes, that's an omission in the docs. Will fix. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
On Sun, Jan 20, 2019 at 2:31 PM Tomas Vondra wrote: > Attached is v6 of the patch, adopting "settings" instead of "guc". Ok, looks good. I tried changing config values with the .conf file, alter system, and alter database, and tried a few queries with auto_explain. I made a pass through the config parameters, and don't see anything obviously left out. I have no comments on the source code. One thing stands out: For the docs on auto_explain, all other options have "Only superusers can change this setting.", but log_settings doesn't. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
Hi John, On 1/16/19 10:08 PM, John Naylor wrote: >> [v5] > > Hi Tomas, > Peter suggested upthread to use 'settings' rather than 'gucs' for the > explain option (and output?), and you seemed to agree. Are you going > to include that in a future version? Speaking of the output, v5's > default text doesn't match that of formatted text ('GUCs' / 'GUC'). > Attached is v6 of the patch, adopting "settings" instead of "guc". regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index 7b22927674..edc50f9368 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_settings = 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_settings", + "Log modified configuration parameters affecting query planning.", + NULL, + _explain_log_settings, + 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->settings = auto_explain_log_settings; ExplainBeginOutput(es); ExplainQueryText(es, queryDesc); diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml index 120b168d45..b0a129cfee 100644 --- a/doc/src/sgml/auto-explain.sgml +++ b/doc/src/sgml/auto-explain.sgml @@ -169,6 +169,23 @@ LOAD 'auto_explain'; + + + auto_explain.log_settings (boolean) + + auto_explain.log_settings configuration parameter + + + + + auto_explain.log_settings controls whether information + about modified configuration options affecting query planning 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. + + + + auto_explain.log_format (enum) diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml index 8dc0d7038a..a7dca5f208 100644 --- a/doc/src/sgml/ref/explain.sgml +++ b/doc/src/sgml/ref/explain.sgml @@ -39,6 +39,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ] VERBOSE [ boolean ] COSTS [ boolean ] +SETTINGS [ boolean ] BUFFERS [ boolean ] TIMING [ boolean ] SUMMARY [ boolean ] @@ -152,6 +153,17 @@ ROLLBACK; + +SETTINGS + + + Include information on configuration parameters. Specifically, include + configuration parameters that were modified and are considered to affect + query planning. This parameter defaults to FALSE. + + + + BUFFERS diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index ae7f038203..4309c8d137 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -30,6 +30,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" @@ -163,6 +164,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, "settings") == 0) + es->settings = defGetBoolean(opt); else if (strcmp(opt->defname, "timing") == 0) { timing_set = true; @@ -597,6 +600,68 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es, ExplainCloseGroup("Query", NULL, true, es); } +static void +ExplainShowSettings(ExplainState *es) +{ + int num; + struct config_generic **gucs; + + /* bail out if GUC information not requested */ + if (!es->settings) + return; + + gucs = get_explain_guc_options(); + + /* also bail out of there are no options */ + if (!num) + return; + + if (es->format != EXPLAIN_FORMAT_TEXT) + { + int i; + + ExplainOpenGroup("Settings", "Settings", false, es); + + for (i = 0; i < num; i++) + { + char *setting; + struct config_generic *conf = gucs[i]; + + setting = GetConfigOptionByName(conf->name, NULL, true); + +
Re: explain plans with information about (modified) gucs
> [v5] Hi Tomas, Peter suggested upthread to use 'settings' rather than 'gucs' for the explain option (and output?), and you seemed to agree. Are you going to include that in a future version? Speaking of the output, v5's default text doesn't match that of formatted text ('GUCs' / 'GUC').
Re: explain plans with information about (modified) gucs
On 1/14/19 11:13 PM, Alvaro Herrera wrote: > On 2019-Jan-14, Tomas Vondra wrote: > >> The one slightly annoying issue is that currently all the options are >> formatted as text, including e.g. cpu_tuple_cost. That's because the GUC >> options may have show hook, so I can't access the value directly (not >> sure if there's an option around it). > > I think the problem is that you'd have to know how to print the value, > which can be in one of several different C types. You'd have to grow > some more infrastructure in the GUC tables, I think, and that doesn't > seem worth the trouble. Printing as text seems enough. > I don't think the number of formats is such a big issue - the range of formats is quite limited: PGC_BOOL, PGC_INT, PGC_REAL, PGC_STRING and PGC_ENUM. But the show hook simply returns string, and I'm not sure it's guaranteed it matches the raw value (afaik the assign/show hooks can do all kinds of funny stuff). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
Tomas Vondra-4 wrote > Hello Sergei, > >> This patch correlates with my proposal >> "add session information column to pg_stat_statements" >> https://www.postgresql.org/message-id/3aa097d7-7c47-187b-5913-db8366cd4491%40gmail.com >> They both address the problem to identify the factors that make >> different execution plans for the same SQL statements. You are >> interested in the current settings that affect the execution plan, I'm >> concerned about historical data in pg_stat_statements. From my >> experience the most often offending settings are >> current_schemas/search_path and current_user. Please have in mind that >> probably the same approach that you will use to extend explain plan >> functionality will be eventually implemented to extend >> pg_stat_statements. > > Possibly, although I don't have an ambition to export the GUCs into > pg_stat_statements in this patch. There's an issue with merging > different values of GUCs in different executions of a statement, and > it's unclear how to solve that. > > [...] > > -- > Tomas Vondra http://www.2ndQuadrant.com > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services This explain with GUCs feature can be included very easily for historical data management in pg_store_plans or pg_stat_sql_plans extensions (that both use a planid based on the normalized plan text). Regards PAscal -- Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: explain plans with information about (modified) gucs
On 2019-Jan-14, Tomas Vondra wrote: > The one slightly annoying issue is that currently all the options are > formatted as text, including e.g. cpu_tuple_cost. That's because the GUC > options may have show hook, so I can't access the value directly (not > sure if there's an option around it). I think the problem is that you'd have to know how to print the value, which can be in one of several different C types. You'd have to grow some more infrastructure in the GUC tables, I think, and that doesn't seem worth the trouble. Printing as text seems enough. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
Hello Sergei, > This patch correlates with my proposal > "add session information column to pg_stat_statements" > https://www.postgresql.org/message-id/3aa097d7-7c47-187b-5913-db8366cd4491%40gmail.com > They both address the problem to identify the factors that make > different execution plans for the same SQL statements. You are > interested in the current settings that affect the execution plan, I'm > concerned about historical data in pg_stat_statements. From my > experience the most often offending settings are > current_schemas/search_path and current_user. Please have in mind that > probably the same approach that you will use to extend explain plan > functionality will be eventually implemented to extend > pg_stat_statements. Possibly, although I don't have an ambition to export the GUCs into pg_stat_statements in this patch. There's an issue with merging different values of GUCs in different executions of a statement, and it's unclear how to solve that. > I think that the list of the GUCs that are reported > by explain plan should be structured like JSON, something like > extended_settings: { "current_schemas" : ["pg_catalog", "s1", "s2", "public"], > "current_user" : "user1", > "enable_nestloop" : "off", > "work_mem" = "32MB" > } > It is less important for yours use case explain, but is important > for pg_stat_statements case. > The pg_stat_statements is often accessed by monitoring and reporting > tools, and it will be a good idea to have > the data here in the > structured and easily parsed format. Yes, that's a good point. I think it's fine to keep the current format for TEXT output, and use a structured format when the explain format is set to json or yaml. That's what we do for data about Hash nodes, for example (see show_hash_info). So I've done that in the attached v5 of the patch, which now produces something like this: test=# explain (gucs, format json) select * from t; QUERY PLAN - [ + {+ "Plan": { + "Node Type": "Seq Scan", + "Parallel Aware": false, + "Relation Name": "t",+ "Alias": "t",+ "Startup Cost": 0.00,+ "Total Cost": 61.00, + "Plan Rows": 2550, + "Plan Width": 4 + }, + "GUC": [ + "cpu_tuple_cost": "0.02",+ "work_mem": "1GB"+ ] + }+ ] (1 row) The one slightly annoying issue is that currently all the options are formatted as text, including e.g. cpu_tuple_cost. That's because the GUC options may have show hook, so I can't access the value directly (not sure if there's an option around it). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index 7b22927674..d3c4def942 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, + _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'; + + + auto_explain.log_gucs (boolean) + + auto_explain.log_gucs configuration parameter + + + + + auto_explain.log_gucs 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. + + + +
Re: explain plans with information about (modified) gucs
Hi, every now and then I have to investigate an execution plan that is strange in some way and I can't reproduce the same behavior. Usually it's simply due to data distribution changing since the problem was observed (say, after a nightly batch load/update). In many cases it however may be due to some local GUC tweaks, usually addressing some query specific issues (say, disabling nested loops or lowering join_collapse_limit). I've repeatedly ran into cases where the GUC was not properly reset to the "regular" value, and it's rather difficult to identify this is what's happening. Or cases with different per-user settings and connection pooling (SET SESSION AUTHORIZATION / ROLE etc.). So 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): test=# explain (gucs) select * from t; QUERY PLAN Seq Scan on t (cost=0.00..35.50 rows=2550 width=4) GUCs: application_name = 'x', client_encoding = 'UTF8', cpu_tuple_cost = '0.01' (2 rows) Of course, this directly applies to auto_explain too, which gets a new option log_gucs. The patch is quite trivial, but there are about three open questions: 1) names of the options I'm not particularly happy with calling the option "gucs" - it's an acronym and many users have little idea what GUC stands for. So I think a better name would be desirable, but I'm not sure what would that be. Options? Parameters? 2) format of output At this point the names/values are simply formatted into a one-line string. That's not particularly readable, and it's not very useful for the YAML/JSON formats I guess. So adding each modified GUC as an extra text property would be better. 3) identifying modified (and interesting) GUCs 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. For now I've used /* return only options that were modified (not as in config file) */ if ((conf->source <= PGC_S_ARGV) || (conf->source == PGC_S_OVERRIDE)) continue; which generally does the right thing, although it also includes stuff like application_name or client_encoding. But perhaps it'd be better to whitelist the GUCs in some way, because some of the user-defined GUCs may be sensitive and should not be included in plans. Opinions? regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services This patch correlates with my proposal "add session information column to pg_stat_statements" https://www.postgresql.org/message-id/3aa097d7-7c47-187b-5913-db8366cd4491%40gmail.com They both address the problem to identify the factors that make different execution plans for the same SQL statements. You are interested in the current settings that affect the execution plan, I'm concerned about historical data in pg_stat_statements. From my experience the most often offending settings are current_schemas/search_path and current_user. Please have in mind that probably the same approach that you will use to extend explain plan functionality will be eventually implemented to extend pg_stat_statements. I think that the list of the GUCs that are reported by explain plan should be structured like JSON, something like extended_settings: { "current_schemas" : ["pg_catalog", "s1", "s2", "public"], "current_user" : "user1", "enable_nestloop" : "off", "work_mem" = "32MB" } It is less important for yours use case explain, but is important for pg_stat_statements case. The pg_stat_statements is often accessed by monitoring and reporting tools, and it will be a good idea to have the data here in the structured and easily parsed format. Thank you, Sergei Agalakov
Re: explain plans with information about (modified) gucs
On 1/2/19 4:20 PM, Peter Eisentraut wrote: > On 14/12/2018 12:41, Tomas Vondra wrote: >> 1) names of the options >> >> I'm not particularly happy with calling the option "gucs" - it's an >> acronym and many users have little idea what GUC stands for. So I think >> a better name would be desirable, but I'm not sure what would that be. >> Options? Parameters? > > "settings" > > (see pg_settings, SET) > Yup, that seems like a better choice. Thanks. -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
On 14/12/2018 12:41, Tomas Vondra wrote: > 1) names of the options > > I'm not particularly happy with calling the option "gucs" - it's an > acronym and many users have little idea what GUC stands for. So I think > a better name would be desirable, but I'm not sure what would that be. > Options? Parameters? "settings" (see pg_settings, SET) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
út 1. 1. 2019 v 20:11 odesílatel Tomas Vondra napsal: > > > On 1/1/19 6:48 PM, Pavel Stehule wrote: > > > > > > út 1. 1. 2019 v 18:39 odesílatel Tomas Vondra > > mailto:tomas.von...@2ndquadrant.com>> > napsal: > > > > Attached is v4, changing how GUCs are picked for inclusion on the > query > > plans. Instead of picking the GUCs based on group and/or explicitly, > a > > new GUC_EXPLAIN flag is used for that. > > > > I went through GUCs defined in guc.c and marked those in > QUERY_TUNING* > > groups accordingly, with the exception of default_statistics_target > > because that seems somewhat useless without showing the value used to > > actually analyze the table (and/or columns). > > > > I've also included a couple of other GUCs, that I find to be > relevant: > > > > - parallel_leader_participation > > - max_parallel_workers_per_gather > > - max_parallel_workers > > - search_path > > - effective_io_concurrency > > - work_mem > > - temp_buffers > > - plan_cache_mode > > > > > > when plan_cache_mode is auto, you know maybe too less executed query. > > Maybe you can read somewhere if plan was custom or generic. > > > > This patch is about showing GUCs, not such additional internal info. > Also, you'll see the plan actually used. > I understand to your goal, and I understand so collecting some data inside can be hard or impossible. But if you collect a data important for understanding to planner behave/decision, then some important information is inside plancache too. - and now it is not visible from user space. It is just my note - so not only GUC are interesting - nothing more. > > > > I think this covers the interesting GUCs pretty well, although > perhaps I > > missed something. > > > > > > seq_page_cost, random_page_cost, from_collapse_limit, > > join_collapse_limit, ... enable_*** > > > > All these GUCs are included, of course. > ok - thank you for info. Regards Pavel > > > > -- > Tomas Vondra http://www.2ndQuadrant.com > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
Re: explain plans with information about (modified) gucs
On 1/1/19 6:48 PM, Pavel Stehule wrote: > > > út 1. 1. 2019 v 18:39 odesílatel Tomas Vondra > mailto:tomas.von...@2ndquadrant.com>> napsal: > > Attached is v4, changing how GUCs are picked for inclusion on the query > plans. Instead of picking the GUCs based on group and/or explicitly, a > new GUC_EXPLAIN flag is used for that. > > I went through GUCs defined in guc.c and marked those in QUERY_TUNING* > groups accordingly, with the exception of default_statistics_target > because that seems somewhat useless without showing the value used to > actually analyze the table (and/or columns). > > I've also included a couple of other GUCs, that I find to be relevant: > > - parallel_leader_participation > - max_parallel_workers_per_gather > - max_parallel_workers > - search_path > - effective_io_concurrency > - work_mem > - temp_buffers > - plan_cache_mode > > > when plan_cache_mode is auto, you know maybe too less executed query. > Maybe you can read somewhere if plan was custom or generic. > This patch is about showing GUCs, not such additional internal info. Also, you'll see the plan actually used. > > I think this covers the interesting GUCs pretty well, although perhaps I > missed something. > > > seq_page_cost, random_page_cost, from_collapse_limit, > join_collapse_limit, ... enable_*** > All these GUCs are included, of course. -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
út 1. 1. 2019 v 18:39 odesílatel Tomas Vondra napsal: > Attached is v4, changing how GUCs are picked for inclusion on the query > plans. Instead of picking the GUCs based on group and/or explicitly, a > new GUC_EXPLAIN flag is used for that. > > I went through GUCs defined in guc.c and marked those in QUERY_TUNING* > groups accordingly, with the exception of default_statistics_target > because that seems somewhat useless without showing the value used to > actually analyze the table (and/or columns). > > I've also included a couple of other GUCs, that I find to be relevant: > > - parallel_leader_participation > - max_parallel_workers_per_gather > - max_parallel_workers > - search_path > - effective_io_concurrency > - work_mem > - temp_buffers > - plan_cache_mode > when plan_cache_mode is auto, you know maybe too less executed query. Maybe you can read somewhere if plan was custom or generic. > I think this covers the interesting GUCs pretty well, although perhaps I > missed something. > seq_page_cost, random_page_cost, from_collapse_limit, join_collapse_limit, ... enable_*** > The one bit that needs fixing is escaping the GUC values when showing > them in the plan. Looking at the other places that currently escape > stuff, I see they only care about YAML/JSON/XML and leave the regular > output unescaped. I was wondering if it's OK with the current format > with all GUCs on a single line > > QUERY PLAN > --- > Seq Scan on t (cost=0.00..54.63 rows=13 width=4) >Filter: ('x''y'::text = (a)::text) > GUCs: enable_nestloop = 'off', work_mem = '32MB' > (3 rows) > > but I suppose it is, because without the escaping a user can break > whatever format we use. So I'll do the same thing, escaping just the > structured formats (YAML et al). > > The question however is whether someone has a better formatting idea? > > regards > > -- > Tomas Vondra http://www.2ndQuadrant.com > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services > >
Re: explain plans with information about (modified) gucs
Attached is v4, changing how GUCs are picked for inclusion on the query plans. Instead of picking the GUCs based on group and/or explicitly, a new GUC_EXPLAIN flag is used for that. I went through GUCs defined in guc.c and marked those in QUERY_TUNING* groups accordingly, with the exception of default_statistics_target because that seems somewhat useless without showing the value used to actually analyze the table (and/or columns). I've also included a couple of other GUCs, that I find to be relevant: - parallel_leader_participation - max_parallel_workers_per_gather - max_parallel_workers - search_path - effective_io_concurrency - work_mem - temp_buffers - plan_cache_mode I think this covers the interesting GUCs pretty well, although perhaps I missed something. The one bit that needs fixing is escaping the GUC values when showing them in the plan. Looking at the other places that currently escape stuff, I see they only care about YAML/JSON/XML and leave the regular output unescaped. I was wondering if it's OK with the current format with all GUCs on a single line QUERY PLAN --- Seq Scan on t (cost=0.00..54.63 rows=13 width=4) Filter: ('x''y'::text = (a)::text) GUCs: enable_nestloop = 'off', work_mem = '32MB' (3 rows) but I suppose it is, because without the escaping a user can break whatever format we use. So I'll do the same thing, escaping just the structured formats (YAML et al). The question however is whether someone has a better formatting idea? regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 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, + _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'; + + + auto_explain.log_gucs (boolean) + + auto_explain.log_gucs configuration parameter + + + + + auto_explain.log_gucs 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. + + + + auto_explain.log_format (enum) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index 094e977fb5..e32497d30d 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -30,6 +30,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" @@ -163,6 +164,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; @@ -634,6 +637,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 =
Re: explain plans with information about (modified) gucs
On 12/18/18 12:43 AM, Andres Freund wrote: > Hi, > > On 2018-12-18 00:38:16 +0100, Tomas Vondra wrote: >> On 12/17/18 11:16 PM, Tom Lane wrote: >>> Tomas Vondra writes: Yeah, I've been thinking about that too. Currently it gets filtered out because it's in the CLIENT_CONN_STATEMENT group, but the code only includes QUERY_TUNING_*. But we don't want to include everything from CLIENT_CONN_STATEMENT, because that would include various kinds of timeouts, vacuuming parameters, etc. And the same issue applies to work_mem, which is in RESOURCES_MEM. And of course, there's a lot of unrelated stuff in RESOURCES_MEM. So I guess we'll need to enable QUERY_TUNING_* and then selectively a couple of individual options from other groups. >>> >>> This is putting way too much policy into the mechanism, if you ask me. >>> >> >> Doesn't that depend on how it'd be implemented? I have not envisioned to >> make these decisions in explain.c, but rather to keep them in guc.c >> somehow. Say in a function like this: >> >> bool guc_affects_query_planning(config_generic *conf); >> >> which would be a fairly simple check outlined before (QUERY_TUNING_* >> plus a couple of individual GUCs). Other use cases might provide similar >> filters. > > If we were to do that, I'd suggest implementing a GUC_* flag specified > in the definition of the GUC, rather than a separate function containing > all the knowledge (but such a function could obviously still be used to > return such a fact). > Seems reasonable. > >> An alternative would be to somehow track this in the GUC definitions >> directly (similarly to how we track the group), but that seems rather >> inflexible and I'm not sure how would that handle ad-hoc use cases. > > Not sure what problem you see here? > My main concern with that was how many flags could we need, if we use this as the way to implement this and similar use cases. There are 32 bits available, and 24 of them are already used for GUC_* flags. So if we use this as an "official" way to support similar use cases, we could run out of space pretty fast. The callback would also allow ad hoc filtering, which is not very practical from extensions (e.g. you can't define a new flag, because it might conflict with something defined in another extension). But I think that's nonsense - so far we have not seen any such use case, so it's pretty pointless to design for it. If that changes and some new use case is proposed in the future, we can rethink this based on it. I'll go with a new flag, marking all GUCs related to query planning, and post a new patch soon. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
Hi, On 2018-12-18 00:38:16 +0100, Tomas Vondra wrote: > On 12/17/18 11:16 PM, Tom Lane wrote: > > Tomas Vondra writes: > >> Yeah, I've been thinking about that too. Currently it gets filtered out > >> because it's in the CLIENT_CONN_STATEMENT group, but the code only > >> includes QUERY_TUNING_*. > >> But we don't want to include everything from CLIENT_CONN_STATEMENT, > >> because that would include various kinds of timeouts, vacuuming > >> parameters, etc. > >> And the same issue applies to work_mem, which is in RESOURCES_MEM. And > >> of course, there's a lot of unrelated stuff in RESOURCES_MEM. > >> So I guess we'll need to enable QUERY_TUNING_* and then selectively a > >> couple of individual options from other groups. > > > > This is putting way too much policy into the mechanism, if you ask me. > > > > Doesn't that depend on how it'd be implemented? I have not envisioned to > make these decisions in explain.c, but rather to keep them in guc.c > somehow. Say in a function like this: > > bool guc_affects_query_planning(config_generic *conf); > > which would be a fairly simple check outlined before (QUERY_TUNING_* > plus a couple of individual GUCs). Other use cases might provide similar > filters. If we were to do that, I'd suggest implementing a GUC_* flag specified in the definition of the GUC, rather than a separate function containing all the knowledge (but such a function could obviously still be used to return such a fact). > An alternative would be to somehow track this in the GUC definitions > directly (similarly to how we track the group), but that seems rather > inflexible and I'm not sure how would that handle ad-hoc use cases. Not sure what problem you see here? Greetings, Andres Freund
Re: explain plans with information about (modified) gucs
On 12/17/18 11:16 PM, Tom Lane wrote: > Tomas Vondra writes: >> On 12/17/18 10:56 PM, legrand legrand wrote: >>> what would you think about adding >>> search_path >>> to that list ? > >> Yeah, I've been thinking about that too. Currently it gets filtered out >> because it's in the CLIENT_CONN_STATEMENT group, but the code only >> includes QUERY_TUNING_*. >> But we don't want to include everything from CLIENT_CONN_STATEMENT, >> because that would include various kinds of timeouts, vacuuming >> parameters, etc. >> And the same issue applies to work_mem, which is in RESOURCES_MEM. And >> of course, there's a lot of unrelated stuff in RESOURCES_MEM. >> So I guess we'll need to enable QUERY_TUNING_* and then selectively a >> couple of individual options from other groups. > > This is putting way too much policy into the mechanism, if you ask me. > Doesn't that depend on how it'd be implemented? I have not envisioned to make these decisions in explain.c, but rather to keep them in guc.c somehow. Say in a function like this: bool guc_affects_query_planning(config_generic *conf); which would be a fairly simple check outlined before (QUERY_TUNING_* plus a couple of individual GUCs). Other use cases might provide similar filters. An alternative would be to somehow track this in the GUC definitions directly (similarly to how we track the group), but that seems rather inflexible and I'm not sure how would that handle ad-hoc use cases. > At least for the auto_explain use case, it'd make far more sense for > the user to be able to specify which GUCs he wants the query space > to be divided according to. While it's possible to imagine giving > auto_explain a control setting that's a list of GUC names, I'm not > sure how we adapt the idea for other use-cases. But the direction > you're headed here will mostly ensure that nobody is happy. > I certainly don't want to base this on explicitly listing "interesting" GUCs anywhere. That would make it pretty useless for the use case I care about, i.e. using auto_explain to investigate slow plans, when I don't really know what GUC the application might have changed (certainly not in advance). I can't really say how to adopt this to other use cases, considering there are none proposed (and I can't think of any either). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
Tomas Vondra writes: > On 12/17/18 10:56 PM, legrand legrand wrote: >> what would you think about adding >> search_path >> to that list ? > Yeah, I've been thinking about that too. Currently it gets filtered out > because it's in the CLIENT_CONN_STATEMENT group, but the code only > includes QUERY_TUNING_*. > But we don't want to include everything from CLIENT_CONN_STATEMENT, > because that would include various kinds of timeouts, vacuuming > parameters, etc. > And the same issue applies to work_mem, which is in RESOURCES_MEM. And > of course, there's a lot of unrelated stuff in RESOURCES_MEM. > So I guess we'll need to enable QUERY_TUNING_* and then selectively a > couple of individual options from other groups. This is putting way too much policy into the mechanism, if you ask me. At least for the auto_explain use case, it'd make far more sense for the user to be able to specify which GUCs he wants the query space to be divided according to. While it's possible to imagine giving auto_explain a control setting that's a list of GUC names, I'm not sure how we adapt the idea for other use-cases. But the direction you're headed here will mostly ensure that nobody is happy. regards, tom lane
Re: explain plans with information about (modified) gucs
Hi, On 12/17/18 10:56 PM, legrand legrand wrote: > what would you think about adding > search_path > to that list ? > Yeah, I've been thinking about that too. Currently it gets filtered out because it's in the CLIENT_CONN_STATEMENT group, but the code only includes QUERY_TUNING_*. But we don't want to include everything from CLIENT_CONN_STATEMENT, because that would include various kinds of timeouts, vacuuming parameters, etc. And the same issue applies to work_mem, which is in RESOURCES_MEM. And of course, there's a lot of unrelated stuff in RESOURCES_MEM. So I guess we'll need to enable QUERY_TUNING_* and then selectively a couple of individual options from other groups. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
what would you think about adding search_path to that list ? Regards PAscal -- Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: explain plans with information about (modified) gucs
On 12/14/18 4:32 PM, Tomas Vondra wrote: > > > On 12/14/18 4:21 PM, Tom Lane wrote: >> Tomas Vondra 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 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, + _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'; + + + auto_explain.log_gucs (boolean) + + auto_explain.log_gucs configuration parameter + + + + + auto_explain.log_gucs 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. + + + + auto_explain.log_format (enum) 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
Re: explain plans with information about (modified) gucs
On 12/14/18 4:21 PM, Tom Lane wrote: > Tomas Vondra 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. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
Tomas Vondra 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. > 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 :-( regards, tom lane
Re: explain plans with information about (modified) gucs
On 12/14/18 3:01 PM, Tomas Vondra wrote: > On 12/14/18 2:05 PM, Jim Finnerty wrote: >> You might want to only include the GUCs that are query tuning parameters, >> i.e., those returned by: >> >> SELECT name, setting, category >> FROM pg_settings >> WHERE category LIKE 'Query Tuning%' >> ORDER BY category, name; >> > > Good idea! Thanks. V2 filtering the options to QUERY_TUNING group (and subgroups). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 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, + _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'; + + + auto_explain.log_gucs (boolean) + + auto_explain.log_gucs configuration parameter + + + + + auto_explain.log_gucs 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. + + + + auto_explain.log_format (enum) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index de09ded65b..b8cab69f71 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; @@ -547,6 +550,37 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es, /* Create textual dump of plan tree */ ExplainPrintPlan(es, queryDesc); + if (es->gucs) + { + int i; + int num; + StringInfoData str; + struct config_generic **gucs; + + gucs = get_modified_guc_options(); + + for (i = 0; i < num; i++) + { + char *setting; + struct config_generic *conf = gucs[i]; + + if (i == 0) +initStringInfo(); + else +appendStringInfoString(, ", "); + + setting = GetConfigOptionByName(conf->name, NULL, true); + + if (setting) +appendStringInfo(, "%s = '%s'", conf->name, setting); + else +appendStringInfo(, "%s = NULL", conf->name); + } + + if (num > 0) + ExplainPropertyText("GUCs", str.data, es); + } + if (es->summary && planduration) { double plantime = INSTR_TIME_GET_DOUBLE(*planduration); diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 6fe1939881..2d37760c7a 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -8556,6 +8556,45 @@ 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++) + { + 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) && +
Re: explain plans with information about (modified) gucs
On 12/14/18 2:05 PM, Jim Finnerty wrote: > You might want to only include the GUCs that are query tuning parameters, > i.e., those returned by: > > SELECT name, setting, category > FROM pg_settings > WHERE category LIKE 'Query Tuning%' > ORDER BY category, name; > Good idea! Thanks. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: explain plans with information about (modified) gucs
pá 14. 12. 2018 v 12:41 odesílatel Tomas Vondra < tomas.von...@2ndquadrant.com> napsal: > Hi, > > every now and then I have to investigate an execution plan that is > strange in some way and I can't reproduce the same behavior. Usually > it's simply due to data distribution changing since the problem was > observed (say, after a nightly batch load/update). > > In many cases it however may be due to some local GUC tweaks, usually > addressing some query specific issues (say, disabling nested loops or > lowering join_collapse_limit). I've repeatedly ran into cases where the > GUC was not properly reset to the "regular" value, and it's rather > difficult to identify this is what's happening. Or cases with different > per-user settings and connection pooling (SET SESSION AUTHORIZATION / > ROLE etc.). > > So 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): > > test=# explain (gucs) select * from t; > > QUERY PLAN > >Seq Scan on t (cost=0.00..35.50 rows=2550 width=4) >GUCs: application_name = 'x', client_encoding = 'UTF8', > cpu_tuple_cost = '0.01' >(2 rows) > > Of course, this directly applies to auto_explain too, which gets a new > option log_gucs. > > The patch is quite trivial, but there are about three open questions: > > 1) names of the options > > I'm not particularly happy with calling the option "gucs" - it's an > acronym and many users have little idea what GUC stands for. So I think > a better name would be desirable, but I'm not sure what would that be. > Options? Parameters? > > 2) format of output > > At this point the names/values are simply formatted into a one-line > string. That's not particularly readable, and it's not very useful for > the YAML/JSON formats I guess. So adding each modified GUC as an extra > text property would be better. > > 3) identifying modified (and interesting) GUCs > > 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. > > For now I've used > > /* return only options that were modified (not as in config file) */ > if ((conf->source <= PGC_S_ARGV) || (conf->source == PGC_S_OVERRIDE)) > continue; > > which generally does the right thing, although it also includes stuff > like application_name or client_encoding. But perhaps it'd be better to > whitelist the GUCs in some way, because some of the user-defined GUCs > may be sensitive and should not be included in plans. > > Opinions? > has sense Pavel > > regards > > -- > Tomas Vondra http://www.2ndQuadrant.com > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
explain plans with information about (modified) gucs
Hi, every now and then I have to investigate an execution plan that is strange in some way and I can't reproduce the same behavior. Usually it's simply due to data distribution changing since the problem was observed (say, after a nightly batch load/update). In many cases it however may be due to some local GUC tweaks, usually addressing some query specific issues (say, disabling nested loops or lowering join_collapse_limit). I've repeatedly ran into cases where the GUC was not properly reset to the "regular" value, and it's rather difficult to identify this is what's happening. Or cases with different per-user settings and connection pooling (SET SESSION AUTHORIZATION / ROLE etc.). So 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): test=# explain (gucs) select * from t; QUERY PLAN Seq Scan on t (cost=0.00..35.50 rows=2550 width=4) GUCs: application_name = 'x', client_encoding = 'UTF8', cpu_tuple_cost = '0.01' (2 rows) Of course, this directly applies to auto_explain too, which gets a new option log_gucs. The patch is quite trivial, but there are about three open questions: 1) names of the options I'm not particularly happy with calling the option "gucs" - it's an acronym and many users have little idea what GUC stands for. So I think a better name would be desirable, but I'm not sure what would that be. Options? Parameters? 2) format of output At this point the names/values are simply formatted into a one-line string. That's not particularly readable, and it's not very useful for the YAML/JSON formats I guess. So adding each modified GUC as an extra text property would be better. 3) identifying modified (and interesting) GUCs 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. For now I've used /* return only options that were modified (not as in config file) */ if ((conf->source <= PGC_S_ARGV) || (conf->source == PGC_S_OVERRIDE)) continue; which generally does the right thing, although it also includes stuff like application_name or client_encoding. But perhaps it'd be better to whitelist the GUCs in some way, because some of the user-defined GUCs may be sensitive and should not be included in plans. Opinions? regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 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, + _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'; + + + auto_explain.log_gucs (boolean) + + auto_explain.log_gucs configuration parameter + + + + + auto_explain.log_gucs 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. + + + + auto_explain.log_format (enum) diff --git