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