Skip site navigation (1) Skip section navigation (2)

Re: Auto-explain patch

From: "Alex Hunsaker" <badalex(at)gmail(dot)com>
To: "ITAGAKI Takahiro" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
Cc: pgsql-hackers(at)postgresql(dot)org, "Dean Rasheed" <dean_rasheed(at)hotmail(dot)com>, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Simon Riggs" <simon(at)2ndquadrant(dot)com>
Subject: Re: Auto-explain patch
Date: 2008-09-27 06:53:27
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
On Wed, Aug 27, 2008 at 8:54 PM, ITAGAKI Takahiro
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Here is a contrib version of auto-explain.
> I'd like to add it the next commit-fest in September.

Here is my review:


It seems to be windows newline damaged? lots of ^M at the end of the
lines, could not get it to apply cleanly.  New patch attached.

My only other concern is the changes to DefineCustom*() to tag the new
flags param.  Now I think anyone who uses Custom gucs will want/should
be able to set that. I did not see any people in contrib using it but
did not look on PGfoundry.  Do we need to document the change
somewhere for people who might be using it???


This looks much better than the old exporting of ExplainState way and
fixes tom's complaint about exporting ExplainState, so looks good to


This is not needed anymore because we log to LOG. (as you pointed out)
Tom also voiced some concern about this possibly breaking (or broken) clients.

I set my client_min_messages to notice and tried tab completing common
things I do.. I did not see any NOTICES, so unless we have an annoying
message someone knows about (and maybe it should not be a notice
then).  I think this should get dropped.



Hrm this strikes me as kind of kludgy...  Any thoughts on the 4 cases
in the switch getting out of sync (T_AppendState, T_BitmapAndState,
T_BitmapOrState, T_SubqueryScanState)?  The only "cleaner" way I can
see is to add a hook for CreateQueryDesc so we can overload
doInstrument and ExecInitNode will InstrAlloc them all for us.
I dunno Suggestions??

use the {outer|inner}PlanState macros instead of ->lefttree, ->righttree

can probably save a few cycles by wrapping those in a
if (outerPlanNode(planstate))

A quick check shows they can be null, but maybe they never can be when
they get to this point... So maybe thats a mute point.

If this sticks around I would suggest restructuring it to better match
explain_outNode so its easier to match up
something like...

if (planstate->initPlan)

if (outerPlanState())

if (innerPlanState())

the only other comment I have is suset_assign() do we really need to
be a superuser if its all going to LOG ? There was some concern about
explaining security definer functions right? but surely a regular
explain on those shows the same thing as this explain?  Or what am I

and obviously your self noted comment that README.txt should be sgml

Attachment: custom_guc_flags.patch
Description: application/octet-stream (6.3 KB)

In response to


pgsql-hackers by date

Next:From: KaiGai KoheiDate: 2008-09-27 09:59:53
Subject: Re: Updates of SE-PostgreSQL 8.4devel patches
Previous:From: KaiGai KoheiDate: 2008-09-27 03:18:45
Subject: Re: Updates of SE-PostgreSQL 8.4devel patches

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group