On Wed, Aug 27, 2008 at 8:54 PM, ITAGAKI Takahiro <[EMAIL PROTECTED]> 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
custom_guc_flags.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers