Re: generic explain options v3

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.

> - 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.

> - 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.

> - 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.
        regards, tom lane