On Sun, Apr 18, 2010 at 4:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.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
