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: 34d269d40809262353i150a6b6gf5d6ba48aa9b26d0@mail.gmail.com (view raw or flat)
Thread:
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:

*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

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

In response to

Responses

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-2014 The PostgreSQL Global Development Group