Re: generic explain options v3

On Thu, Jul 23, 2009 at 2:23 PM, Robert Haas<robertmhaas@gmail.com> wrote:
> On Thu, Jul 23, 2009 at 12:08 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> Ugh.  I took a look at this and it turns out that there are some
>>> tentacles.  It doesn't seem very sane to actually do anything with a
>>> list of DefElem nodes, so we really need to parse that list and
>>> convert it to a more sensible format right away (this also seems
>>> important for proper error checking).
>>
>> Yeah, the standard approach is to convert it into a group of values
>> at the start of execution of the utility command.
>>
>>> The obvious solution to that is to create the ExplainState sooner,
>>> back up at the ExplainQuery level.  If we do that, though, then
>>> ExplainState will need to become a public API, because
>>> contrib/auto_explain calls ExplainPrintPlan().
>>
>> Well, if we add any more options to EXPLAIN then auto_explain may well
>> be interested in them, so I'm not sure this is bad.  The alternative
>> is to keep adding retail parameters to the public functions.
>>
>>> And if we do that,
>>> then probably we should declare it in include/nodes/execnodes.h and
>>> make it a node type...
>>
>> No, just a struct declared in commands/explain.h.  There's no reason
>> for it to be part of the Node system.
>
> Oh, OK.  That will work.  Thanks.

Here's the update.  There are a few things that I'm not entirely happy
with here, but not quite sure what to do about either.

- ExplainPrintPlan() is now almost trivial.  It seems like there
should be some way to get rid of this altogether, but I'm not quite
sure how.  I thought about ripping pstmt and rtable out of
ExplainState and just storying queryDesc there.  But that involves
changing a lot of code, and while it makes some things simpler, it
makes other parts more complex.  I'm not sure whether it's a win or
not; I'm also not sure how much brainpower it's worth spending on
this.

- It's becoming increasingly evident to me that the explain stuff in
prepare.c has no business being there and should be moved to
explain.c.  I haven't done that here, but it's worth thinking about.
We could turn several functions that are currently public into statics
if we did that.

- The hack needed in ExplainLogLevel is just that.

Help!

...Robert