fix: charge the grouped aggregate path for the folding it does (#349) - #350
Conversation
ColumnarTryGroupAggPath priced itself at the cheapest non-index scan plus cpu_tuple_cost, and charged nothing for the folding it performs. Every competing plan pays a per-row aggregation cost; this one paid none, so it won by construction against anything -- including a parallel plan measured 1.9x faster than itself. Measured on the 100M-row TSBS bench, GROUP BY hostname over the full scan with 4000 groups: enabling the path took the query from 5,953 ms to 11,478 ms, because it replaced a four-worker Finalize GroupAggregate with a single serial node. Confirmed over three alternating runs (6134/5972/5985 against 11473/11616/11445) with near-identical buffers, so the cost is CPU, not I/O. The node is serial, so it cannot be priced low and left to win; it has to compete honestly. It folds vectors rather than advancing a row-wise Agg, which is cheaper per row, but it does that on every row without dividing the work across workers. Charging cpu_operator_cost per input row per aggregate, the same rate core charges a transition, expresses exactly that, and is conservative because the fold is cheaper than the advance it replaces. Against the planner's own estimates on that fixture, per row per aggregate: any charge above ~0.0013 correctly loses the full-scan case and any below ~0.0107 correctly wins the windowed ones. The default cpu_operator_cost of 0.0025 sits inside with margin at both ends, so this is not tuned to the fixture. Charging per input row rather than per output group also avoids the failure the previous comment records, where an earlier per-group charge let autoanalyze's n_distinct estimate flip the choice on large inputs so the node sometimes did not run at all. Per input row does not depend on n_distinct. It also makes the estimate respond to the number of aggregates, which it did not: ten aggregates were priced identically to one while costing 2.35x as much to run. Verified on the bench, plan choice and runtime together: shape chosen before after 1 metric, 12h window vectorized serial 3,604 ms 3,175 ms 10 metrics, 12h window vectorized serial 8,474 ms 6,842 ms full scan, 4000 groups core parallel 11,478 ms 5,380 ms The regression is gone and both windowed shapes keep their wins. test/native_groupagg.sh gains the property that was observably wrong and that does not depend on the planner choosing any particular plan at fixture scale: the estimate must respond to how much folding the node will do. Removal proof: with the charge reverted, one aggregate and ten are both priced at 20601 and the check fails; with it, 25601 against 70601. This does not make the node parallel-aware, which remains the larger win -- it stops the node from winning plans it should lose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011miCFRSatixeNRw3w5yNq8
|
Reviewed — correct and mergeable as the interim cost fix (parallel-awareness still the larger win, as you note). The costing is right:
I checked whether the ungrouped node I shipped (#337/#343) has the same blind spot, since it's the same "priced at the scan, charges nothing for folding" shape. It does not regress in a realistic config: with parallelism available and No objections. |
Addresses the regression measured in #349. Does not close it -- making the grouped node parallel-aware remains the larger win; this stops the node winning plans it should lose.
The defect
ColumnarTryGroupAggPathpriced itself at the cheapest non-index scan pluscpu_tuple_cost, and charged nothing for the folding it performs. Every competing plan pays a per-row aggregation cost; this one paid none, so it won by construction against anything -- including a parallel plan measured 1.9x faster than itself.The existing comment states the intent outright: "a negligible fixed bump keeps this reliably below the ordinary Agg-over-scan plan whenever the feature is enabled... when it is on it should be the plan, not a coin-flip." That is defensible for an accelerator that is always faster. Measurement says it is not always faster.
Measured, 100M-row TSBS bench
GROUP BY hostnameover the full scan, 4,000 groups: enabling the path took the query from 5,953 ms to 11,478 ms, because it replaced a four-workerFinalize GroupAggregatewith one serial node. Three alternating runs, near-identical buffers (38,912 vs 39,426), so the cost is CPU not I/O:The fix
Charge
cpu_operator_cost * scan_rows * naggs-- the same rate core charges a transition, and conservative, since folding is cheaper per row than the row-wise advance it replaces.Against the planner's own estimates on that fixture, per row per aggregate: any charge above ~0.0013 correctly loses the full-scan case, any below ~0.0107 correctly wins the windowed ones. The default 0.0025 sits inside with margin at both ends, so this is not tuned to the fixture.
Charging per input row rather than per output group also avoids the failure the old comment records -- an earlier per-group charge let autoanalyze's
n_distinctflip the choice so the node sometimes did not run at all. Input rows do not depend onn_distinct.It also makes the estimate respond to aggregate count, which it did not: ten aggregates were priced identically to one while costing 2.35x as much to run.
Result
Regression gone; both windowed shapes keep their wins.
Test
test/native_groupagg.shgains the property that was observably wrong and that does not depend on the planner choosing any particular plan at fixture scale: the estimate must respond to how much folding the node will do.Removal proof: with the charge reverted, one aggregate and ten are priced identically at 20601 and the check fails by name. With it: 25601 against 70601.
Gate
Full 15-19 matrix,
ALL VERSIONS PASSED,native_groupagg5/5. Zero build warnings on all five majors.A correction I owe the record
I posted analysis on #349 arguing that honest costing could not work -- that our cost is roughly the serial scan and core's roughly serial/4, so pricing honestly would fix the full-scan case and lose the windowed ones. That reasoning was wrong: it ignored that core's plan also pays per-row aggregation, dominated by a Sort on the windowed shapes. The planner's actual numbers show a 3.17x margin on the windowed case against only 1.37x on the full-scan one -- exactly the room needed to separate them. I will correct that comment.
🤖 Generated with Claude Code
https://claude.ai/code/session_011miCFRSatixeNRw3w5yNq8