Re: Auto-explain patch

On Wed, Aug 27, 2008 at 8:54 PM, ITAGAKI Takahiro
 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:

*custom_guc_flags-0828.patch

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???

*export_explain.patch:

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

*psql_ignore_notices-0828.patch:

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.

*auto_explalin.c:

init_instrument()

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)
    foreach...
        init_instrument()

if (outerPlanState())
    foreach...

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
missing?

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