From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Rafia Sabih <rafia.pghackers@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: John Naylor <john.naylor@2ndquadrant.com>
Cc: Sergei Agalakov <sergei.agalakov@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: explain plans with information about (modified) gucs
Date: Tue, 26 Mar 2019 21:04:53 +0100
Message-ID: <20190326200453.GA16551@development> (raw)
In-Reply-To: <CA+FpmFeBVwVyAxyMd9MX1jD8fUA3Qg96Po6sV+Po57egySKpGQ@mail.gmail.com>
References: <23164510-3f59-3289-3b32-9c8643711a6c@gmail.com>
<5396e093-ed6b-5962-2ed5-337a0425160b@2ndquadrant.com>
<CACPNZCsZ=VA-A+rMjxE4mm9iXcHEz1teMN=QYN8DsWmeHMOMqg@mail.gmail.com>
<f95ff490-672b-032c-6876-2e83661b93bb@2ndquadrant.com>
<CACPNZCuYOk1YtAK8CL5b0N76DV7E8ybmPgp=3Fd3HC3DXMFu4A@mail.gmail.com>
<603089e0-3387-30f1-3cbc-25dd7797e17a@2ndquadrant.com>
<20190204022209.GY1881@paquier.xyz>
<905460bc-db16-112f-1e2c-8dd48667f000@2ndquadrant.com>
<CA+FpmFeBVwVyAxyMd9MX1jD8fUA3Qg96Po6sV+Po57egySKpGQ@mail.gmail.com>
On Mon, Mar 18, 2019 at 11:31:48AM +0100, Rafia Sabih wrote:
>On Sun, 24 Feb 2019 at 00:06, Tomas Vondra <tomas.vondra@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).
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.cindex 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.cindex 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 *) guc_variables, num_guc_variables,
sizeof(struct config_generic *), guc_var_compare);
@@ -8819,6 +8837,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)
{
@@ -8826,7 +8849,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++)
{
@@ -8894,6 +8923,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;
Attachments:
[text/plain] explain-with-settings-20190326.patch (4.0K, ../20190326200453.GA16551@development/2-explain-with-settings-20190326.patch)
download | inline diff:diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.cindex 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.cindex 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 *) guc_variables, num_guc_variables,
sizeof(struct config_generic *), guc_var_compare);
@@ -8819,6 +8837,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)
{
@@ -8826,7 +8849,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++)
{
@@ -8894,6 +8923,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;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, rafia.pghackers@gmail.com, michael@paquier.xyz, john.naylor@2ndquadrant.com, sergei.agalakov@gmail.com
Subject: Re: explain plans with information about (modified) gucs
In-Reply-To: <20190326200453.GA16551@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox