On Wed, Aug 27, 2008 at 8:54 PM, ITAGAKI Takahiro
<itagaki.takahiro@oss.ntt.co.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:
*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
