From: | Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com> |
---|---|
To: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
Cc: | Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Partition-wise aggregation/grouping |
Date: | 2017-09-27 10:12:29 |
Message-ID: | CAM2+6=X_+U-ryfV-F-ShPwQwuU_dq2cSmUzWuBN5tjPmgXf4wg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(dot)bapat(at)enterprisedb(dot)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
Attachment | Content-Type | Size |
---|---|---|
partition-wise-agg-v4.tar.gz | application/x-gzip | 32.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Stas Kelvich | 2017-09-27 10:12:43 | Re: Transactions involving multiple postgres foreign servers |
Previous Message | Alvaro Herrera | 2017-09-27 10:05:59 | ALTER enums (was Re: [COMMITTERS] pgsql: doc: first draft of Postgres 10 release notes) |