Re: Eager aggregation, take 3
Richard Guo <guofenglinux@gmail.com>
On Thu, Jul 24, 2025 at 12:21 PM Richard Guo <guofenglinux@gmail.com> wrote: > This patch no longer applies; here's a rebased version. Nothing > essential has changed. Based on some off-list testing by Matheus (CC'ed), several TPC-DS queries that used to apply eager aggregation no longer do, which suggests that the v18 patch is too strict about when eager aggregation can be used. I looked into query 4 and query 11, and found two reasons why they no longer apply eager aggregation with v18. * The has_internal_aggtranstype() check. To avoid potential memory blowout risks from large partial aggregation values, v18 avoids applying eager aggregation if any aggregate uses an INTERNAL transition type, as this typically indicates a large internal data structure (as in string_agg or array_agg). However, this also excludes aggregates like avg(numeric) and sum(numeric), which are actually safe to use with eager aggregation. What we really want to exclude are aggregate functions that can produce large transition values by accumulating or concatenating input rows. So I'm wondering if we could instead check the transfn_oid directly and explicitly exclude only F_ARRAY_AGG_TRANSFN and F_STRING_AGG_TRANSFN. We don't need to worry about json_agg, jsonb_agg, or xmlagg, since they don't support partial aggregation anyway. * The EAGER_AGG_MIN_GROUP_SIZE threshold This threshold defines the minimum average group size required to consider applying eager aggregation. It was previously set to 2, but in v18 it was increased to 20 to be cautious about planning overhead. This change was a snap decision though, without any profiling or data to back it. Looking at TPC-DS queries 4 and 11, a threshold of 10 is the minimum needed to consider eager aggregation for them. The resulting plans show nice performance improvements without any measurable increase in planning time. So, I'm inclined to lower the threshold to 10 for now. (Wondering whether we should make this threshold a GUC, so users can adjust it based on their needs.) With these two changes, here are the planning and execution time for queries 4 and 11 (scale factor 1) on my snail-paced machine, with and without eager aggregation. query 4: -- without eager aggregation Planning Time: 6.765 ms Execution Time: 34941.713 ms -- with eager aggregation Planning Time: 6.674 ms Execution Time: 13994.183 ms query 11: -- without eager aggregation Planning Time: 3.757 ms Execution Time: 20888.076 ms -- with eager aggregation Planning Time: 3.747 ms Execution Time: 7449.522 ms Any comments on these two changes? Thanks Richard
Commits
-
Fix eager aggregation for semi/antijoin inner rels
- ffeda04259bb 19 (unreleased) landed
-
Cover additional errors and corner conditions in repack.c
- 2670cc298f42 19 (unreleased) cited
-
Fix volatile function evaluation in eager aggregation
- 3a08a2a8b4fd 19 (unreleased) landed
-
Fix collation handling for grouping keys in eager aggregation
- bd94845e8c90 19 (unreleased) landed
-
Rename apply_at to apply_agg_at for clarity
- 1206df04c200 19 (unreleased) landed
-
Fix comment in eager_aggregate.sql
- 36fd8bde1b77 19 (unreleased) landed
-
Remove unnecessary include of "utils/fmgroids.h"
- f997d777adf7 19 (unreleased) landed
-
Implement Eager Aggregation
- 8e11859102f9 19 (unreleased) landed
-
Allow negative aggtransspace to indicate unbounded state size
- 185e30426334 19 (unreleased) landed
-
Add macros for looping through a List without a ListCell.
- 14dd0f27d7cd 17.0 cited
-
Account for the effect of lossy pages when costing bitmap scans.
- 5edc63bda68a 11.0 cited
-
Fix a thinko in join_is_legal: when we decide we can implement a semijoin
- a43b190e3c71 9.0.0 cited