On Thu, Mar 15, 2018 at 6:08 AM, Ashutosh Bapat <ashutosh.ba...@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. 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. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
