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?