Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Feb 27, 2018 at 4:29 AM, Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)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