On Tue, 26 Mar 2019 at 21:04, Tomas Vondra <[email protected]>
wrote:
> On Mon, Mar 18, 2019 at 11:31:48AM +0100, Rafia Sabih wrote:
> >On Sun, 24 Feb 2019 at 00:06, Tomas Vondra <[email protected]>
> 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);
}
/*
