Re: enable_material patch

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.

regards, tom lane