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

On 2026-08-26 We 2:44 PM, Bryan Green wrote:

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.

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?

Quite right. That was left over from an earlier draft, and is clearly an
error, thanks for catching it. Here's an updated patch.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested

Patch looks good. Just a note: create_upper_paths_hook has no test coverage anywhere in the tree, at any call site — a pre-existing gap, not something to fix here. Could be a separate follow-up.
--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

create_upper_paths_hook has no test coverage anywhere in the tree

I agree this patch needs test coverage, so I have attached 0002. It
reuses test_extensible, which already installs a planner hook: a new
GUC makes it add a partial aggregate path from create_upper_paths_hook,
and the test checks that the planner gathers it and builds Finalize
Aggregate on top. It fails without 0001.

Nit on the code comment: the other five call sites use a single line,
so maybe just "/* Let extensions possibly add some more partial paths
*/" and move the rest to the commit message. "Heap and other AMs are
unaffected" also looks like a leftover from v2.

I will leave it to the author to decide whether to take these.
Thanks,
Shihao

shihao zhong <zhong950419@gmail.com> writes:

create_upper_paths_hook has no test coverage anywhere in the tree

I agree this patch needs test coverage, so I have attached 0002. It
reuses test_extensible, which already installs a planner hook: a new
GUC makes it add a partial aggregate path from create_upper_paths_hook,
and the test checks that the planner gathers it and builds Finalize
Aggregate on top. It fails without 0001.

What does this test, other than the test module itself? The hook
call in itself has no interesting behavior, and I don't think that
we need to spin up a whole database session just to pass through
that code line.

In general, I think people have become far too enamored of tests
for the sake of tests, and are not thinking about the downstream
costs in CI and buildfarm cycles, developer time spent waiting for
tests to run, and for that matter the nonzero ongoing maintenance
effort that every test script has. Sure, those costs are tiny
on a one-time basis, but they add up fast.

It's a good idea to write something like this to convince yourself
that the hook is usable for something useful, but that doesn't mean
we need to carry that proof in-tree forevermore.

regards, tom lane

It's a good idea to write something like this to convince yourself
that the hook is usable for something useful, but that doesn't mean
we need to carry that proof in-tree forevermore.

You're right. The test only checks that the hook gets called. It
doesn't check that anything behaves differently, and that isn't worth
spinning up a session for. Writing it did convince me the hook is
usable, like you say, but that doesn't mean it has to stay in the tree.
Dropping 0002.

I should also correct what I wrote about it. That description was from
an different version which added a path and checked that the planner
gathered it and built Finalize Aggregate on top. Even that is existing
planner behavior, not something 0001 adds.

Review is otherwise unchanged. I only have the Nit commit message and
comment suggestions from my earlier mail.

Given there is no major feedback open, I change the patch to Ready for
Committer.

Thanks,
Shihao