Re: generic explain options v3

On Tue, Jul 21, 2009 at 10:29 PM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jul 21, 2009 at 10:05 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Tue, Jul 21, 2009 at 7:47 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Also, I'd suggest changing the ExplainStmt struct to have a list of
>>>> DefElem options instead of hard-wiring the option set at that level.
>>
>>> What is the advantage of that?
>>
>> Fewer places to change when you add a new option; in particular, not
>> copyfuncs or equalfuncs.  Also, the way you are doing it is gratuitously
>> unlike every other command that has similar issues to deal with.
>> Everybody else parses their DefElem list at execution time.  I think
>> you should have the legacy ANALYZE and VERBOSE syntax elements generate
>> DefElem list members that get examined at execution.
>
> Not having to touch copyfuncs or equalfuncs for future options is a
> definite plus, so I'll rework along these lines.

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

The natural place to do this would be in ExplainPrintPlan(), which is
already copying the relevant fields from the ExplainStmt over into an
ExplainState, but that's too far down the call tree, which (for a
non-utility statement when ExplainOneQuery_hook is null) looks like
this:

ExplainQuery -> ExplainOneQuery -> ExplainOnePlan -> ExplainPrintPlan

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(). And if we do that,
then probably we should declare it in include/nodes/execnodes.h and
make it a node type... and if we do that then we'll be back to a
copyfuncs/equalfuncs change every time we add a flag.

Now that's not to say there's no advantage in the proposed refactoring
- it's still more consistent with the way things are done elsewhere.
But since it's going to be a fair amount of work and fail to achieve
one of the two goals you set forth for it, I'd like to get
confirmation before proceeding if possible, and any suggestions you
may have for how to make it as clean as possible.

Thanks,

...Robert