> 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
From d59bd4e724c9693c31f45da1a806108fae03d18b Mon Sep 17 00:00:00 2001 From: Zhong ShiHao <[email protected]> Date: Sun, 6 Sep 2026 20:11:38 -0400 Subject: [PATCH] Test create_upper_paths_hook for UPPERREL_PARTIAL_GROUP_AGG Have test_extensible register create_upper_paths_hook and report when it is called for the partially grouped relation. Nothing in the tree registered this hook before, so none of its call sites had any coverage. --- src/test/modules/test_extensible/Makefile | 2 +- .../test_extensible/expected/partial_agg.out | 17 ++++++++++++++ src/test/modules/test_extensible/meson.build | 1 + .../test_extensible/sql/partial_agg.sql | 14 +++++++++++ .../modules/test_extensible/test_extensible.c | 23 +++++++++++++++++++ 5 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 src/test/modules/test_extensible/expected/partial_agg.out create mode 100644 src/test/modules/test_extensible/sql/partial_agg.sql diff --git a/src/test/modules/test_extensible/Makefile b/src/test/modules/test_extensible/Makefile index 4c696eda6c0..a0a65779ce9 100644 --- a/src/test/modules/test_extensible/Makefile +++ b/src/test/modules/test_extensible/Makefile @@ -9,7 +9,7 @@ PGFILEDESC = "test_extensible - test module for extensible node and custom scan" EXTENSION = test_extensible DATA = test_extensible--1.0.sql -REGRESS = test_extensible +REGRESS = test_extensible partial_agg ifdef USE_PGXS PG_CONFIG = pg_config diff --git a/src/test/modules/test_extensible/expected/partial_agg.out b/src/test/modules/test_extensible/expected/partial_agg.out new file mode 100644 index 00000000000..f7335f1bc8e --- /dev/null +++ b/src/test/modules/test_extensible/expected/partial_agg.out @@ -0,0 +1,17 @@ +-- create_upper_paths_hook must be fired for UPPERREL_PARTIAL_GROUP_AGG when +-- the planner considers partial aggregation. +LOAD 'test_extensible'; +CREATE TABLE test_extensible_agg_tbl (a int, b int); +INSERT INTO test_extensible_agg_tbl SELECT i % 3, i FROM generate_series(1, 10) i; +SET min_parallel_table_scan_size = 0; +SET max_parallel_workers_per_gather = 2; +SELECT a, sum(b) FROM test_extensible_agg_tbl GROUP BY a ORDER BY a; +NOTICE: create_upper_paths_hook: UPPERREL_PARTIAL_GROUP_AGG + a | sum +---+----- + 0 | 18 + 1 | 22 + 2 | 15 +(3 rows) + +DROP TABLE test_extensible_agg_tbl; diff --git a/src/test/modules/test_extensible/meson.build b/src/test/modules/test_extensible/meson.build index 54c571f30b6..a09852e8d67 100644 --- a/src/test/modules/test_extensible/meson.build +++ b/src/test/modules/test_extensible/meson.build @@ -28,6 +28,7 @@ tests += { 'regress': { 'sql': [ 'test_extensible', + 'partial_agg', ], }, } diff --git a/src/test/modules/test_extensible/sql/partial_agg.sql b/src/test/modules/test_extensible/sql/partial_agg.sql new file mode 100644 index 00000000000..ebe4e2fea2c --- /dev/null +++ b/src/test/modules/test_extensible/sql/partial_agg.sql @@ -0,0 +1,14 @@ +-- create_upper_paths_hook must be fired for UPPERREL_PARTIAL_GROUP_AGG when +-- the planner considers partial aggregation. + +LOAD 'test_extensible'; + +CREATE TABLE test_extensible_agg_tbl (a int, b int); +INSERT INTO test_extensible_agg_tbl SELECT i % 3, i FROM generate_series(1, 10) i; + +SET min_parallel_table_scan_size = 0; +SET max_parallel_workers_per_gather = 2; + +SELECT a, sum(b) FROM test_extensible_agg_tbl GROUP BY a ORDER BY a; + +DROP TABLE test_extensible_agg_tbl; diff --git a/src/test/modules/test_extensible/test_extensible.c b/src/test/modules/test_extensible/test_extensible.c index 2a1344f28d5..165a2a1af59 100644 --- a/src/test/modules/test_extensible/test_extensible.c +++ b/src/test/modules/test_extensible/test_extensible.c @@ -25,6 +25,7 @@ #include "nodes/readfuncs.h" #include "optimizer/pathnode.h" #include "optimizer/paths.h" +#include "optimizer/planner.h" #include "optimizer/restrictinfo.h" #include "utils/builtins.h" #include "utils/guc.h" @@ -422,6 +423,25 @@ test_set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, add_path(rel, (Path *) cpath); } +static create_upper_paths_hook_type prev_create_upper_paths_hook = NULL; + +/* + * Report the upper planning stages we are called for. Only partial + * aggregation is interesting here; the other stages have always fired the + * hook. + */ +static void +test_create_upper_paths(PlannerInfo *root, UpperRelationKind stage, + RelOptInfo *input_rel, RelOptInfo *output_rel, + void *extra) +{ + if (prev_create_upper_paths_hook) + prev_create_upper_paths_hook(root, stage, input_rel, output_rel, extra); + + if (stage == UPPERREL_PARTIAL_GROUP_AGG) + elog(NOTICE, "create_upper_paths_hook: UPPERREL_PARTIAL_GROUP_AGG"); +} + /* * test_get_extensible_node_methods * @@ -565,4 +585,7 @@ _PG_init(void) /* Install the path-list hook to inject CustomPaths for the test table */ prev_set_rel_pathlist_hook = set_rel_pathlist_hook; set_rel_pathlist_hook = test_set_rel_pathlist; + + prev_create_upper_paths_hook = create_upper_paths_hook; + create_upper_paths_hook = test_create_upper_paths; } -- 2.37.1 (Apple Git-137.1)
From 8187c721ccece56af8d2c75b0d8d3152e329fcc3 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan <[email protected]> Date: Sat, 9 May 2026 14:57:09 -0400 Subject: [PATCH v3] Fire create_upper_paths_hook for UPPERREL_PARTIAL_GROUP_AGG The planner already invites FDWs and extensions to add partial paths on UPPERREL_PARTIAL_DISTINCT (see create_partial_distinct_paths), but the symmetric create_upper_paths_hook call on UPPERREL_PARTIAL_GROUP_AGG is missing. Without it, an extension cannot register a partial-aggregate path through the standard hook surface: by the time its create_upper_paths_hook runs on UPPERREL_GROUP_AGG, the planner has already gathered partial paths from partially_grouped_rel and built Finalize Aggregate on top, so paths added late are dead code. Fire create_upper_paths_hook on partially_grouped_rel at the end of create_partial_grouping_paths, right next to the existing GetForeignUpperPaths call for UPPERREL_PARTIAL_GROUP_AGG. This mirrors create_partial_distinct_paths, which pairs its GetForeignUpperPaths and create_upper_paths_hook calls together at the end of that function for UPPERREL_PARTIAL_DISTINCT. Extensions adding partial paths here are picked up naturally by the caller's subsequent gather_grouping_paths call and become candidates for the upstream Finalize Aggregate built by add_paths_to_grouping_rel. The motivating use case is a CustomScan that wants to participate in partial aggregation -- e.g. a columnstore extension whose pushdown runs cheaper than nodeAgg's standard transition path. Without this hook, the extension is forced to choose between sequential agg pushdown (losing parallelism) or letting nodeAgg run unmodified above its parallel scan (losing the pushdown). Heap and other AMs are unaffected: this commit only adds a single create_upper_paths_hook call at a place where it wasn't being fired before. Reviewed-by: Bryan Green <[email protected]> --- src/backend/optimizer/plan/planner.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index f4689e7c9f8..9b15e6342dd 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -8016,6 +8016,19 @@ create_partial_grouping_paths(PlannerInfo *root, extra); } + /* + * Let extensions possibly add some more partial paths, mirroring the + * FDW call above and the analogous extension hook on + * UPPERREL_PARTIAL_DISTINCT in create_partial_distinct_paths. Paths + * added here are picked up by the caller's subsequent + * gather_grouping_paths call and become candidates for the upstream + * Finalize Aggregate that add_paths_to_grouping_rel builds. + */ + if (create_upper_paths_hook) + (*create_upper_paths_hook) (root, UPPERREL_PARTIAL_GROUP_AGG, + input_rel, partially_grouped_rel, + extra); + return partially_grouped_rel; } -- 2.43.0
