On Sun, Apr 18, 2010 at 4:16 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers