Re: Eager aggregation, take 3

On Wed Aug 6, 2025 at 4:52 AM -03, Richard Guo wrote:
> On Thu, Jul 24, 2025 at 12:21 PM Richard Guo <guofenglinux(at)gmail(dot)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.
>
I think it makes sense to me. I just wondering if we should follow an
"allow" or "don't-allow" strategy. I mean, instead of a list aggregate
functions that are not allowed we could list functions that are actually
allowed to use eager aggregation, so in this case we ensure that for the
functions that are enabled the eager aggregation can work properly.

> * 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.)
>
Having a GUC may sound like a good idea to me TBH. This threshold may
vary from workload to workload (?).

>
> 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?
>
It sounds like a good way to go for me, looking forward to the next
patch version to perform some other tests.

Thanks

--
Matheus Alcantara