Re: explain plans with information about (modified) gucs

From: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
To: Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, John Naylor <john(dot)naylor(at)2ndquadrant(dot)com>, Sergei Agalakov <sergei(dot)agalakov(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: explain plans with information about (modified) gucs
Date: 2019-03-29 21:07:17
Message-ID: 20190329210717.GD4719@development
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

regards

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

Attachment Content-Type Size
explain-with-settings-20190329.patch text/plain 30.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Banck 2019-03-29 21:08:30 Re: Online verification of checksums
Previous Message Andrew Dunstan 2019-03-29 21:06:35 Re: PostgreSQL pollutes the file system