Prev: [PATCH] fix segfault with DO and plperl/plperlu
Next: RPM script bug #5430
From: Tom Lane on 18 Apr 2010 16:16
Robert Haas <robertmhaas(a)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.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 18 Apr 2010 20:55
On Sun, Apr 18, 2010 at 4:16 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas(a)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(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 | 
Pages: 1
Prev: [PATCH] fix segfault with DO and plperl/plperlu
Next: RPM script bug #5430