On Tue, Feb 27, 2018 at 4:29 AM, Jeevan Chalke <jeevan.cha...@enterprisedb.com> wrote: > Hi Robert, > I had a look over his provided testcase and observed that when we create a > Gather Merge path over a cheapest partial path by sorting it explicitly as > generate_gather_paths won't consider it, we accidentally used cheapest > partial path from the input_rel to create a Gather Merge; instead we need a > cheapest partial path from the partially_grouped_rel. > > Attached fix_aggref_in_non-agg_error.patch fixing this.
Oops. Thanks, committed. > test_for_aggref_in_non-agg_error.patch has a testcase reported by Rajkumar > which I have added in a aggregates.sql. Didn't commit this; I think that's overkill. > While doing so, I have observed few cleanup changes, added those in > misc_cleanup.patch. Committed those. > While re-basing my partitionwise aggregate changes, I observed that when we > want to create partial aggregation paths for a child partition, we don't > need to add Gather or Gather Merge on top of it as we first want to append > them all and then want to stick a gather on it. So it will be better to have > that code part in a separate function so that we can call it from required > places. > > I have attached patch (create_non_partial_paths.patch) for it including all > above fix. I don't like that very much. For one thing, the name create_non_partial_paths() is not very descriptive at all. For another thing, it somewhat renders add_paths_to_partial_grouping_rel() a misnomer, as that function then adds only partial paths. I think what you should just do is have the main patch add a test for rel->reloptkind == RELOPT_UPPER_REL; if true, add the Gather paths; if not, skip it. Then it will be skipped for RELOPT_OTHER_UPPER_REL which is what we want. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company