Re: enable_material patch

On Sun, Apr 18, 2010 at 4:16 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Here's a patch to add enable_material, per previous discussion.  I
>> still think we should add enable_joinremoval also, but there wasn't a
>> clear consensus for that.
>
>> I'd appreciate it if someone could check this over for sanity - like,
>> did I get all the places where materialize nodes can be created?  and,
>> did i prevent them from being inserted anywhere that they are
>> necessary for correctness?
>
> I think the code is all right, but the comments (or to be more precise,
> the complete lack of attention to the comments) not so much.  Each of
> the places where you added an enable_material test has an associated
> comment that is reasonably thorough about explaining what's being
> checked and why.  Adding an unrelated test and not adjusting the comment
> to account for it is not acceptable IMO.
>
> Also, as a matter of style, I don't care for burying enable_ checks
> down inside a nest of unrelated if-conditions.  Rather than this:
>
>> -             else if (splan->parParam == NIL &&
>> +             else if (splan->parParam == NIL && enable_material &&
>>                                !ExecMaterializesOutput(nodeTag(plan)))
>>                       plan = materialize_finished_plan(plan);
>
> I'd suggest
>
>                else if (enable_material &&
>                         splan->parParam == NIL &&
>                         !ExecMaterializesOutput(nodeTag(plan)))
>
> and make sure that those tests line up with the order in which the
> conditions are explained in the associated comment.
>
> As far as "missed" changes go, the only place that I found where a
> material node can be created and you didn't touch it was in planner.c
> line 209.  It's correct to not add an enable_ check there because
> the node is required for correctness, but maybe it'd be worth saying
> so in the comment?  Otherwise somebody might "fix" it someday...
>
> Also, documentation-wise, I think this variable needs some weasel
> wording similar to what we have for enable_nestloop and enable_sort,
> ie point out that the variable cannot suppress all uses of
> materialization.
>
> If you fix that stuff I think this is OK to commit for 9.0.

Thanks for the review. Committed with revisions along the lines you suggest.

...Robert