Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Mar 12, 2018 at 7:49 PM, Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>
>
> On Mon, Mar 12, 2018 at 6:07 PM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>>
>> On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat
>>
>> GroupPathExtraData now completely absorbs members from and replaces
>> OtherUpperPathExtraData. This means that we have to consider a way to
>> pass GroupPathExtraData to FDWs through GetForeignUpperPaths(). I
>> haven't tried it in this patch.
>
>
> Initially, I was passing OtherUpperPathExtraData to FDW. But now we need to
> pass GroupPathExtraData.
>
> However, since GetForeignUpperPaths() is a generic function for all upper
> relations, we might think of renaming this struct to UpperPathExtraData. Add
> an UpperRelationKind member to it
> Which will be used to distinguish the passed in extra data. But now we only
> have extra data for grouping only, I chose not to do that here. But someone,
> when needed, may choose this approach.

We don't need UpperRelationKind member in that structure. That will be
provided by the RelOptInfo passed.

The problem here is the extra information required for grouping is not
going to be the same for that needed for window aggregate and
certainly not for ordering. If we try to jam everything in the same
structure, it will become large with many members useless for a given
operation. A reader will not have an idea about which of them are
useful and which of them are not. So, instead we should try some
polymorphism. I think we can pass a void * to GetForeignUpperPaths()
and corresponding FDW hook knows what to cast it to based on the
UpperRelationKind passed.

BTW, the patch has added an argument to GetForeignUpperPaths() but has
not documented the change in API. If we go the route of polymorphism,
we will need to document the mapping between UpperRelationKind and the
type of structure passed in.

>
>>
>> With this patch there's a failure in partition_aggregation where the
>> patch is creating paths with MergeAppend with GatherMerge underneath.
>> I think this is related to the call
>> add_paths_to_partial_grouping_rel() when try_parallel_aggregation is
>> true. But I didn't investigate it further.
>
>
> I fixed it. We need to pass is_partial_agg instead of
> extra->partial_partitionwise_grouping while calling
> add_paths_to_partial_grouping_rel() in case of parallelism.

Thanks for investigation and the fix.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company