Thanks Ashutosh for reviewing. Attached new patch-set with following changes:
1. Removed earlier 0007 and 0008 patches which were PoC for supporting partial aggregation over fdw. I removed them as it will be a different issue altogether and hence I will tackle them separately once this is done. This patch-set now includes support for parallel plans within partitions. Notes: HEAD: 59597e6 Partition-wise Join Version: 34 (First six patches 0001 - 0006, remains the same functionality-wise) 0007 - Refactors partial grouping paths creation into the separate function. 0008 - Enables parallelism within the partition-wise aggregation. This patch also includes a fix for the crash reported by Rajkumar. While forcibly applying scan/join target to all the Paths for the scan/join rel, earlier I was using apply_projection_to_path() which modifies the path in-place which causing this crash as the path finally chosen has been updated by partition-wise agg path creation. Now I have used create_projection_path() like we do in partial aggregation paths. Also, fixed issues reported by Ashutosh. On Tue, Sep 26, 2017 at 6:16 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > Hi Jeevan, > I have started reviewing these patches. > > 0001 looks fine. There might be some changes that will be needed, but > those will be clear when I review the patch that uses this > refactoring. > > 0002 > + * > + * If targetlist is provided, we use it else use targetlist from the root. > */ > static double > get_number_of_groups(PlannerInfo *root, > double path_rows, > - grouping_sets_data *gd) > + grouping_sets_data *gd, > + List *tlist) > { > Query *parse = root->parse; > double dNumGroups; > + List *targetList = (tlist == NIL) ? parse->targetList : tlist; > > May be we should just pass targetlist always. Instead of passing NIL, > pass parse->targetList directly. That would save us one conditional > assignment. May be passing NIL is required for the patches that use > this refactoring, but that's not clear as is in this patch. > Done in attached patch-set. > > 0003 > In the documenation of enable_partition_wise_aggregate, we should > probably explain why the default is off or like partition_wise_join > GUC, explain the consequences of turning it off. I have updated this. Please have a look. > I doubt if we could > accept something like partition_wise_agg_cost_factor looks. But we can > discuss this at a later stage. Mean time it may be worthwhile to fix > the reason why we would require this GUC. If the regular aggregation > has cost lesser than partition-wise aggregation in most of the cases, > then probably we need to fix the cost model. > Yep. I will have a look mean-while. > > I will continue reviewing rest of the patches. > > Thanks -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
partition-wise-agg-v4.tar.gz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers