Re: parallel.c is not marked as test covered

<div dir="ltr">On 12 May 2016 at 07:04, Robert Haas <<a
href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>>wrote:<br />> On Wed, May 11, 2016 at 1:57 PM,
RobertHaas <<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>> wrote:<br />>> I don't
immediatelyunderstand what's going wrong here.  It looks to<br />>> me like make_group_input_target() already
called,and that worked OK,<br />>> but now make_partialgroup_input_target() is failing using more-or-less<br
/>>>the same logic.  Presumably that's because make_group_input_target()<br />>> was called on final_target
asreturned by create_pathtarget(root,<br />>> tlist), but make_partialgroup_input_target() is being called on<br
/>>>grouping_target, which I'm guessing came from<br />>> make_window_input_target, which somehow lacks
sortgroupreflabeling.<br />>> But I don't immediately see how that would happen, so there's<br />>>
obviouslysomething I'm missing here.<br />><br />> So, it turns out you can reproduce this bug pretty easily
without<br/>> force_parallel_mode, like this:<br />><br />> alter table int4_tbl set (parallel_degree = 4);<br
/>>SELECT SUM(COUNT(f1)) OVER () FROM int4_tbl WHERE f1=42;<br />><br />> Or you can just a query that
involvesa window function on a table<br />> large enough for parallelism to be considered:<br />><br />>
SELECTSUM(COUNT(aid)) OVER () FROM pgbench_accounts;<br />><br />> The crash goes way if the target list involves
atleast one plain<br />> column that uses no aggregate or window function, because then the<br />> PathTarget
listhas a sortgrouprefs array.  Trivial fix patch<br />> attached, although some more review from you (Tom Lane,
PathTarget<br/>> inventor and planner whiz) and David Rowley (author of this function)<br />> would be
appreciatedin case there are deeper issues here.<br /><br />The problem is make_group_input_target() only calls
add_column_to_pathtarget()(which allocates this array) when there's a GROUP BY, otherwise it just appends to the
non_group_collist. Since your query has no GROUP BY it means that add_column_to_pathtarget() is never called with a
non-zerosortgroupref.<br /><br />It looks like Tom has intended that PathTarget->sortgrouprefs can be NULL going by
boththe comment /* corresponding sort/group refnos, or 0 */, and the coding inside add_column_to_pathtarget(), which
doesnot allocate the array if given a 0 sortgroupref.<br /><br />It looks like make_sort_input_target(),
make_window_input_target()and make_group_input_target() all get away without this check because they're all using
final_target,which was built by make_pathtarget_from_tlist() which *always* allocates the sortgrouprefs array, even if
it'sleft filled with zeros.<br /><br />It might be better if this was all consistent. Perhaps it would be worth
modifyingmake_pathtarget_from_tlist() to only allocate the sortgrouprefs array if there's any non-zero
tle->ressortgroupref,then modify the other make_*_input_target() functions to handle a NULL array, similar to the
fixthat's in your patch. This saves an allocation which is likely much more expensive than the NULL check later.
Alternativelyadd_column_to_pathtarget() could be modified to allocate the array even if sortgroupref is zero.<br /><br
/>Ithink consistency is good here, as if this had been consistent this would not be a bug.<br /><br />-- <br /> David
Rowley                  <a href="http://www.2ndQuadrant.com/">http://www.2ndQuadrant.com/</a><br /> PostgreSQL
Development,24x7 Support, Training & Services</div>