Re: Aggregate Push Down - Performing aggregation on foreign server

Hi,

On Mon, Sep 12, 2016 at 5:17 PM, Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:

> Thanks Ashutosh for the detailed review comments.
>
> I am working on it and will post updated patch once I fix all your
> concerns.
>
>
>
Attached new patch fixing the review comments.

Here are few comments on the review points:

1. Renamed deparseFromClause() to deparseFromExpr() and
deparseAggOrderBy() to appendAggOrderBy()

2. Done

3. classifyConditions() assumes list expressions of type RestrictInfo. But
HAVING clause expressions (and JOIN conditions) are plain expressions. Do
you mean we should modify the classifyConditions()? If yes, then I think it
should be done as a separate (cleanup) patch.

4, 5. Both done.

6. Per my understanding, I think checking for just aggregate function is
enough as we are interested in whole aggregate result. Meanwhile I will
check whether we need to find and check shippability of transition,
combination and finalization functions or not.

7, 8, 9, 10, 11, 12. All done.

13. Fixed. However instead of adding new function made those changes inline.
Adding support for deparsing List expressions separating list by comma can
be
considered as cleanup patch as it will touch other code area not specific to
aggregate push down.

14, 15, 16, 17. All done.

18. I think re-factoring estimate_path_cost_size() should be done separately
as a cleanup patch too.

19, 20, 21, 22, 23, 24, 25, 26, 27, 28. All done.

29. I have tried passing input rel's relids to fetch_upper_rel() call in
create_grouping_paths(). It solved this patch's issue, but ended up with
few regression failures which is mostly plan changes. I am not sure whether
we get good plan now or not as I have not analyzed them.
So for this patch, I am setting relids in add_foreign_grouping_path()
itself.
However as suggested, used bms_copy(). I still kept the FIXME over there as
I think it should have done by the core itself.

30, 31, 32, 33. All done.

Let me know your views.

Thanks
--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company