Subject: Eager aggregation, take 3

From: Richard Guo <[email-masked]>
Date: Mon, 4 Mar 2024 16:27:24 +0800

Hi All,

Eager aggregation is a query optimization technique that partially
pushes a group-by past a join, and finalizes it once all the relations
are joined.  Eager aggregation reduces the number of input rows to the
join and thus may result in a better overall plan.  This technique is
thoroughly described in the 'Eager Aggregation and Lazy Aggregation'
paper [1].

Back in 2017, a patch set has been proposed by Antonin Houska to
implement eager aggregation in thread [2].  However, it was at last
withdrawn after entering the pattern of "please rebase thx" followed by
rebasing and getting no feedback until "please rebase again thx".  A
second attempt in 2022 unfortunately fell into the same pattern about
one year ago and was eventually closed again [3].

That patch set has included most of the necessary concepts to implement
eager aggregation.  However, as far as I can see, it has several weak
points that we need to address.  It introduces invasive changes to some
core planner functions, such as make_join_rel().  And with such changes
join_is_legal() would be performed three times for the same proposed
join, which is not great.  Another weak point is that the complexity of
join searching dramatically increases with the growing number of
relations to be joined.  This occurs because when we generate partially
aggregated paths, each path of the input relation is considered as an
input path for the grouped paths.  As a result, the number of grouped
paths we generate increases exponentially, leading to a significant
explosion in computational complexity.  Other weak points include the
lack of support for outer joins and partitionwise joins.  And during my
review of the code, I came across several bugs (planning error or crash)
that need to be addressed.

I'd like to give it another take to implement eager aggregation, while
borrowing lots of concepts and many chunks of codes from the previous
patch set.  Please see attached.  I have chosen to use the term 'Eager
Aggregation' from the paper [1] instead of 'Aggregation push-down', to
differentiate the aggregation push-down technique in FDW.

The patch has been split into small pieces to make the review easier.

0001 introduces the RelInfoList structure, which encapsulates both a
list and a hash table, so that we can leverage the hash table for faster
lookups not only for join relations but also for upper relations.  With
eager aggregation, it is possible that we generate so many upper rels of
type UPPERREL_PARTIAL_GROUP_AGG that a hash table can help a lot with
lookups.

0002 introduces the RelAggInfo structure to store information needed to
create grouped paths for base and join rels.  It also revises the
RelInfoList related structures and functions so that they can be used
with RelAggInfos.

0003 checks if eager aggregation is applicable, and if so, collects
suitable aggregate expressions and grouping expressions in the query,
and records them in root->agg_clause_list and root->group_expr_list
respectively.

0004 implements the functions that check if eager aggregation is
applicable for a given relation, and if so, create RelAggInfo structure
for the relation, using the infos about aggregate expressions and
grouping expressions we collected earlier.  In this patch, when we check
if a target expression can act as grouping expression, we need to check
if this expression can be known equal to other expressions due to ECs
that can act as grouping expressions.  This patch leverages function
exprs_known_equal() to achieve that, after enhancing this function to
consider opfamily if provided.

0005 implements the functions that generate paths for grouped relations
by adding sorted and hashed partial aggregation paths on top of paths of
the plain base or join relations.  For sorted partial aggregation paths,
we only consider any suitably-sorted input paths as well as sorting the
cheapest-total path.  For hashed partial aggregation paths, we only
consider the cheapest-total path as input.  By not considering other
paths we can reduce the number of grouping paths as much as possible
while still achieving reasonable results.

0006 builds grouped relations for each base relation if possible, and
generates aggregation paths for the grouped base relations.

0007 builds grouped relations for each just-processed join relation if
possible, and generates aggregation paths for the grouped join
relations.  The changes made to make_join_rel() are relatively minor,
with the addition of a new function make_grouped_join_rel(), which finds
or creates a grouped relation for the just-processed joinrel, and
generates grouped paths by joining a grouped input relation with a
non-grouped input relation.

The other way to generate grouped paths is by adding sorted and hashed
partial aggregation paths on top of paths of the joinrel.  This occurs
in standard_join_search(), after we've run set_cheapest() for the
joinrel.  The reason for performing this step after set_cheapest() is
that we need to know the joinrel's cheapest paths (see 0005).

This patch also makes the grouped relation for the topmost join rel act
as the upper rel representing the result of partial aggregation, so that
we can add the final aggregation on top of that.  Additionally, this
patch extends the functionality of eager aggregation to work with
partitionwise join and geqo.

This patch also makes eager aggregation work with outer joins.  With
outer join, the aggregate cannot be pushed down if any column referenced
by grouping expressions or aggregate functions is nullable by an outer
join above the relation to which we want to apply the partiall
aggregation.  Thanks to Tom's outer-join-aware-Var infrastructure, we
can easily identify such situations and subsequently refrain from
pushing down the aggregates.

Starting from this patch, you should be able to see plans with eager
aggregation.

0008 adds test cases for eager aggregation.

0009 adds a section in README that describes this feature (copied from
previous patch set, with minor tweaks).

Thoughts and comments are welcome.

[1] https://www.vldb.org/conf/1995/P345.PDF
[2] https://www.postgresql.org/message-id/flat/9666.1491295317%40localhost
[3]
https://www.postgresql.org/message-id/flat/OS3PR01MB66609589B896FBDE190209F495EE9%40OS3PR01MB6660.jpnprd01.prod.outlook.com

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Fri, 13 Jun 2025 16:41:57 +0900

I've switched back to this thread and will begin by working through
the key concerns that were previously raised.

The first concern is the lack of a proof demonstrating the correctness
of this transformation.  To address this, I plan to include a detailed
proof in the README, along the lines of the following.

====== proof start ======
To prove that the transformation is correct, we partition the tables
in the FROM clause into two groups: those that contain at least one
aggregation column, and those that do not contain any aggregation
columns.  Each group can be treated as a single relation formed by the
Cartesian product of the tables within that group.  Therefore, without
loss of generality, we can assume that the FROM clause contains
exactly two relations, R1 and R2, where R1 represents the relation
containing all aggregation columns, and R2 represents the relation
without any aggregation columns.

Let the query be of the form:

SELECT G, AGG(A)
FROM R1 JOIN R2 ON J
GROUP BY G;

where G is the set of grouping keys that may include columns from R1
and/or R2; AGG(A) is an aggregate function over columns A from R1; J
is the join condition between R1 and R2.

The transformation of eager aggregation is:

    GROUP BY G, AGG(A) on (R1 JOIN R2 ON J)
    =
    GROUP BY G, AGG(agg_A) on ((GROUP BY G1, AGG(A) AS agg_A on R1)
JOIN R2 ON J)

This equivalence holds under the following conditions:

1) AGG is decomposable, meaning that it can be computed in two stages:
a partial aggregation followed by a final aggregation;
2) The set G1 used in the pre-aggregation of R1 includes:
    * all columns from R1 that are part of the grouping keys G, and
    * all columns from R1 that appear in the join condition J.
3) The grouping operator for any column in G1 must be compatible with
the operator used for that column in the join condition J.

Since G1 includes all columns from R1 that appear in either the
grouping keys G or the join condition J, all rows within each partial
group have identical values for both the grouping keys and the
join-relevant columns from R1, assuming compatible operators are used.
As a result, the rows within a partial group are indistinguishable in
terms of their contribution to the aggregation and their behavior in
the join.  This ensures that all rows in the same partial group share
the same "destiny": they either all match or all fail to match a given
row in R2.  Because the aggregate function AGG is decomposable,
aggregating the partial results after the join yields the same final
result as aggregating after the full join, thereby preserving query
semantics.

Q.E.D.

The second concern is that a RelOptInfo representing a grouped
relation may include paths that produce different row sets due to
partial aggregation being applied at different join levels.  This
potentially violates a fundamental assumption in the planner.

Additionally, the patch currently performs an exhaustive search by
exploring partial aggregation at every possible join level, leading to
excessive planning effort, which may not be justified by the
cost-benefit ratio.

To address these concerns, I'm thinking that maybe we can adopt a
strategy where partial aggregation is only pushed to the lowest
possible level in the join tree that is deemed useful.  In other
words, if we can build a grouped path like "AGG(B) JOIN A" -- and
AGG(B) yields a significant reduction in row count -- we skip
exploring alternatives like "AGG(A JOIN B)".

This is somewhat analogous to how we handle qual clauses: we only push
a qual clause down to the lowest scan or join level that includes all
the relations it references -- following the "filter early, join late"
principle.  For example, if predicate Pb only references B, we only
consider "A JOIN sigma[Pb](B)" and skip "sigma[Pb](A JOIN B)".  (Note
that if Pb involves costly functions and the join is highly selective,
we may want to apply the predicate after the join.)

This ensures that all grouped paths for the same grouped relation
produce the same set of rows (e.g., consider "A JOIN AGG(B) JOIN C"
vs. "AGG(B) JOIN C JOIN A").  As a result, we avoid the complexity of
comparing costs between different grouped paths of the same grouped
relation, and also eliminate the need for special handling of row
estimates on join paths.  It also significantly reduces planning
effort.

While this approach may miss potentially more efficient plans where
applying partial aggregation at a higher join level would yield better
performance, it strikes a practical balance: we can still find plans
that outperform those without eager aggregation, without incurring
excessive planning overhead.  As discussed earlier, it's uncommon in
practice to encounter multiple joins that dramatically inflate row
counts.  So in most cases, pushing partial aggregation to the lowest
level where it offers a significant row count reduction tends to be
the most efficient strategy.

I think this heuristic serves as a good starting point, and we can
look into extending it with more advanced strategies as the feature
evolves.

Any thoughts?

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Thu, 26 Jun 2025 11:01:35 +0900

> exploring alternatives like "AGG(A JOIN B)".

Here is the patch based on the proposed ideas.  It includes the proof
of correctness in the README and implements the strategy of pushing
partial aggregation only to the lowest applicable join level where it
is deemed useful.  This is done by introducing a "Relids apply_at"
field to track that level and ensuring that partial aggregation is
applied only at the recorded "apply_at" level.

Additionally, this patch changes how grouped relations are stored.
Since each grouped relation represents a partially aggregated version
of a non-grouped relation, we now associate each grouped relation with
the RelOptInfo of the corresponding non-grouped relation.  This
eliminates the need for a dedicated list of all grouped relations and
avoids list searches when retrieving a grouped relation.

It also addresses other previously raised concerns, such as the
potential memory blowout risks with large partial-aggregation values,
and includes improvements to comments and the commit message.

Another change is that this feature is now enabled by default.

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Thu, 24 Jul 2025 12:21:30 +0900

> Another change is that this feature is now enabled by default.

This patch no longer applies; here's a rebased version.  Nothing
essential has changed.

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Wed, 6 Aug 2025 16:52:36 +0900

> This patch no longer applies; here's a rebased version.  Nothing

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

---

From: "Matheus Alcantara" <[email-masked]>
Date: Wed, 06 Aug 2025 10:44:29 -0300

> jsonb_agg, or xmlagg, since they don't support partial aggregation
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.

> 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 (?).

> 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

---

From: Richard Guo <[email-masked]>
Date: Sat, 9 Aug 2025 10:32:21 +0900

> functions that are enabled the eager aggregation can work properly.

I ended up still checking for INTERNAL transition types, but
explicitly excluded aggregates that use F_NUMERIC_AVG_ACCUM transition
function, assuming that avg(numeric) and sum(numeric) are safe in this
context.  This might still be overly strict, but I prefer to be on the
safe side for now.

> vary from workload to workload (?).

I've made this threshold a GUC, with a default value of 8 (further
benchmark testing showed that a value of 10 is still too strict for
TPC-DS query 4).

> patch version to perform some other tests.

OK.  Here it is.

Thanks
Richard

---

From: "Matheus Alcantara" <[email-masked]>
Date: Thu, 14 Aug 2025 16:22:49 -0300

>> patch version to perform some other tests.
Thanks! I can confirm now that I can see the eager aggregate in action
in some of these queries that I've tested on the TPC-DS benchmark.

I few questions regarding the new version:

I've noticed that when a query has a WHERE clause filtering columns from
the same relation being aggregated using "=" operator the Partial and
Finalize aggregation nodes are not present on explain results even if
setup_eager_aggregation() returns true on all if statements and also
RelAggInfo->agg_useful is true. For example, consider this query that is
used on eager aggregation paper that use some tables from TPC-H
benchmark:

tpch=# show enable_eager_aggregate ;
 enable_eager_aggregate
------------------------
 on
(1 row)

tpch=# set max_parallel_workers_per_gather to 0;
SET

tpch=# EXPLAIN(COSTS OFF) SELECT O_CLERK,
       SUM(L_EXTENDEDPRICE * (1 - L_DISCOUNT)) AS LOSS
FROM LINEITEM
JOIN ORDERS ON L_ORDERKEY = O_ORDERKEY
WHERE L_RETURNFLAG = 'R'
GROUP BY O_CLERK;
                          QUERY PLAN
--------------------------------------------------------------
 HashAggregate
   Group Key: orders.o_clerk
   ->  Hash Join
         Hash Cond: (lineitem.l_orderkey = orders.o_orderkey)
         ->  Seq Scan on lineitem
               Filter: (l_returnflag = 'R'::bpchar)
         ->  Hash
               ->  Seq Scan on orders
(8 rows)

Debugging this query shows that all if conditions on
setup_eager_aggregation() returns false and create_agg_clause_infos()
and create_grouping_expr_infos() are called. The RelAggInfo->agg_useful
is also being set to true so I would expect to see Finalize and Partial
agg nodes, is this correct or am I missing something here?

Removing the WHERE clause I can see the Finalize and Partial agg nodes:

tpch=# EXPLAIN(COSTS OFF) SELECT O_CLERK,
       SUM(L_EXTENDEDPRICE * (1 - L_DISCOUNT)) AS LOSS
FROM LINEITEM
JOIN ORDERS ON L_ORDERKEY = O_ORDERKEY
GROUP BY O_CLERK;
                              QUERY PLAN
----------------------------------------------------------------------
 Finalize HashAggregate
   Group Key: orders.o_clerk
   ->  Merge Join
         Merge Cond: (lineitem.l_orderkey = orders.o_orderkey)
         ->  Partial GroupAggregate
               Group Key: lineitem.l_orderkey
               ->  Index Scan using idx_lineitem_orderkey on lineitem
         ->  Index Scan using orders_pkey on orders
(8 rows)

This can also be reproduced with an addition of a WHERE clause on some
tests on eager_aggregate.sql:

postgres=# EXPLAIN (VERBOSE, COSTS OFF)
SELECT t1.a, avg(t2.c)
FROM eager_agg_t1 t1
JOIN eager_agg_t2 t2
    ON t1.b = t2.b
WHERE t2.c = 5
GROUP BY t1.a
ORDER BY t1.a;
                            QUERY PLAN
------------------------------------------------------------------
 GroupAggregate
   Output: t1.a, avg(t2.c)
   Group Key: t1.a
   ->  Sort
         Output: t1.a, t2.c
         Sort Key: t1.a
         ->  Hash Join
               Output: t1.a, t2.c
               Hash Cond: (t1.b = t2.b)
               ->  Seq Scan on public.eager_agg_t1 t1
                     Output: t1.a, t1.b, t1.c
               ->  Hash
                     Output: t2.c, t2.b
                     ->  Seq Scan on public.eager_agg_t2 t2
                           Output: t2.c, t2.b
                           Filter: (t2.c = '5'::double precision)
(16 rows)

Note that if I use ">" operator for example, this doesn't happen:
SELECT t1.a, avg(t2.c)
FROM eager_agg_t1 t1
JOIN eager_agg_t2 t2
    ON t1.b = t2.b
WHERE t2.c > 5
GROUP BY t1.a
ORDER BY t1.a;
                               QUERY PLAN
------------------------------------------------------------------------
 Finalize GroupAggregate
   Output: t1.a, avg(t2.c)
   Group Key: t1.a
   ->  Sort
         Output: t1.a, (PARTIAL avg(t2.c))
         Sort Key: t1.a
         ->  Hash Join
               Output: t1.a, (PARTIAL avg(t2.c))
               Hash Cond: (t1.b = t2.b)
               ->  Seq Scan on public.eager_agg_t1 t1
                     Output: t1.a, t1.b, t1.c
               ->  Hash
                     Output: t2.b, (PARTIAL avg(t2.c))
                     ->  Partial HashAggregate
                           Output: t2.b, PARTIAL avg(t2.c)
                           Group Key: t2.b
                           ->  Seq Scan on public.eager_agg_t2 t2
                                 Output: t2.a, t2.b, t2.c
                                 Filter: (t2.c > '5'::double precision)
(19 rows)

Is this behavior correct? If it's correct, would be possible to check
this limitation on setup_eager_aggregation() and maybe skip all the
other work?

--
Matheus Alcantara

---

From: Richard Guo <[email-masked]>
Date: Fri, 15 Aug 2025 10:41:18 +0900

> agg nodes, is this correct or am I missing something here?

Well, just because eager aggregation *can* be applied does not mean
that it *will* be; it depends on whether it produces a lower-cost
execution plan.  This transformation is cost-based, so it's not the
right mindset to assume that it will always be applied when possible.

In your case, with the filter "t2.c = 5", the row estimate for t2 is
just 1 after the filter has been applied.  The planner decides that
adding a partial aggregation on top of such a small result set doesn't
offer much benefit, which seems reasonable to me.

->  Hash  (cost=18.50..18.50 rows=1 width=12)
          (actual time=0.864..0.865 rows=1.00 loops=1)
      Buckets: 1024  Batches: 1  Memory Usage: 9kB
      ->  Seq Scan on eager_agg_t2 t2  (cost=0.00..18.50 rows=1 width=12)
                                       (actual time=0.060..0.851
rows=1.00 loops=1)
            Filter: (c = '5'::double precision)
            Rows Removed by Filter: 999

With the filter "t2.c > 5", the row estimate for t2 is 995 after
filtering.  A partial aggregation can reduce that to 10 rows, so the
planner decides that adding a partial aggregation is beneficial -- and
does so.  That also seems reasonable to me.

->  Partial HashAggregate  (cost=23.48..23.58 rows=10 width=36)
                           (actual time=2.427..2.438 rows=10.00 loops=1)
      Group Key: t2.b
      Batches: 1  Memory Usage: 32kB
      ->  Seq Scan on eager_agg_t2 t2  (cost=0.00..18.50 rows=995 width=12)
                                       (actual time=0.053..0.989
rows=995.00 loops=1)
            Filter: (c > '5'::double precision)
            Rows Removed by Filter: 5

> this limitation on setup_eager_aggregation() and maybe skip all the

Hmm, I wouldn't consider this a limitation; it's just the result of
the planner's cost-based tournament for path selection.

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Mon, 1 Sep 2025 10:32:33 +0900

> OK.  Here it is.

This patch needs a rebase; here it is.  No changes were made.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Fri, 5 Sep 2025 16:35:17 +0900

> This patch needs a rebase; here it is.  No changes were made.

Here is a rebase after the GUC tables change.

- Richard

---

From: Robert Haas <[email-masked]>
Date: Fri, 5 Sep 2025 09:09:56 -0400

Sorry for the slow response.

> the operator used for that column in the join condition J.

This proof seems to ignore join-order constraints. I'm not sure to
what degree that influences the ultimate outcome here, but given A
LEFT JOIN (B INNER JOIN C), we cannot simply decide that A and C
comprise R1 and B comprises R2, because it is not actually possible to
do the A-C join first and treat the result as a relation to be joined
to B. That said, I do very much like the explicit enumeration of
criteria that must be met for the optimization to be valid. That makes
it a lot easier to evaluate whether the theory of the patch is
correct.

> exploring alternatives like "AGG(A JOIN B)".

I really like this idea. I believe we need some heuristic here and
this seems like a reasonable one. I think there could be a better one,
potentially. For instance, it would be reasonable (in my opinion) to
do some kind of evaluation of AGG(A JOIN B) vs. AGG(B) JOIN A that
does not involve performing full path generation for both cases; e.g.
one could try to decide considering only row counts, for instance.
However, I'm not saying that would work better than your proposal
here, or that it should be a requirement for this to be committed;
it's just an idea. IMHO, the requirement to have something committable
is that there is SOME heuristic limiting the search space and at the
same time the patch can still be demonstrated to give SOME benefit. I
think what you propose here meets those criteria. I also like the fact
that it's simple and easy to understand. If it does go wrong, it will
not be too difficult for someone to understand why it has gone wrong,
which is very desirable.

> look into extending it with more advanced strategies as the feature

So IOW, +1 to what you say here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Robert Haas <[email-masked]>
Date: Fri, 5 Sep 2025 09:12:44 -0400

> jsonb_agg, or xmlagg, since they don't support partial aggregation

This strategy seems fairly unfriendly towards out-of-core code. Can
you come up with something that allows the author of a SQL-callable
function to include or exclude the function by a choice that is under
their control, rather than hard-coding something in PostgreSQL itself?

---

From: Robert Haas <[email-masked]>
Date: Fri, 5 Sep 2025 10:37:10 -0400

> Here is a rebase after the GUC tables change.

I spent a bit of time scrolling through this today. Here are a few
observations/review comments.

It looks as though this will create a bunch of RelOptInfo objects that
don't end up getting used for anything once the apply_at test in
generate_grouped_paths() fails. It seems to me that it would be better
to altogether avoid generating the RelOptInfo in that case.

I think it would be worth considering generating the partially grouped
relations in a second pass. Right now, as you progress from the bottom
of the join tree towards the top, you created grouped rels as you go.
But you could equally well finish planning everything up to the
scan/join target first and then go back and add grouped_rels to
relations where it seems worthwhile. I don't know if this would really
make a big difference as you have things today, but I think it might
provided a better structure for the future, because you would then
have a lot more information with which to judge where to do
aggregation. For instance, you could looked at the row counts of any
number of those ungrouped-rels before deciding where to put the
partial aggregation. That seems like it could be pretty valuable.

I haven't done a detailed comparison of generate_grouped_paths() to
other parts of the code, but I have an uncomfortable feeling that it
might be rather similar to some existing code that probably already
exists in multiple, slightly-different versions. Is there any
refactoring we could do here?

Do you need a test of this feature in combination with GEQO? You have
code for it but I don't immediately see a test. I didn't check
carefully, though.

Overall I like the direction this is heading. I don't feel
well-qualified to evaluate whether all of the things that you're doing
are completely safe. The logic in is_var_in_aggref_only() and
is_var_needed_by_join() scares me a bit because I worry that the
checks are somehow non-exhaustive, but I don't know of a specific
hazard. That said, I think that modulo such issues, this has a good
chance of significantly improving performance for certain query
shapes.

One thing to check might be whether you can construct any cases where
the strategy is applied too boldly. Given the safeguards you've put in
place that seems a little a little hard to construct. The most obvious
thing that occurs to me is an aggregate where combining is more
expensive than aggregating, so that the partial aggregation gives the
appearance of saving more work than it really does, but I can't
immediately think of a problem case. Another case could be where the
row counts are off, leading to us mistakenly believing that we're
going to reduce the number of rows that need to be processed when we
really don't. Of course, such a case would arguably be a fault of the
bad row-count estimate rather than this patch, but if the patch has
that problem frequently, it might need to be addressed. Still, I have
a feeling that the testing you've already been doing might have
surfaced such cases if they were common. Have you looked into how many
queries in the regression tests, or in TPC-H/DS, expend significant
planning effort on this strategy before discarding it? That might be a
good way to get a sense of whether the patch is too aggressive, not
aggressive enough, a mix of the two, or just right.

---

From: Robert Haas <[email-masked]>
Date: Fri, 5 Sep 2025 10:50:15 -0400

> adjust it based on their needs.)

Like Matheus, I think a GUC is reasonable. A significant danger here
appears to be the possibility of a performance cliff, where queries
are optimized very different when the ratio is 9.99 vs. 10.01, say. It
would be nice if there were some way to mitigate that danger, but at
least a GUC avoids chaining the performance of the whole system to a
hard-coded value.

It might be worth considering whether there are heuristics other than
the group size that could help here. Possibly that's just making
things more complicated to no benefit. It seems to me, for example,
that reducing 100 rows to 10 is quite different from reducing a
million rows to 100,000. On the whole, the latter seems more likely to
work out well, but it's tricky, because the effort expended per group
can be arbitrarily high. I think we do want to let the cost model make
most of the decisions, and just use this threshold to prune ideas that
are obviously bad at an early stage. That said, it's worth thinking
about how this interacts with the just-considered-one-eager-agg
strategy. Does this threshold apply before or after that rule?

For instance, consider AGG(FACT_TABLE JOIN DIMENSION_TABLE), like a
count of orders grouped by customer name. Aggregating on the dimension
table (in this case, the list of customers) is probably useless, but
aggregating on the join column of the fact table has a good chance of
being useful. If we consider only one of those strategies, we want it
to be the right one. This threshold could be the thing that helps us
to get it right.

---

From: Richard Guo <[email-masked]>
Date: Tue, 9 Sep 2025 18:07:54 +0900

> it a lot easier to evaluate whether the theory of the patch is

Thanks for pointing this out.  I should have clarified that the proof
is intended for the inner join case.  My plan was to first establish
the correctness for inner joins, and then extend the proof to cover
outer joins, but I failed to make that clear.

In the case where there are any outer joins, the situation becomes
more complex due to join order constraints and the semantics of
null-extension in outer joins.  If the relations that contain at least
one aggregation column cannot be treated as a single relation because
of the join order constraints, partial aggregation paths will not be
generated, and thus the transformation is not applicable.

Otherwise, to preserve correctness, we need to add an additional
condition: R1 must not be on the nullable side of any outer join.
This ensures that partial aggregation over R1 does not suppress any
null-extended rows that would be introduced by outer joins.

I'll update the proof in README to cover the outer join case.

> So IOW, +1 to what you say here.

Thanks for liking this idea.  Another way this heuristic makes life
easier is that it ensures all grouped paths for the same grouped
relation produce the same set of rows.  This means we don't need all
the hacks for comparing costs between grouped paths, nor do we have to
resolve disputes about how many RelOptInfos to create for a single
grouped relation.  I'd prefer to keep this property for now and
explore more complex heuristics in the future.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Tue, 9 Sep 2025 18:20:21 +0900

> their control, rather than hard-coding something in PostgreSQL itself?

Yeah, ideally we should tell whether an aggregate's transition state
may grow unbounded just by looking at system catalogs.  Unfortunately,
after trying for a while, it seems to me that the current catalog
doesn't provide enough information.

I once considered adding a flag (e.g., aggtransbounded) to catalog
pg_aggregate to indicate whether the transition state size is bounded.
This flag could be specified by users when creating aggregate
functions, and then leveraged by features such as eager aggregation.

However, adding new information to system catalogs involves a lot of
discussions and changes, including updates to DDL commands, dump and
restore processes, and upgrade procedures.  Therefore, to keep the
focus of this patch on the eager aggregation feature itself, I prefer
to treat this enhancement as future work.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Tue, 9 Sep 2025 19:30:04 +0900

> observations/review comments.

Thanks for all the comments.

> to altogether avoid generating the RelOptInfo in that case.

Hmm, that's not the case.  make_grouped_join_rel() guarantees that for
a given relation, if its grouped paths are not considered useful, and
no grouped paths can be built by joining grouped input relations, then
its grouped relation will not be created.  IOW, we only create a
grouped RelOptInfo if we've determined that we can generate useful
grouped paths for it.

In the case you mentioned, where the apply_at test in
generate_grouped_paths() fails, it must mean that grouped paths can be
built by joining its outer and inner relations.  Also, note that calls
to generate_grouped_paths() are always followed by calls to
set_cheapest().  If we failed to generate any grouped paths for a
grouped relation, the set_cheapest() call should already have reported
an error.

> relations where it seems worthwhile.

Hmm, I don't think so.  I think the presence of eager aggregation
could change the best join order.  For example, without eager
aggregation, the optimizer might find that (A JOIN B) JOIN C the best
join order.  But with eager aggregation on B, the optimizer could
prefer A JOIN (AGG(B) JOIN C).  I'm not sure how we could find the
best join order with eager aggregation applied without building the
join tree from the bottom up.

> refactoring we could do here?

Yeah, we currently have several functions that do similar, but not
exactly the same, things.  Maybe some refactoring is possible -- maybe
not -- I haven't looked into it closely yet.  However, I'd prefer to
address that in a separate patch if possible, since this issue also
exists on master, and I want to avoid introducing such changes in this
already large patch.

> code for it but I don't immediately see a test. I didn't check

Good point.  I do have manually tested GEQO by setting geqo_threshold
to 2 and running the regression tests to check for any planning
errors, crashes, or incorrect results.  However, I'm not sure where
test cases for GEQO should be added.  I searched the regression tests
and found only one explicit GEQO test, added back in 2009 (commit
a43b190e3).  It's not quite clear to me what the current policy is for
adding GEQO test cases.

Anyway, I will add some test cases in eager_aggregate.sql with
geqo_threshold set to 2.

> aggressive enough, a mix of the two, or just right.

I previously looked into the TPC-DS queries where eager aggregation
was applied and didn't observe any regressions in planning time or
execution time.  I can run TPC-DS again to check the planning time for
the remaining queries.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Tue, 9 Sep 2025 20:18:20 +0900

> least a GUC avoids chaining the performance of the whole system to a

Yeah, I think the performance cliff issue does exist.  It might be
mitigated by carefully selecting the threshold value to ensure that
small differences in the average group size near the boundary don't
cause big performance swings with and without eager aggregation, but
this doesn't seem like an easy task.

How is this issue avoided in other thresholds?  For example, with
min_parallel_table_scan_size, is there a performance cliff when the
table size is 7.99MB vs. 8.01MB, where a parallel scan is considered
in the latter case but not the former?

> strategy. Does this threshold apply before or after that rule?

If I understand correctly, this means that we need to explore each
join level to find out the most optimal position for applying partial
aggregation.  For example, suppose Agg(B) reduces 100 rows to 10, and
Agg(A JOIN B) reduces a million rows to 100,000, it might be better to
apply partial aggregation at the (A JOIN B) level rather than just
over B.  However, that's not always the case: the Agg(B) option can
reduce the number of input rows to the join earlier, potentially
outperforming the Agg(A JOIN B) approach.  Therefore, we need to
consider both options and compare their costs.

This is actually what the patch used to do before I introduced the
always-push-to-lowest heuristic.

> to be the right one. This threshold could be the thing that helps us

Now I see what you meant.  However, in the current implementation, we
only push partial aggregation down to relations that contain all the
aggregation columns.  So, in the case you mentioned, if the
aggregation columns come from the dimension table, unfortunately, we
don't have the option to partially aggregate the fact table.

The paper does discuss several other transformations, such as "Eager
Count", "Double Eager", and "Eager Split", that can perform partial
aggregation on relations that don't contain aggregation columns, or
even on both sides of the join.  However, those are beyond the scope
of this patch.

- Richard

---

From: Robert Haas <[email-masked]>
Date: Tue, 9 Sep 2025 10:20:14 -0400

> to treat this enhancement as future work.

I don't really like that. I think there's a lot of danger of that
future work never getting done, and thus leaving us stuck more-or-less
permanently with a system that's not really extensible. Data type and
function extensibility is one of the strongest areas of PostgreSQL,
and we should try hard to avoid situations where we regress it. I'm
not sure whether the aggtransbounded flag is exactly the right thing
here, but I don't think adding a new catalog column is an unreasonable
amount of work for a feature of this type.

Having said that, I wonder whether there's some way that we could use
the aggtransspace property for this. For instance, for stanullfrac, we
use values >0 to mean absolute quantities and values <0 to mean
proportions. The current definition of aggtranspace assigns no meaning
to values <0, and the current coding seems to assume that sizes are
fixed regardless of how many inputs are supplied. Maybe we could
define aggtransspace<0 to mean that the number of bytes used per input
value is the additive inverse of the value, or something like that.

---

From: Robert Haas <[email-masked]>
Date: Tue, 9 Sep 2025 10:30:01 -0400

> join tree from the bottom up.

Oh, that is a problem, yes. :-(

> exists on master, and I want to avoid introducing such changes in this

Well, it's not just a matter of "this already exists" -- it gets
harder and harder to unify things the more near-copies you add.

> Anyway, I will add some test cases in eager_aggregate.sql with

Sounds good. I think GEQO is mostly-unmaintained these days, but if
we're updating the code, I think it is good to add tests. Being that
the code is so old, it probably lacks adequate test coverage.

---

From: Richard Guo <[email-masked]>
Date: Fri, 12 Sep 2025 18:34:43 +0900

> value is the additive inverse of the value, or something like that.

I really like this idea.  Currently, aggtransspace represents an
estimate of the transition state size provided by the aggregate
definition.  If it's set to zero, a default estimate based on the
state data type is used.  Negative values currently have no defined
meaning.  I think it makes perfect sense to reuse this field so that
a negative value indicates that the transition state data can grow
unboundedly in size.

Attached 0002 implements this idea.  It requires fewer code changes
than I expected.  This is mainly because that our current code uses
aggtransspace in such a way that if it's a positive value, that value
is used as it's provided by the aggregate definition; otherwise, some
heuristics are applied to estimate the size.  For the aggregates that
accumulate input rows (e.g., array_agg, string_agg), I don't currently
have a better heuristic for estimating their size, so I've chosen to
keep the current logic.  This won't regress anything in estimating
transition state data size.

- Richard

---

From: Robert Haas <[email-masked]>
Date: Fri, 12 Sep 2025 14:47:48 -0400

> transition state data size.

This might be OK, but it's not what I was suggesting: I was suggesting
trying to do a calculation like space_used = -aggtransspace *
rowcount, not just using a <0 value as a sentinel.

---

From: Richard Guo <[email-masked]>
Date: Sat, 13 Sep 2025 17:27:41 +0900

> rowcount, not just using a <0 value as a sentinel.

I've considered your suggestion, but I'm not sure I'll adopt it in the
end.  Here's why:

1) At the point where we check whether any aggregates might pose a
risk of excessive memory usage during partial aggregation, row count
information is not yet available.  You could argue that we could
reorganize the logic to perform this check after we've had the row
count, but that seems quite tricky.  If I understand correctly, the
"rowcount" in this context actually means the number of rows within
one partial group.  That would require us to first decide on the
grouping expressions for the partial aggregation, then compute the
group row counts, then estimate space usage, and only then decide
whether memory usage is excessive and fall back.  This would come
quite late in planning and adds nontrivial overhead, compared to the
current approach which checks at the very beginning.

2) Even if we were able to estimate space usage based on the number of
rows per partial group and determined that memory usage seems
acceptable, we still couldn't guarantee that the transition state data
won't grow excessively after further joins.  Joins can multiply
partial aggregates, potentially causing a blowup in memory usage even
if the initial estimate seemed safe.

3) I don't think "-aggtransspace * rowcount" reflects the true memory
footprint for aggregates that accumulate input rows.  For example,
what if we have an aggregate like string_agg(somecolumn, 'a very long
delimiter')?

4) AFAICS, the main downside of the current approach compared to yours
is that it avoids pushing down aggregates like string_agg() that
accumulate input rows, whereas your suggestion might allow pushing
them down in some cases where we *think* it wouldn't blow up memory.
You might argue that the current implementation is over-conservative.
But I prefer to start safe.

That said, I appreciate you proposing the idea of reusing
aggtransspace, although I ended up using it in a different way than
you suggested.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Thu, 25 Sep 2025 13:23:00 +0900

I've run TPC-DS again to compare planning times with and without eager
aggregation.  Out of 99 queries, only one query (query 64) shows a
noticeable increase in planning time.  This query performs inner joins
across 38 tables.  This is a very large search space.  (I'm talking
about the standard join search method, not the GEQO.)

If my math doesn't fail me, the maximum number of different join
orders when joining n tables is: Catalan(n − 1) x n!.  For n = 38,
this number is astronomically large.  In practice, query 64 joins 19
tables twice (due to a CTE), which still results in about 3.4E28
different join orders.

Of course, in practice, with the help of join_collapse_limit and other
heuristics, the effective search space is reduced a lot, but even
then, it remains very large.  Given this, I'm not too surprised that
query 64 shows an increase in planning time when eager aggregation is
applied -- exploring the best join order in such a space is inherently
expensive.

That said, I've identified a few performance hotspots that can be
optimized to help reduce planning time:

1) the exprs_known_equal() call in get_expression_sortgroupref(),
which is used to check if a given expression is known equal to a
grouping expression due to ECs.  We can optimize this by storing the
EC of each grouping expression, and then get_expression_sortgroupref()
would only need to search the relevant EC, rather than scanning all of
them.

2) the estimate_num_groups() call in create_rel_agg_info().  We can
optimize this by avoiding unnecessary calls to estimate_num_groups()
where possible.

Attached is an updated version of the patch with these optimizations
applied.  With this patch, the planning times for query 64, with and
without eager aggregation, are:

-- with eager aggregation
 Planning Time: 9432.042 ms
-- without eager aggregation
 Planning Time: 7196.999 ms

I think the increase in planning time is acceptable given the large
search space involved, though I may be biased.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Mon, 29 Sep 2025 11:09:07 +0900

On Thu, Sep 25, 2025 at 1:23 PM Richard Guo <[email-masked]> wrote:
> applied.

FWIW, I plan to do another self-review of this patch soon, with the
goal of assessing whether it's ready to be pushed.  If anyone has any
concerns about any part of the patch or would like to review it, I
would greatly appreciate hearing from you.

- Richard

---

From: Matheus Alcantara <[email-masked]>
Date: Wed, 1 Oct 2025 20:54:32 -0300

[ getting back to testing this patch ...]

> right mindset to assume that it will always be applied when possible.
Sorry for the noise here. I didn't consider the costs.

> would greatly appreciate hearing from you.
I spent some time testing patch v23 using the TPC-DS benchmark and am
seeing worse execution times when using eager aggregation.
The most interesting cases are:

Query    |  planning time |  execution time |
query 31 |   -2.03%       │    -99.56%      │
query 71 |  -15.51%       │    -68.88%      │
query 20 |  -10.77%       │    -32.40%      │
query 26 |  -28.01%       │    -32.35%      │
query 85 |  -10.57%       │    -31.91%      │
query 77 |  -30.07%       │    -31.38%      │
query 69 |  -32.79%       │    -29.21%      │
query 32 |  -68.48%       │    -27.89%      │
query 57 |   -7.99%       │    -27.32%      │
query 91 |  -24.81%       │    -26.20%      │
query 23 |  -11.72%       │    -18.24%      │

The query 31 seems bad, I don't know if I'm doing something completely
wrong but I've just setup a TPC-DS database and then executed the query
on master and with the v23 patch and I got these results:

Master:
    Planning Time: 3.191 ms
    Execution Time: 16950.619 ms

Patch:
    Planning Time: 3.257 ms
    Execution Time: 3848355.646 ms

Note that I've executed ANALYZE before running the queries on both
scenarios (master and patched).

I'm attaching an EXPLAIN(ANALYZE) output for the query 31 from master
and with the patch applied.

Please let me know if there is any other test that I can run to
benchmark this patch.

--
Matheus Alcantara

---

From: Richard Guo <[email-masked]>
Date: Thu, 2 Oct 2025 10:13:48 +0900

>     Execution Time: 3848355.646 ms

Thanks for reporting this.  It does seem odd.  I checked the TPC-DS
benchmarking on v13 and found that the execution time for query 31,
with and without eager aggregation, is as follows:

       EAGER-AGG-OFF           EAGER-AGG-ON
q31     10463.536 ms            10244.175 ms

There appears to be a regression between v13 and v23.  Looking into
it...

- Richard

---

From: Richard Guo <[email-masked]>
Date: Thu, 2 Oct 2025 10:39:50 +0900

> There appears to be a regression between v13 and v23.  Looking into

I noticed something interesting while comparing the two EXPLAIN
(ANALYZE) outputs: the patched version uses parallel plans, whereas
the master does not.  To rule that out as a factor, I ran "SET
max_parallel_workers_per_gather TO 0;" and re-ran query 31 on both
master and the patched version.  This time, I got a positive result.

-- on master
 Planning Time: 5.281 ms
 Execution Time: 7222.665 ms

-- on patched
 Planning Time: 4.855 ms
 Execution Time: 5977.287 ms

It seems eager aggregation doesn't cope well with parallel plans for
this query.  Looking into it.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Thu, 2 Oct 2025 17:49:27 +0900

> this query.  Looking into it.

It turns out that this is not related to parallel plans but rather to
poor size estimates.

Looking at query 31, it involves joining 6 base relations, all of
which are CTE references (i.e., RTE_CTE relations) to two different
CTEs.  Each CTE involves aggregations and GROUP BY clauses.
Unfortunately, our size estimates for CTE relations are quite poor,
especially when the CTE uses GROUP BY.  In these cases, we don't have
any ANALYZE statistics available (cf. examine_simple_variable).  As a
result, when computing the selectivity of the CTE relation's qual
clauses, we have to fall back on default values.  For example, for
quals like "CTE.var = const", which are used a lot in query 31, the
selectivity is computed as "1.0 / DEFAULT_NUM_DISTINCT(200)", with the
assumption that there are DEFAULT_NUM_DISTINCT distinct values in the
relation, and that these values are equally common (cf. var_eq_const).

The consequence is that the size estimates are significantly different
from the actual values.  For example, from the EXPLAIN(ANALYZE) output
provided by Matheus:

->  CTE Scan on ws ws3  (cost=0.00..1797.35 rows=2 width=110)
                 (actual time=0.001..74.725 rows=1261.00 loops=1)
      Filter: ((d_year = 1999) AND (d_qoy = 3))

Interestingly, with eager aggregation applied, the row count estimates
for the two CTE plans actually become closer to the actual values.

-- without eager aggregation
CTE ws
  ->  HashAggregate  (cost=96009.03..114825.35 rows=718952 width=54)
                (actual time=977.215..1014.889 rows=23320.00 loops=1)

-- with eager aggregation
CTE ws
  ->  Finalize GroupAggregate  (cost=52144.19..62314.79 rows=71894 width=54)
                          (actual time=275.121..340.107 rows=23312.00 loops=1)

However, due to the highly underestimated selectivity for the qual
clauses, the row count estimates for CTE Scan nodes become worse.
This is because:

-- without eager aggregation
718952 * (1.0/200) * (1.0/200) ~= 18

-- with eager aggregation
71894 * (1.0/200) * (1.0/200) ~= 2

... while the actual row count is 1261.00 as shown above.

That is to say, on master, the CTE plan rows are overestimated while
the selectivity estimates are severely underestimated.  With eager
aggregation, the CTE plan rows become closer to the actual values, but
the selectivity estimates remain equally underestimated.  As a result,
the row count estimates for the CTE Scan nodes worsen with eager
aggregation.  This causes the join order in the final plan to change
when eager aggregation is applied, leading to longer execution times
in this case.

Another point to note is that, due to severely underestimated
selectivity estimates (0.000025, sometimes 0.000000125), the size
estimates for the CTE relations are very small, causing the planner to
tend to choose nestloops.  I tried manually disabling nestloop, and
here are what I got for query 31.

-- on master, set enable_nestloop to on;
 Planning Time: 4.613 ms
 Execution Time: 7142.090 ms

-- on master, set enable_nestloop to off;
 Planning Time: 4.315 ms
 Execution Time: 2262.330 ms

-- on patched, set enable_nestloop to off;
 Planning Time: 4.321 ms
 Execution Time: 1214.376 ms

That is, on master, simply disabling nestloop makes query 31 run more
than 3 times faster.  Enabling eager aggregation on top of that
improves performance further, making it run 1.86 times faster relative
to the nested-loop-disabled baseline.

I manually disabled nested loops for other TPC-DS queries on master
and discovered some additional interesting findings.

For query 4, on master:

-- set enable_nestloop to on
 Planning Time: 3.054 ms
 Execution Time: 3231356.258 ms

-- set enable_nestloop to off
 Planning Time: 4.291 ms
 Execution Time: 12751.170 ms

That is, on master, simply disabling nestloop makes query 4 run more
than 253 times faster.

For query 11, on master:

-- set enable_nestloop to on
 Planning Time: 1.435 ms
 Execution Time: 1824860.937 ms

-- set enable_nestloop to off
 Planning Time: 2.479 ms
 Execution Time: 7984.360 ms

Disabling nestloop makes query 11 run more than 228 times faster.

I believe you can find more such queries in TPC-DS if you keep
looking.  Given this, I don't think it makes much sense to debug a
performance regression on TPC-DS with nestloop enabled.

Matheus, I wonder if you could help run TPC-DS again with this patch,
this time with nested loops disabled for all queries.

- Richard

---

From: Matheus Alcantara <[email-masked]>
Date: Thu, 2 Oct 2025 15:40:47 -0300

On Thu Oct 2, 2025 at 5:49 AM -03, Richard Guo wrote:
> [ ... ]

> this time with nested loops disabled for all queries.
Thanks for all the details. I've disabled the nested loops and executed
the benchmark again and the results look much better! I see a 55%
improvement on query_31 on my machine now (MacOS M3 Max).

The only query that I see a considerable regression is query 23 which I
get a 23% worst execution time. I'm attaching the EXPLAIN(ANALYZE)
output from master and from the patched version if it's interesting.

I'm also attaching a csv with the planning time and execution time from
master and the patched version for all queries. It contains the % of
difference between the executions. Negative numbers means that the
patched version using eager aggregation is faster. (I loaded this csv on
a postgres table and played with some queries to analyze the results).

I'm just wondering if there is anything that can be done on the planner
to prevent this type of situation?

--
Matheus Alcantara

---

From: Richard Guo <[email-masked]>
Date: Fri, 3 Oct 2025 12:14:40 +0900

> improvement on query_31 on my machine now (MacOS M3 Max).

Great!  That is 2.23 times faster.

> output from master and from the patched version if it's interesting.

I tested query 23 in my local environment but didn't observe the
regression.

-- on master
 Planning Time: 1.950 ms
 Execution Time: 3260.924 ms

-- on patched
 Planning Time: 2.197 ms
 Execution Time: 3237.287 ms

I ran the benchmark at scale factor 1 and executed ANALYZE beforehand.
For the build configuration, I disabled cassert.

Comparing the plans, I noticed one key difference: in the plan you
provided (query-23.patch.explain), the frequent_ss_items CTE uses
parallel aggregation, whereas in my local environment it does not.
This leads to a different final join order between the two plans.

However, given the highly inaccurate size and cost estimates for the
CTE Scan nodes, I'm not sure it's worth investigating further.  I'm
starting to feel that trying to tune performance here, with such
inaccurate underlying estimates for CTEs, is like building on sand.

> a postgres table and played with some queries to analyze the results).

I really appreciate this; it's very helpful.

> to prevent this type of situation?

I think the ideal solution is to improve our estimates for CTE
relations to make the plans for TPC-DS queries more reasonable.  Of
course, for queries from other benchmarks, the issues may stem from
other plan nodes.  IMHO, we really need some improvements in our cost
estimation.

- Richard

---

From: "Matheus Alcantara" <[email-masked]>
Date: Fri, 03 Oct 2025 17:03:08 -0300

> For the build configuration, I disabled cassert.
I've disabled the cassert and executed the ANALYZE again before
benchmarking and now I have similar results with a improvement on eager
aggregate version:

-- master
Planning Time: 2.734 ms
Execution Time: 5238.128 ms

-- patched
Planning Time: 2.578 ms
Execution Time: 4732.584 ms

> [ ...]
Fair points, agree.

The performance results look good to me. I don't have to much comments
about the code although I'm still learning about the planner internals
this patch seems in good shape to me.

I'm just attaching a new csv with the last results after running with
cassert disabled and after executing ANALYZE. It looks good to me.

Thanks for working on this!

--
Matheus Alcantara

---

From: Richard Guo <[email-masked]>
Date: Mon, 6 Oct 2025 09:56:25 +0900

> Execution Time: 4732.584 ms

Great!

> this patch seems in good shape to me.

Thanks for running the benchmark and reviewing the patch.

> cassert disabled and after executing ANALYZE. It looks good to me.

Yeah, the results look good this time.  There are no performance
regressions; on the contrary, several queries actually show very
really nice improvements.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Mon, 6 Oct 2025 09:59:11 +0900

> would greatly appreciate hearing from you.

Barring any objections, I'll plan to push v23 in a couple of days.

- Richard

---

From: David Rowley <[email-masked]>
Date: Tue, 7 Oct 2025 02:59:44 +1300

> Barring any objections, I'll plan to push v23 in a couple of days.

Not a complete review, but a customary look:

1. setup_base_grouped_rels() by name and the header comment claim to
operate on base relations, but the code seems to be coded to handle
OTHER_MEMBER rels too.

Note that set_base_rel_pathlists() explicitly skips anything that's
not RELOPT_BASEREL, so if you're not doing that, then you shouldn't
use "base" in the function name. It's confusing.

2. All the calls to generate_grouped_paths() pass the grouped_rel
RelOptInfo and also grouped_rel->agg_info. Is there a reason to keep
it that way rather than access the agg_info from the given grouped_rel
from within the function?

3. " * The information needed are provided by the RelAggInfo
structure." This should use "is" rather than "are"

4. standard_join_search(). I think it's worth getting rid of the
duplicate "if (!bms_equal(rel->relids, root->all_query_rels))" check.
How about setting that in a local variable rather than recalling
bms_equal(). I don't believe the compiler will optimise the extra one
away as it can't know set_cheapest() doesn't change the relids. Also,
wouldn't it be better to check rel->grouped_rel != NULL first? Won't
that be NULL in most cases, where as !bms_equal(rel->relids,
root->all_query_rels) will be true in most cases? Likewise in
generate_partitionwise_join_paths().

5. Wouldn't it be better to do 0002 first and get that into core so
you don't have to do the hacky stuff in is_partial_agg_memory_risky()?

6. Shouldn't this be using lappend()?

 agg_clause_list = list_append_unique(agg_clause_list, ac_info);

I don't understand why ac_info could already be in the list. You've
just done: ac_info = makeNode(AggClauseInfo);

7. The following comment talks about "base" relations. I don't think
it should be as the RelOptInfo can be an OTHER_MEMBER rel.

 * build_simple_grouped_rel
 *   Construct a new RelOptInfo representing a grouped version of the input
 *   base relation.
 */

8. Normally we check the List is NIL instead of:

if (list_length(group_clauses) == 0)

9. In get_expression_sortgroupref(), a comment claims "We ignore child
members here.". I think that's outdated since ec_members no longer has
child members.

10. I don't think this comment quite makes sense:

 * "apply_at" tracks the lowest join level at which partial aggregation is
 * applied.

maybe "minimum set of rels to join before partial aggregation can be applied"?

or at least swap "is" for "can be".

My confusion comes from the fact you're stating "lowest join level",
which seems to indicate that it could be applied after further
relations have been joined, but then you're saying "is applied" to
indicate that it can only be applied at that level.

11. The way you've written the header comments for typedef struct
RelAggInfo seems weird.  I've only ever seen extra details in the
header comment when the inline comments have been kept to a single
line. You're spanning multiple lines, so why have the out of line
comments in the header at all?

12. This just doesn't feel like the right name for this field:

/* lowest level partial aggregation is applied at */
Relids apply_at;

I can't help think that it should be something like "agg_relids" or
"required_relids".  I understand you're currently only applying the
partial grouping when you get exactly the minimum set of relids in the
join search, but if this can be made fast enough, I expect that could
be changed in the future. If you do change it, then "apply_at" is a
pretty confusing name.  Perhaps I've misunderstood here and if you did
that, you'd need to create another RelAggInfo to represent that?

13. Parameter names mismatch between definition and declaration in:

extern RelOptInfo *build_simple_grouped_rel(PlannerInfo *root,
RelOptInfo *rel_plain);
extern RelOptInfo *build_grouped_rel(PlannerInfo *root,
RelOptInfo *rel_plain);

extern void generate_grouped_paths(PlannerInfo *root,
   RelOptInfo *rel_grouped,
   RelOptInfo *rel_plain,
   RelAggInfo *agg_info);

14. Do all the regression tests need VERBOSE in EXPLAIN? It's making
the output kinda huge. It might also be nice to wrap the long queries
onto multiple lines to make them easier to read.

David

---

From: Richard Guo <[email-masked]>
Date: Tue, 7 Oct 2025 19:56:48 +0900

> Not a complete review, but a customary look:

Thanks for all the comments!  They've been very helpful.

> operate on base relations, but the code seems to be coded to handle

Indeed.  I renamed it to setup_simple_grouped_rels() and updated the
related comments in v24.

> from within the function?

Thanks.  Fixed by removing the agg_info parameter.

> structure." This should use "is" rather than "are"

Yes.

> generate_partitionwise_join_paths().

Good point.  Done that way in v24.

> you don't have to do the hacky stuff in is_partial_agg_memory_risky()?

Agreed.  Done in v24.

> just done: ac_info = makeNode(AggClauseInfo);

A query can specify the same Aggref expressions multiple times in the
target list.  Using lappend here can lead to duplicate partial Aggref
nodes in the targetlist of a grouped path, which is what I want to
avoid.

>  *   Construct a new RelOptInfo representing a grouped version of the input

Fixed in v24.

> if (list_length(group_clauses) == 0)

Right.  Updated in v24.

> members here.". I think that's outdated since ec_members no longer has

I think that comment is used to explain why we only scan ec_members
here.  Similar comments can be found in many other places, such as in
equivclass.c:

  /*
   * Found our match.  Scan the other EC members and attempt to generate
   * joinclauses.  Ignore children here.
   */
  foreach(lc2, cur_ec->ec_members)
  {

> that, you'd need to create another RelAggInfo to represent that?

Hmm, RelAggInfo is a per-relation structure; each grouped relation has
a valid RelAggInfo.  The apply_at field represents the set of relids
where partial aggregation is applied within the paths of this grouped
relation.  If we ever change this approach and allow the planner to
explore all join levels for placing partial aggregation, the apply_at
field will become obsolete (cf. prior to v17 patches).

I've updated the comment for apply_at to clarify that it refers to the
relids at which partial aggregation is applied.

I've also updated the comments within RelAggInfo to use one-line
style.

I retained the name of this field though.

> extern void generate_grouped_paths(PlannerInfo *root,

Nice catch!  Fixed in v24.

> onto multiple lines to make them easier to read.

One of the challenges in this patch is generating the correct target
list for each grouped relation.  So I'm kind of inclined to retain
VERBOSE in EXPLAIN.  As I recall, the output target list in the test
cases saved me several times during development when I introduced
problematic code changes.

I wrapped the long queries in v24.

- Richard

---

From: David Rowley <[email-masked]>
Date: Thu, 9 Oct 2025 00:14:39 +1300

> nodes in the targetlist of a grouped path, which is what I want to

I was getting that mixed up with list_append_unique_ptr().

>   foreach(lc2, cur_ec->ec_members)

I'd say that's also wrong. "Ignore" means not to pay attention to
something that's there. The child members are not there.

> I've also updated the comments within RelAggInfo to use one-line

The style I'd thought of had the comments on the same line as the
field. Something like struct EquivalenceClass.

>I wrapped the long queries in v24.

+-- Enable eager aggregation, which by default is disabled.
+SET enable_eager_aggregate TO on;

The above comment and command mismatch to my understanding from
looking at postgresql.conf.sample and guc_parameters.dat.

David

---

From: Robert Haas <[email-masked]>
Date: Wed, 8 Oct 2025 10:45:12 -0400

> I retained the name of this field though.

For what it's worth, I also don't like that field name. I'm not sure
what to propose instead, but I don't think apply_at is very clear.

---

From: Richard Guo <[email-masked]>
Date: Thu, 9 Oct 2025 10:48:27 +0900

> Barring any objections, I'll plan to push v23 in a couple of days.

I've pushed v24 -- thanks for all the reviews!  Now bracing for the
upcoming bug reports.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Thu, 9 Oct 2025 10:49:48 +0900

> looking at postgresql.conf.sample and guc_parameters.dat.

Right.  This GUC was disabled by default prior to v17, and this is a
leftover from that.  Will push a fix.  Thanks for pointing it out!

- Richard

---

From: Richard Guo <[email-masked]>
Date: Thu, 9 Oct 2025 10:51:03 +0900

> what to propose instead, but I don't think apply_at is very clear.

This field represents the set of relids at which partial aggregation
is applied.  So how about naming it partial_agg_designated_relids?
That feels a bit verbose, though.  How about partial_agg_relids or,
for brevity, agg_relids instead?

- Richard

---

From: Tom Lane <[email-masked]>
Date: Wed, 08 Oct 2025 22:13:28 -0400

> for brevity, agg_relids instead?

I might be missing a subtlety here, but how about
"apply_aggregation_at" or "apply_partial_agg_at"?

I don't think including "relids" in the field name adds anything,
given the field's declared type and comments.

			regards, tom lane

---

From: Richard Guo <[email-masked]>
Date: Thu, 9 Oct 2025 12:10:31 +0900

> given the field's declared type and comments.

Fair point.

'agg' seems better to me than 'aggregation' when used in a name: it's
shorter, and it's unlikely anyone would interpret it as anything other
than "aggregation".

I kind of wonder whether we need to include 'partial' in the name.
Given the context, it seems very clear that we're referring to
partial aggregation rather than final aggregation.

So I'm weighing between "apply_partial_agg_at" and "apply_agg_at".

- Richard

---

From: Antonin Houska <[email-masked]>
Date: Thu, 09 Oct 2025 07:09:00 +0200

> I've pushed v24 -- thanks for all the reviews!  Now bracing for the

Thanks for finishing this! The lack of feedback I encountered earlier made me
so frustrated that I could not find motivation to collaborate with you. I'm
happy now that my effort did not get wasted.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

---

From: Richard Guo <[email-masked]>
Date: Thu, 9 Oct 2025 16:01:28 +0900

> happy now that my effort did not get wasted.

Your efforts in the earlier versions were very important for getting
this feature done.  Thank you for your work.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Thu, 9 Oct 2025 17:07:08 +0900

> leftover from that.  Will push a fix.  Thanks for pointing it out!

I noticed an unnecessary header include in initsplan.c.  Will fix that
as well.

- Richard

---

From: Richard Guo <[email-masked]>
Date: Mon, 30 Mar 2026 12:17:03 +0900

> I noticed an unnecessary header include in initsplan.c.  Will fix that

I noticed a couple of issues that can lead to unexpected results.
I've attached two patches to fix them.

1. Eager aggregation was incorrectly checking the data type's default
collation rather than the expression's actual collation.  This allowed
columns with non-deterministic collations to be pushed down, resulting
in incorrect grouping.  Fixed by 0001.

2. Pushing aggregates containing volatile functions below a join
alters their execution count.  Fixed by 0002.

(As briefly discussed on Discord, this non-deterministic collation
issue also exists in our long-existing logic for pushing HAVING down
to WHERE.  But let's fix it for the eager aggregation first.)

- Richard

---

From: "Matheus Alcantara" <[email-masked]>
Date: Thu, 02 Apr 2026 09:18:50 -0300

> to WHERE.  But let's fix it for the eager aggregation first.)

Hi Richard,

The patches looks good to me and are working as expected. It seems very
straightforward, so I don't have any major comments.

I'm attaching some new tests that I've added to collate.icu.utf8 and
eager_aggregate regression tests during my review, fell free to include
any of them if it could be helpful or none.

--
Matheus Alcantara
EDB: https://www.enterprisedb.com
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index fbcdb7eb58c..a2dd8a34da4 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2726,6 +2726,95 @@ GROUP BY t1.id;
 
 DROP TABLE eager_agg_t1;
 DROP TABLE eager_agg_t2;
+--
+-- Test for eager aggregation with multiple columns having different collations
+--
+CREATE TABLE eager_agg_t3 (
+    id int,
+    val1 text COLLATE case_insensitive,
+    val2 text COLLATE "C"
+);
+CREATE TABLE eager_agg_t4 (
+    val1 text COLLATE case_insensitive,
+    val2 text COLLATE "C"
+);
+INSERT INTO eager_agg_t3 SELECT 1, 'a', 'x' FROM generate_series(1, 50);
+INSERT INTO eager_agg_t3 SELECT 1, 'A', 'x' FROM generate_series(1, 50);
+INSERT INTO eager_agg_t4 VALUES ('A', 'x');
+ANALYZE eager_agg_t3;
+ANALYZE eager_agg_t4;
+-- Ensure that eager aggregation is not used when grouping by a column with
+-- non-deterministic collation, even when other grouping columns have
+-- deterministic collations.
+EXPLAIN (COSTS OFF)
+SELECT t1.id, t1.val1, count(*)
+  FROM eager_agg_t3 t1
+  JOIN eager_agg_t4 t2 ON t1.val1 = t2.val1 COLLATE "C" AND t1.val2 = t2.val2
+GROUP BY t1.id, t1.val1;
+                                     QUERY PLAN                                     
+------------------------------------------------------------------------------------
+ HashAggregate
+   Group Key: t1.id, t1.val1
+   ->  Nested Loop
+         Join Filter: (((t1.val1)::text = (t2.val1)::text) AND (t1.val2 = t2.val2))
+         ->  Seq Scan on eager_agg_t4 t2
+         ->  Seq Scan on eager_agg_t3 t1
+(6 rows)
+
+-- Verify correct results (should return 1 row with count = 50)
+SELECT t1.id, t1.val1, count(*)
+  FROM eager_agg_t3 t1
+  JOIN eager_agg_t4 t2 ON t1.val1 = t2.val1 COLLATE "C" AND t1.val2 = t2.val2
+GROUP BY t1.id, t1.val1;
+ id | val1 | count 
+----+------+-------
+  1 | A    |    50
+(1 row)
+
+DROP TABLE eager_agg_t3;
+DROP TABLE eager_agg_t4;
+--
+-- Test for eager aggregation with explicit COLLATE on grouping expression
+--
+CREATE TABLE eager_agg_t5 (id int, val text COLLATE "C");
+CREATE TABLE eager_agg_t6 (val text COLLATE "C");
+INSERT INTO eager_agg_t5 SELECT 1, 'a' FROM generate_series(1, 50);
+INSERT INTO eager_agg_t5 SELECT 1, 'A' FROM generate_series(1, 50);
+INSERT INTO eager_agg_t6 VALUES ('A');
+ANALYZE eager_agg_t5;
+ANALYZE eager_agg_t6;
+-- When grouping by an expression with explicit non-deterministic COLLATE,
+-- eager aggregation should not be used even if the column's native collation
+-- is deterministic.
+EXPLAIN (COSTS OFF)
+SELECT t1.id, t1.val COLLATE case_insensitive, count(*)
+  FROM eager_agg_t5 t1
+  JOIN eager_agg_t6 t2 ON t1.val = t2.val
+GROUP BY t1.id, t1.val COLLATE case_insensitive;
+                  QUERY PLAN                   
+-----------------------------------------------
+ HashAggregate
+   Group Key: t1.id, (t1.val)::text
+   ->  Hash Join
+         Hash Cond: (t1.val = t2.val)
+         ->  Seq Scan on eager_agg_t5 t1
+         ->  Hash
+               ->  Seq Scan on eager_agg_t6 t2
+(7 rows)
+
+-- Verify correct results (should return 1 row with count = 100, since 'a' and
+-- 'A' are equal under case_insensitive collation)
+SELECT t1.id, t1.val COLLATE case_insensitive, count(*)
+  FROM eager_agg_t5 t1
+  JOIN eager_agg_t6 t2 ON t1.val = t2.val
+GROUP BY t1.id, t1.val COLLATE case_insensitive;
+ id | val | count 
+----+-----+-------
+  1 | A   |    50
+(1 row)
+
+DROP TABLE eager_agg_t5;
+DROP TABLE eager_agg_t6;
 -- virtual generated columns
 CREATE TABLE t5 (
     a int,
diff --git a/src/test/regress/expected/eager_aggregate.out b/src/test/regress/expected/eager_aggregate.out
index d1b86be3a62..2bf983d12cb 100644
--- a/src/test/regress/expected/eager_aggregate.out
+++ b/src/test/regress/expected/eager_aggregate.out
@@ -448,6 +448,26 @@ GROUP BY t1.a ORDER BY t1.a;
                      ->  Seq Scan on eager_agg_t1 t1
 (9 rows)
 
+-- Ensure eager aggregation is not applied when FILTER clause contains
+-- volatile function
+EXPLAIN (COSTS OFF)
+SELECT t1.a, avg(t2.c) FILTER (WHERE random() > 0.5)
+  FROM eager_agg_t1 t1
+  JOIN eager_agg_t2 t2 ON t1.b = t2.b
+GROUP BY t1.a ORDER BY t1.a;
+                     QUERY PLAN                      
+-----------------------------------------------------
+ GroupAggregate
+   Group Key: t1.a
+   ->  Sort
+         Sort Key: t1.a
+         ->  Hash Join
+               Hash Cond: (t2.b = t1.b)
+               ->  Seq Scan on eager_agg_t2 t2
+               ->  Hash
+                     ->  Seq Scan on eager_agg_t1 t1
+(9 rows)
+
 DROP TABLE eager_agg_t1;
 DROP TABLE eager_agg_t2;
 DROP TABLE eager_agg_t3;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 0e6b76b11b8..93c22b37727 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -1021,6 +1021,76 @@ GROUP BY t1.id;
 DROP TABLE eager_agg_t1;
 DROP TABLE eager_agg_t2;
 
+--
+-- Test for eager aggregation with multiple columns having different collations
+--
+CREATE TABLE eager_agg_t3 (
+    id int,
+    val1 text COLLATE case_insensitive,
+    val2 text COLLATE "C"
+);
+CREATE TABLE eager_agg_t4 (
+    val1 text COLLATE case_insensitive,
+    val2 text COLLATE "C"
+);
+
+INSERT INTO eager_agg_t3 SELECT 1, 'a', 'x' FROM generate_series(1, 50);
+INSERT INTO eager_agg_t3 SELECT 1, 'A', 'x' FROM generate_series(1, 50);
+INSERT INTO eager_agg_t4 VALUES ('A', 'x');
+
+ANALYZE eager_agg_t3;
+ANALYZE eager_agg_t4;
+
+-- Ensure that eager aggregation is not used when grouping by a column with
+-- non-deterministic collation, even when other grouping columns have
+-- deterministic collations.
+EXPLAIN (COSTS OFF)
+SELECT t1.id, t1.val1, count(*)
+  FROM eager_agg_t3 t1
+  JOIN eager_agg_t4 t2 ON t1.val1 = t2.val1 COLLATE "C" AND t1.val2 = t2.val2
+GROUP BY t1.id, t1.val1;
+
+-- Verify correct results (should return 1 row with count = 50)
+SELECT t1.id, t1.val1, count(*)
+  FROM eager_agg_t3 t1
+  JOIN eager_agg_t4 t2 ON t1.val1 = t2.val1 COLLATE "C" AND t1.val2 = t2.val2
+GROUP BY t1.id, t1.val1;
+
+DROP TABLE eager_agg_t3;
+DROP TABLE eager_agg_t4;
+
+--
+-- Test for eager aggregation with explicit COLLATE on grouping expression
+--
+CREATE TABLE eager_agg_t5 (id int, val text COLLATE "C");
+CREATE TABLE eager_agg_t6 (val text COLLATE "C");
+
+INSERT INTO eager_agg_t5 SELECT 1, 'a' FROM generate_series(1, 50);
+INSERT INTO eager_agg_t5 SELECT 1, 'A' FROM generate_series(1, 50);
+INSERT INTO eager_agg_t6 VALUES ('A');
+
+ANALYZE eager_agg_t5;
+ANALYZE eager_agg_t6;
+
+-- When grouping by an expression with explicit non-deterministic COLLATE,
+-- eager aggregation should not be used even if the column's native collation
+-- is deterministic.
+EXPLAIN (COSTS OFF)
+SELECT t1.id, t1.val COLLATE case_insensitive, count(*)
+  FROM eager_agg_t5 t1
+  JOIN eager_agg_t6 t2 ON t1.val = t2.val
+GROUP BY t1.id, t1.val COLLATE case_insensitive;
+
+-- Verify correct results (should return 1 row with count = 100, since 'a' and
+-- 'A' are equal under case_insensitive collation)
+SELECT t1.id, t1.val COLLATE case_insensitive, count(*)
+  FROM eager_agg_t5 t1
+  JOIN eager_agg_t6 t2 ON t1.val = t2.val
+GROUP BY t1.id, t1.val COLLATE case_insensitive;
+
+DROP TABLE eager_agg_t5;
+DROP TABLE eager_agg_t6;
+
 -- virtual generated columns
 CREATE TABLE t5 (
     a int,
diff --git a/src/test/regress/sql/eager_aggregate.sql b/src/test/regress/sql/eager_aggregate.sql
index 97e10dd7cf4..9c935ef0633 100644
--- a/src/test/regress/sql/eager_aggregate.sql
+++ b/src/test/regress/sql/eager_aggregate.sql
@@ -171,6 +171,14 @@ SELECT t1.a, avg(t2.c + random())
   JOIN eager_agg_t2 t2 ON t1.b = t2.b
 GROUP BY t1.a ORDER BY t1.a;
 
+-- Ensure eager aggregation is not applied when FILTER clause contains
+-- volatile function
+EXPLAIN (COSTS OFF)
+SELECT t1.a, avg(t2.c) FILTER (WHERE random() > 0.5)
+  FROM eager_agg_t1 t1
+  JOIN eager_agg_t2 t2 ON t1.b = t2.b
+GROUP BY t1.a ORDER BY t1.a;
+
 DROP TABLE eager_agg_t1;
 DROP TABLE eager_agg_t2;
 DROP TABLE eager_agg_t3;

---

From: Richard Guo <[email-masked]>
Date: Mon, 6 Apr 2026 12:06:44 +0900

> any of them if it could be helpful or none.

Thanks for the review.  I have added two of your test cases and
committed the patches.

- Richard

---

From: Radim Marek <[email-masked]>
Date: Fri, 29 May 2026 17:55:01 +0200

Hey Richard,

I might be out of my depth here - but while testing RegreSQL as
correctness/performance harness on PostgreSQL it picked up a problem with
the wrong-results case during eager aggregation.

It reproduces on current HEAD
(commit 2670cc298f42cd7b1c426bf7ccfb0652d8e0b347 now)
with enable_eager_aggregate enabled.

My testing environment
  - Linux aarch64, gcc 12 (Debian)
  - macOS arm64, Apple clang 21
    (PostgreSQL 19devel on aarch64-apple-darwin25.5.0)

== How to reproduce

  CREATE TEMP TABLE c(id int, country text);
  CREATE TEMP TABLE o(customer_id int);
  INSERT INTO c VALUES (1,'US'),(2,'US'),(3,'DE'),(4,'DE'),(5,'DE');
  INSERT INTO o VALUES (1),(3);   -- only customers 1 and 3 have a row in o

  SELECT c.country, count(*) AS n
  FROM c
  WHERE NOT EXISTS (SELECT 1 FROM o WHERE o.customer_id = c.id)
  GROUP BY c.country
  ORDER BY c.country;

Expected results (everywhere except master)

 country | n
---------+---
 DE      | 2
 US      | 1
(2 rows)

The actual result with enable_eager_aggregate = on (default)

 country | n
---------+---
 DE      | 0
 US      | 0
(2 rows)

With SET enable_eager_aggregate = off, the result is correct (DE=2, US=1),
as it is on PG18.

Query Plan

                                                            QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------------------
 Sort  (cost=108.19..108.69 rows=200 width=40) (actual time=0.195..0.197
rows=2.00 loops=1)
   Sort Key: c.country
   Sort Method: quicksort  Memory: 25kB
   Buffers: local hit=2
   ->  Finalize HashAggregate  (cost=98.55..100.55 rows=200 width=40)
(actual time=0.183..0.186 rows=2.00 loops=1)
         Group Key: c.country
         Batches: 1  Memory Usage: 32kB
         Buffers: local hit=2
         ->  Hash Anti Join  (cost=52.75..95.37 rows=635 width=40) (actual
time=0.177..0.179 rows=3.00 loops=1)
               Hash Cond: (c.id = o.customer_id)
               Buffers: local hit=2
               ->  Seq Scan on c  (cost=0.00..22.70 rows=1270 width=36)
(actual time=0.024..0.025 rows=5.00 loops=1)
                     Buffers: local hit=1
               ->  Hash  (cost=50.25..50.25 rows=200 width=12) (actual
time=0.145..0.146 rows=2.00 loops=1)
                     Buckets: 1024  Batches: 1  Memory Usage: 9kB
                     Buffers: local hit=1
                     ->  Partial HashAggregate  (cost=48.25..50.25 rows=200
width=12) (actual time=0.122..0.123 rows=2.00 loops=1)
                           Group Key: o.customer_id
                           Batches: 1  Memory Usage: 32kB
                           Buffers: local hit=1
                           ->  Seq Scan on o  (cost=0.00..35.50 rows=2550
width=4) (actual time=0.002..0.003 rows=2.00 loops=1)
                                 Buffers: local hit=1
 Planning Time: 0.294 ms
 Execution Time: 0.255 ms
(24 rows)

If this is already known or in progress, apologies for the noise.

---

Radim

---

From: Tender Wang <[email-masked]>
Date: Sun, 31 May 2026 19:28:37 +0800

Radim Marek <[email-masked]> 于2026年5月29日周五 23:55写道：
>
> Hey Richard,
>
> I might be out of my depth here - but while testing RegreSQL as correctness/performance harness on PostgreSQL it picked up a problem with the wrong-results case during eager aggregation.
>
> It reproduces on current HEAD (commit 2670cc298f42cd7b1c426bf7ccfb0652d8e0b347 now) with enable_eager_aggregate enabled.
>
> My testing environment
> With SET enable_eager_aggregate = off, the result is correct (DE=2, US=1), as it is on PG18.
>
> Query Plan
>
>                                                             QUERY PLAN
>  Sort  (cost=108.19..108.69 rows=200 width=40) (actual time=0.195..0.197 rows=2.00 loops=1)
>    Sort Key: c.country
>    ->  Finalize HashAggregate  (cost=98.55..100.55 rows=200 width=40) (actual time=0.183..0.186 rows=2.00 loops=1)
>          Group Key: c.country
>          ->  Hash Anti Join  (cost=52.75..95.37 rows=635 width=40) (actual time=0.177..0.179 rows=3.00 loops=1)
>                ->  Seq Scan on c  (cost=0.00..22.70 rows=1270 width=36) (actual time=0.024..0.025 rows=5.00 loops=1)
>                      Buffers: local hit=1
>                ->  Hash  (cost=50.25..50.25 rows=200 width=12) (actual time=0.145..0.146 rows=2.00 loops=1)
>                      ->  Partial HashAggregate  (cost=48.25..50.25 rows=200 width=12) (actual time=0.122..0.123 rows=2.00 loops=1)
>                            Group Key: o.customer_id
>                            ->  Seq Scan on o  (cost=0.00..35.50 rows=2550 width=4) (actual time=0.002..0.003 rows=2.00 loops=1)
>                                  Buffers: local hit=1
>  Planning Time: 0.294 ms
>  Execution Time: 0.255 ms
> (24 rows)
>
Thanks for the report. This is a bug.
When we use eager_agg, it can reduce many tuples before doing a join
on the partial agg side.
After partial agg, when we are doing a join,  the matched rows will be
significantly reduced.
This is also the effect we want to achieve from eager_agg.

But we should be careful about anti-join. Because we will ignore the
matched row. The aggregate of unmatched rows seems wrong.
And I can get the wrong results from the semi-join, too.
For example:
postgres=# CREATE TEMP TABLE c(id int, country text);
CREATE TEMP TABLE o(customer_id int);
INSERT INTO c VALUES (1,'US'),(2,'US'),(3,'DE'),(4,'DE'),(5,'DE');
INSERT INTO o VALUES (1),(3);
CREATE TABLE
CREATE TABLE
INSERT 0 5
INSERT 0 2
postgres=# insert into o values (1);
INSERT 0 1
-- correct result
postgres=#  SELECT c.country, count(*) AS n
FROM c
WHERE EXISTS (SELECT 1 FROM o WHERE o.customer_id = c.id)
GROUP BY c.country
ORDER BY c.country;
 country | n
---------+---
 DE      | 1
 US      | 1
(2 rows)

I do some hacks that make the cost of the path created in
make_grouped_join_rel() very small.
So we can get a partial agg plan, as follow:

postgres=# explain SELECT c.country, count(*) AS n
FROM c
WHERE EXISTS (SELECT 1 FROM o WHERE o.customer_id = c.id)
GROUP BY c.country
ORDER BY c.country;
                                       QUERY PLAN
-----------------------------------------------------------------------------------------
 Finalize GroupAggregate  (cost=31.56..38.32 rows=200 width=40)
   Group Key: c.country
   ->  Sort  (cost=31.56..33.15 rows=635 width=40)
         Sort Key: c.country
         ->  Hash Semi Join  (cost=1.00..2.00 rows=635 width=40)
               Hash Cond: (c.id = o.customer_id)
               ->  Seq Scan on c  (cost=0.00..22.70 rows=1270 width=36)
               ->  Hash  (cost=200.91..200.91 rows=200 width=12)
                     ->  Partial GroupAggregate  (cost=179.78..200.91
rows=200 width=12)
                           Group Key: o.customer_id
                           ->  Sort  (cost=179.78..186.16 rows=2550 width=4)
                                 Sort Key: o.customer_id
                                 ->  Seq Scan on o  (cost=0.00..35.50
rows=2550 width=4)
(13 rows)

postgres=#  SELECT c.country, count(*) AS n
FROM c
WHERE EXISTS (SELECT 1 FROM o WHERE o.customer_id = c.id)
GROUP BY c.country
ORDER BY c.country;
 country | n
---------+---
 DE      | 1
 US      | 2
(2 rows)

You can see that the count(us) has 2. Because partial agg
pre-aggregates the results for country =1.
However, for the semantics of semi-join, it returns once a match is found.

I haven't thought about it too deeply yet. Maybe we can do something
in the make_grouped_join_rel().
...
if (sjinfo->jointype == JOIN_ANTI || sjinfo->jointype == JOIN_SEMI)
    return;
...
The fixes above can temporarily resolve these issues. But it seems too strict.

-- 
Thanks,
Tender Wang

---

From: Richard Guo <[email-masked]>
Date: Mon, 1 Jun 2026 16:19:03 +0900

> > Expected results (everywhere except master)

Thanks for the report.  This is a bug.  We should never push a partial
aggregation down to a relation on the inner (RHS) side of a semi/anti
join.  A semi/anti join does not preserve its inner rows in the join
output, so a partial aggregate computed on the inner side would not
survive the join and could not be combined by the final aggregation.

> if (sjinfo->jointype == JOIN_ANTI || sjinfo->jointype == JOIN_SEMI)

That does fix the reported case, but I think it's too broad: it also
disables pushing a partial aggregate to the outer side of a semi/anti
join, which is valid.  And by the time we reach make_grouped_join_rel
the grouped relation for the inner-side relation has already been
built, so it would just go unused.

So I'd rather fix it in eager_aggregation_possible_for_relation, right
next to the existing outer-join check, by rejecting a relation that
lies on the inner side of a semijoin/antijoin.  See attached.

- Richard

---

From: Tender Wang <[email-masked]>
Date: Mon, 1 Jun 2026 15:57:39 +0800

> built, so it would just go unused.

Yes, checking only the jointype and concluding that partial agg is not allowed
will cause us to miss some optimization opportunities for the outer
side of the semi or anti-join.

> lies on the inner side of a semijoin/antijoin.  See attached.

The attached LGTM.

-- 
Thanks,
Tender Wang

---

From: Richard Guo <[email-masked]>
Date: Wed, 3 Jun 2026 10:25:01 +0900

> The attached LGTM.

Thanks for the review.  Pushed.

And thanks to Radim for the report and the well-contained repro.

- Richard