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
