Re: parallel.c is not marked as test covered

On 12 May 2016 at 07:04, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, May 11, 2016 at 1:57 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
>> I don't immediately understand what's going wrong here. It looks to
>> me like make_group_input_target() already called, and that worked OK,
>> but now make_partialgroup_input_target() is failing using more-or-less
>> the same logic. Presumably that's because make_group_input_target()
>> was called on final_target as returned by create_pathtarget(root,
>> tlist), but make_partialgroup_input_target() is being called on
>> grouping_target, which I'm guessing came from
>> make_window_input_target, which somehow lacks sortgroupref labeling.
>> But I don't immediately see how that would happen, so there's
>> obviously something I'm missing here.
>
> So, it turns out you can reproduce this bug pretty easily without
> force_parallel_mode, like this:
>
> alter table int4_tbl set (parallel_degree = 4);
> SELECT SUM(COUNT(f1)) OVER () FROM int4_tbl WHERE f1=42;
>
> Or you can just a query that involves a window function on a table
> large enough for parallelism to be considered:
>
> SELECT SUM(COUNT(aid)) OVER () FROM pgbench_accounts;
>
> The crash goes way if the target list involves at least one plain
> column that uses no aggregate or window function, because then the
> PathTarget list has a sortgrouprefs array. Trivial fix patch
> attached, although some more review from you (Tom Lane, PathTarget
> inventor and planner whiz) and David Rowley (author of this function)
> would be appreciated in case there are deeper issues here.

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_col list. Since your
query has no GROUP BY it means that add_column_to_pathtarget() is never
called with a non-zero sortgroupref.

It looks like Tom has intended that PathTarget->sortgrouprefs can be NULL
going by both the comment /* corresponding sort/group refnos, or 0 */, and
the coding inside add_column_to_pathtarget(), which does not allocate the
array if given a 0 sortgroupref.

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's left filled
with zeros.

It might be better if this was all consistent. Perhaps it would be worth
modifying make_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 fix
that's in your patch. This saves an allocation which is likely much more
expensive than the NULL check later. Alternatively
add_column_to_pathtarget() could be modified to allocate the array even if
sortgroupref is zero.

I think consistency is good here, as if this had been consistent this would
not be a bug.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services