On 18 March 2016 at 01:22, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Mar 17, 2016 at 10:35 AM, David Rowley
> <david.rowley@2ndquadrant.com> wrote:
>>
>> On 17 March 2016 at 01:19, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > Few assorted comments:
>> >
>> > 2.
>> > AggPath *
>> > create_agg_path(PlannerInfo *root,
>> > @@ -2397,9 +2399,11 @@ create_agg_path(PlannerInfo *root,
>> >
>> > List *groupClause,
>> > List *qual,
>> > const AggClauseCosts
>> > *aggcosts,
>> > - double numGroups)
>> > + double numGroups,
>> > +
>> > bool combineStates,
>> > + bool finalizeAggs)
>> >
>> > Don't you need to set parallel_aware flag in this function as we do for
>> > create_seqscan_path()?
>>
>> I don't really know the answer to that... I mean there's nothing
>> special done in nodeAgg.c if the node is running in a worker or in the
>> main process.
>>
>
> On again thinking about it, I think it is okay to set parallel_aware flag as
> false. This flag means whether that particular node has any parallelism
> behaviour which is true for seqscan, but I think not for partial aggregate
> node.
>
> Few other comments on latest patch:
>
> 1.
>
> + /*
> + * XXX does this need estimated for each partial path, or are they
> + * all
> going to be the same anyway?
> + */
> + dNumPartialGroups = get_number_of_groups(root,
> +
> clamp_row_est(partial_aggregate_path->rows),
> +
> rollup_lists,
> +
> rollup_groupclauses);
>
> For considering partial groups, do we need to rollup related lists?
No it doesn't you're right. I did mean to remove these, but they're
NIL anyway. Seems better to remove them to prevent confusion.
> 2.
> + hashaggtablesize = estimate_hashagg_tablesize(partial_aggregate_path,
> +
> &agg_costs,
> +
> dNumPartialGroups);
> +
> + /*
> + * Generate a
> hashagg Path, if we can, but we'll skip this if the hash
> + * table looks like it'll exceed work_mem.
> +
> */
> + if (can_hash && hashaggtablesize < work_mem * 1024L)
>
>
> hash table size should be estimated only if can_hash is true.
Good point. Changed.
>
> 3.
> + foreach(lc, grouped_rel->partial_pathlist)
> + {
> + Path *path =
> (Path *) lfirst(lc);
> + double total_groups;
> +
> + total_groups = path-
>>parallel_degree * path->rows;
> +
> + path = (Path *) create_gather_path(root, grouped_rel, path,
> NULL,
> + &total_groups);
>
> Do you need to perform it foreach partial path or just do it for
> firstpartial path?
That's true. The order here does not matter since we're passing
directly into a Gather node, so it's wasteful to consider anything
apart from the cheapest path. -- Fixed.
There was also a missing hash table size check on the Finalize
HashAggregate Path consideration. I've added that now.
Updated patch is attached. Thanks for the re-review.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services