Re: Fire create_upper_paths_hook for UPPERREL_PARTIAL_GROUP_AGG

On 8/12/26 17:32, Andrew Dunstan wrote:
>
> Greetings
>
> create_partial_grouping_paths() builds the UPPERREL_PARTIAL_GROUP_AGG
> upper relation.  It already calls the FDW callback GetForeignUpperPaths
> there, but never calls create_upper_paths_hook, the general-purpose
> hook that non-FDW extensions use to add paths. UPPERREL_PARTIAL_DISTINCT
> doesn't have this gap: create_partial_distinct_paths() calls both
> GetForeignUpperPaths and create_upper_paths_hook for it.
>
> This patch adds the missing create_upper_paths_hook call right next to
> the existing GetForeignUpperPaths call, so a non-FDW extension can add
> partial aggregation paths at the same point an FDW already can, before
> those paths are gathered and a Finalize Aggregate is built on top.
>
> It's a small, self-contained planner change with no effect on existing
> plans unless an extension registers create_upper_paths_hook and adds
> paths at this new call site.
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

Andrew,

If you use enable_partitionwise_aggregate then
create_partial_grouping_paths will run once for the parent and then
again for each child. This leads to the
upper_targets[UPPERREL_PARTIAL_GROUP_AGG] assignment being overwritten
per child, resulting in upper_targets holding the last child's
reltarget. This happens whenever partitionwise aggregation is
considered-- it doesn't have to win. So any extension reading the slot
after the grouping stage gets the wrong target. I reproduced this with a
test hook.

The hook already gets the target as output_rel->reltarget, so I think we
could just drop the assignment?

bg

--
Bryan Green
EDB: https://www.enterprisedb.com