On Thu, Mar 15, 2018 at 7:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 15, 2018 at 6:08 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> In current create_grouping_paths() (without any of your patches
>> applied) we first create partial paths in partially grouped rel and
>> then add parallel path to grouped rel using those partial paths. Then
>> we hand over this to FDW and extension hooks, which may add partial
>> paths, which might throw away a partial path used to create a parallel
>> path in grouped rel causing a segfault. I think this bug exists since
>> we introduced parallel aggregation or upper relation refactoring
>> whichever happened later. Introduction of partially grouped rel has
>> just made it visible.
>
> I don't think there's really a problem here; or at least if there is,
> I'm not seeing it. If an FDW wants to introduce partially grouped
> paths, it should do so when it is called for
> UPPERREL_PARTIAL_GROUP_AGG from within
> add_paths_to_partial_grouping_rel. If it wants to introduce fully
> grouped paths, it should do so when it is called for
> UPPERREL_GROUP_AGG from within create_grouping_paths. By the time the
> latter call is made, it's too late to add partially grouped paths; if
> the FDW does, that's a bug in the FDW.
Right.
>
> Admittedly, this means that commit
> 3bf05e096b9f8375e640c5d7996aa57efd7f240c subtly changed the API
> contract for FDWs. Before that, an FDW that wanted to support partial
> aggregation would have needed to add partially grouped paths to
> UPPERREL_GROUP_AGG when called for that relation; whereas now it would
> need to add them to UPPERREL_PARTIAL_GROUP_AGG when called for that
> relation.
And when an FDW added partial paths to the grouped rel
(UPPERREL_GROUP_AGG) it might throw away the partial paths gathered by
the core code. 3bf05e096b9f8375e640c5d7996aa57efd7f240c has fixed that
bug by allowing FDW to add partial paths to UPPERREL_PARTIAL_GROUP_AGG
before adding gather paths to the UPPERREL_GROUP_AGG. But I agree that
it has subtly changed that API and we need to add this to the
documentation (may be we should have added that in the commit which
introduced partially grouped relation).
> This doesn't actually falsify any documentation, though,
> because this oddity wasn't documented before. Possibly more
> documentation could stand to be written in this area, but that's not
> the topic of this thread.
+1.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
