Re: explain plans with information about (modified) gucs

On Fri, 29 Mar 2019 at 22:07, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:

> On Wed, Mar 27, 2019 at 09:06:04AM +0100, Rafia Sabih wrote:
> >On Tue, 26 Mar 2019 at 21:04, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
> >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(dot)vondra(at)2ndquadrant(dot)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