Re: generic explain options v3

On Sun, Jul 26, 2009 at 7:40 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> 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.
>
> Committed with a few editorializations.

Thanks.

>> - 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.
>
> I think the problem here is that you chose to treat ExplainState.pstmt
> as a parameter, when it's better considered as an internal field.
> I changed it to the latter approach.

Sounds fine.

>> - 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.
>
> I'm unconvinced.  The reason that code is that way is that the
> alternative would require explain.c to know quite a lot about prepared
> plans, which does not seem like an improvement.

I didn't consider that.  As it is, prepare.c has to know quite a lot
about explaining, so it may be six of one, half a dozen of the other.

>> - The hack needed in ExplainLogLevel is just that.
>
> Yeah, I thought that was okay.  We could alternatively refactor the
> code so that the parameter analysis code is a separate function that
> utility.c could call, but it's unclear that it's worth the trouble.

OK.

It seems I have quite a bit of work in front of me unbreaking the
machine-readable explain patch.  I started grinding through it, but
it's not pretty.  I'll post an updated version when I have it.

...Robert