Subject: Re: Eager aggregation, take 3

From: Richard Guo <[email-masked]>
Date: Tue, 17 Dec 2024 12:42:28 +0900

On Fri, Nov 1, 2024 at 2:54 PM Richard Guo <[email-masked]> wrote:
> Perhaps we could introduce a GroupPathInfo into the Path structure to
> store common information for a grouped path, such as the location of
> the partial aggregation (which could be the relids of the relation on
> top of which we place the partial aggregation) and the estimated
> rowcount for this grouped path, similar to how ParamPathInfo functions
> for parameterized paths.  Then we should be able to compare the
> grouped paths of the same location apples to apples.  I haven’t
> thought this through in detail yet, though.

After thinking over this again, I think one difference from the
parameterized path case is that, for a parameterized path, the fewer
the required outer rels, the better, as more outer rels imply more
join restrictions.  Therefore, the number of required outer rels
serves as a criterion when comparing paths in add_path().

For a grouped path, however, we don't concern ourselves with the
location of the partial aggregation.  What matters is whether one
grouped path is preferable to another based on the current merits of
add_path().  Therefore, I think it's acceptable to compare grouped
paths for the same grouped rel, regardless of where the partial
aggregation is placed.

Note that non-grouped and grouped paths will not appear in the same
RelOptInfo.  All paths for a grouped rel are grouped paths, meaning
there is a partial aggregation node somewhere in the path tree.
Similarly, all paths for a non-grouped rel are non-grouped paths.
That is to say, it is not possible to compare a grouped path with a
non-grouped path.

Two different grouped paths for the same grouped rel can have very
different rowcount estimates, depending on where the partial
aggregation is placed in the path tree.  Therefore, for a grouped
join path, we have to calculate its rowcount estimate using its outer
and inner paths, as what we do in set_joinpath_size().  This is
similar to what we do for parameterized paths: two different
parameterized paths for the same rel can also have very different
rowcount estimates, depending on which outer rels supply the
parameters.  So we calculate the rowcount estimates for parameterized
join paths for each different parameterization in
get_parameterized_joinrel_size().

set_joinpath_size() adds a special case into final_cost_nestloop(),
final_cost_mergejoin(), and final_cost_hashjoin().  For non-grouped
paths, it adds an additional check - IS_GROUPED_REL(rel), which is
defined as

#define IS_GROUPED_REL(rel)  ((rel)->agg_info != NULL)

I doubt that this additional simple pointer check will cause general
performance regressions.

> Yeah, this patch does not get it correct here.  Basically the logic is
> that for the partial aggregation pushed down to a non-aggregated
> relation, we need to consider all columns of that relation involved in
> upper join clauses and include them in the grouping keys.  Currently,
> the patch only checks whether a column is involved in upper join
> clauses but does not verify how the column is used.  We need to ensure
> that the operator used in the join clause is at least compatible with
> the grouping operator; otherwise, the grouping operator might
> interpret the values as the same while the join operator sees them as
> different.

Hmm, I think we can prevent this issue from occurring if we ensure
that "equality implies image equality" for each grouping key used in
partial aggregation.  In such cases, if the grouping operator in
partial aggregation treats two values as equal, the join operator in
the upper join clause must also treat them as equal.

On the other hand, it’s possible that the grouping operator treats two
values as different, while the join operator treats them as equal.
This is fine, as the different partial groups will be combined during
the final aggregation.

Attached is the patch rebased on the latest master.  It refines the
theoretical justification for the correctness of this transformation
in README and commit message.  It also adds the check for image
equality for all grouping keys used in partial aggregation, and fixes
the issue reported by Jian.  It does not yet handle the RLS case
though.

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Sat, 21 Dec 2024 10:05:09 +0900

> the issue reported by Jian.  It does not yet handle the RLS case

I've looked at the RLS case.  AFAIU we want to prevent any
non-leakproof aggregation functions from being pushed down past
securityQuals.  I added a check in create_agg_clause_infos to ensure
that no aggregation is pushed down if securityQuals are present along
with any non-leakproof aggregate functions.  I know this might be
overly strict, but for now, I want to focus on the eager aggregation
transformation itself.  We can relax this restriction in subsequent
patches after this already large one.

Attached is the latest patch, which also includes some cosmetic
tweaks.  I am seeking the possibility of pushing this by the end of
January, so that I can have enough time to react to any bugs before
the feature freeze.

Thanks
Richard

---

From: jian he <[email-masked]>
Date: Thu, 9 Jan 2025 11:15:09 +0800

hi.
in create_grouping_expr_infos

        tce = lookup_type_cache(exprType((Node *) tle->expr),
                                TYPECACHE_BTREE_OPFAMILY);
        if (!OidIsValid(tce->btree_opf) ||
            !OidIsValid(tce->btree_opintype))
            return;
       ....
        /*
         * Get the operator in the btree's opfamily.
         */
        eq_op = get_opfamily_member(tce->btree_opf,
                                    tce->btree_opintype,
                                    tce->btree_opintype,
                                    BTEqualStrategyNumber);
        if (!OidIsValid(eq_op))
            return;
        eq_opfamilies = get_mergejoin_opfamilies(eq_op);
        if (!eq_opfamilies)
            return;
        btree_opfamily = linitial_oid(eq_opfamilies);

If eq_op is valid, then we don't need to call get_mergejoin_opfamilies?
since get_mergejoin_opfamilies output will be the same as tce->btree_opf.
and we already checked (tce->btree_opf) is valid.

In other words, I think eq_op is valid imply
that tce->btree_opf is the value (btree opfamily) we need.

---

From: Richard Guo <[email-masked]>
Date: Thu, 9 Jan 2025 18:27:06 +0900

> that tce->btree_opf is the value (btree opfamily) we need.

Nice catch!  Actually, we can use tce->btree_opf directly, without
needing to check its equality operator, since we know it's a btree
opfamily and it's valid.  If it were a different opfamily (such as a
hash opfamily), we would need to look up its equality operator, and
select some btree opfamily that that operator is part of.  But in this
case, that's not necessary.

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Mon, 13 Jan 2025 11:04:34 +0900

> January, so that I can have enough time to react to any bugs before

Attached is an updated version of this patch that addresses Jian's
review comments, along with some more cosmetic tweaks.  I'm going to
be looking at this patch again from the point of view of committing
it, with the plan to commit it late this week or early next week,
barring any further comments or objections.

Thanks
Richard

---

From: Robert Haas <[email-masked]>
Date: Tue, 14 Jan 2025 10:07:14 -0500

> barring any further comments or objections.

I feel this is rushed. This is a pretty big patch touching a sensitive
area of the code. I'm the only senior hacker who has reviewed it, and
I would say that I've only reviewed it pretty lightly, and that the
concerns I raised were fairly substantial. I don't think it's
customary to go from that point to commit after one more patch
revision. This really deserves to be looked at by multiple senior
hackers familiar with the planner; or at least by Tom.

My core concerns here are still what they were in the first email I
posted to the thread: it's unclear that the cost model will deliver
meaningful answers for the grouped rels, and it doesn't seem like
you've done enough to limit the overhead of the feature.

With regard to the first, I reiterate that we are in general quite bad
at having meaningful statistics for anything above an aggregate, and
this patch greatly expands how much of a query could be above an
aggregate. I felt back in August when I did my first review, and still
feel now, that when faced with a query where aggregation could be done
at any of several levels, the chances of picking the right one are not
much better than random. Why do you think otherwise?

With regard to the second, I suggested several lines of thinking back
in August that could lead to limiting the number of grouped_rels that
we create, but it doesn't really look like much of anything has
changed. We're still creating a partially grouped rel for every
baserel in the query, and every joinrel in the query. I'm not very
happy with "let's just turn it off by default" as the answer to that
concern. A lot of people won't enable the feature, which will mean it
doesn't have much value to our users, and those who do will still see
a lot of overhead. Maybe I'm wrong, but I bet with some good
heuristics the planning cost of this could be reduced by an order of
magnitude or more. If that were done, we could imagine eventually (or
maybe even immediately) enabling this by default; without that, we
still have the burden of maintaining this code and keeping it working,
but almost nobody will benefit.

+      <term><varname>enable_eager_aggregate</varname> (<type>boolean</type>)
+       <para>
+        Enables or disables the query planner's ability to partially push
+        aggregation past a join, and finalize it once all the relations are
+        joined. The default is <literal>off</literal>.

I'm a bit concerned about the naming here. I feel like we're adding an
increasing number of planner features with an increasing number of
disabling GUCs that are all a bit random. I kind of wonder if this
should be called enable_incremental_aggregate. Maybe that's worse,
because "eager" is a word we're not using for anything yet, so using
it here improves greppability and perhaps understandability. On the
other hand, the aggregate that is pushed down by this feature is
always partial (I believe) so we still need a finalize step later,
which means we're aggregating incrementally. There's some nice parity
with incremental sort, too, perhaps.

+/* The original length and hashtable of a RelInfoList */
+typedef struct
+{
+ int savelength;
+ struct HTAB *savehash;
+} RelInfoListInfo;

Both the comment and the name of the data type are completely meaningless.

+ /*
+ * Try at least sorting the cheapest path and also try
+ * incrementally sorting any path which is partially sorted
+ * already (no need to deal with paths which have presorted
+ * keys when incremental sort is disabled unless it's the
+ * cheapest input path).
+ */

This would be the fifth copy of this comment. It's not entirely this
patch's fault, of course, but some kind of refactoring or cleanup is
probably needed here.

+ * cheapest_parameterized_paths also always includes the fewest-row
+ * unparameterized path, if there is one, for grouped relations.  Different
+ * paths of a grouped relation can have very different row counts, and in some
+ * cases the cheapest-total unparameterized path may not be the one with the
+ * fewest row.

As I said back in October, this seems like mixing together in one
RelOptInfo paths that really belong to two different RelOptInfos.

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

---

From: Richard Guo <[email-masked]>
Date: Wed, 15 Jan 2025 15:58:02 +0900

> hackers familiar with the planner; or at least by Tom.

Thank you for your input.  In fact, there have been several changes
since your last review, as I mentioned in the off-list email.
However, I agree that it would be great if someone else, especially
Tom, could take a look at this patch.

> much better than random. Why do you think otherwise?

I understand that we're currently quite bad at estimating the number
of groups after aggregation.  In fact, it's not just aggregation
estimates — we're also bad at join estimates in some cases.  This is a
reality we have to face.  Here's what I think: we should be trying our
best to cost each node type as accurately as possible, and then build
the upper nodes based on those costs.  We should not conclude that,
because we are unable to accurately cost one node type, we should
avoid any cost-based optimizations above that node.

Actually, performing aggregation before joins is not a new concept;
consider JOIN_UNIQUE_OUTER/INNER, for example:

explain (costs off)
select * from t t1 join t t2 on t1.b = t2.b
where (t1.a, t1.b) in
    (select t3.a, t3.b from t t3, t t4 where t3.a > t4.a);
                      QUERY PLAN
------------------------------------------------------
 Hash Join
   Hash Cond: ((t2.b = t1.b) AND (t3.a = t1.a))
   ->  Hash Join
         Hash Cond: (t2.b = t3.b)
         ->  Seq Scan on t t2
         ->  Hash
               ->  HashAggregate
                     Group Key: t3.a, t3.b
                     ->  Nested Loop
                           Join Filter: (t3.a > t4.a)
                           ->  Seq Scan on t t3
                           ->  Materialize
                                 ->  Seq Scan on t t4
   ->  Hash
         ->  Seq Scan on t t1
(15 rows)

I believe the HashAggregate node in this plan faces the same problem
with inaccurate estimates.  However, I don't think it's reasonable to
say that, because we cannot accurately cost the Aggregate node, we
should disregard considering JOIN_UNIQUE_OUTER/INNER.

Back in August, I responded to this issue by "Maybe we can run some
benchmarks first and investigate the regressions discovered on a
case-by-case basis".  In October, I ran the TPC-DS benchmark at scale
10 and observed that eager aggregation was applied in 7 queries, with
no notable regressions.  In contrast, Q4 and Q11 showed performance
improvements of 3–4 times.  Please see [1].

> but almost nobody will benefit.

Actually, I introduced the EAGER_AGGREGATE_RATIO mechanism in October
to limit the planning effort for eager aggregation.  For more details,
please see [2].

And I don't think it's correct to say that we create a partially
grouped rel for every baserel and every joinrel.  This patch includes
a bunch of logic to determine whether it's appropriate to create a
grouped rel for a base or join rel.  Furthermore, with the
EAGER_AGGREGATE_RATIO mechanism, even if creating a grouped rel is
possible, we will skip it if the grouped paths are considered not
useful.  All of these measures help reduce the number of grouped
paths as well as the grouped relations in many cases where eager
aggregation would not help a lot.

Based on the TPC-DS benchmark results, I don't see "a lot of overhead"
in the planning cost, at least for the 7 queries where eager
aggregation is applied.  As I said in [1], "For the planning time, I
do not see notable regressions for any of the seven queries".  In
fact, I initially thought that we might consider enabling this by
default, given the positive benchmark results, but I just couldn't
summon the courage to do it.  Perhaps we should reconsider enabling it
by default, so users can benefit from the new feature and help
identify any potential bugs.

> with incremental sort, too, perhaps.

As I mentioned in [3], the name "Eager Aggregation" is inherited from
the paper "Eager Aggregation and Lazy Aggregation" [4], from which
many of the ideas in this feature are derived.  Personally, I like
this name a lot, but I'm open to other names if others find it
unreasonable.

> Both the comment and the name of the data type are completely meaningless.

Thanks.  Will fix the comment and the name for this data type.

> patch's fault, of course, but some kind of refactoring or cleanup is

Agreed.  However, I think it would be better to refactor this in a
separate patch.  This issue also exists on master, and I'd prefer to
avoid introducing such refactors in this already large patch.

> RelOptInfo paths that really belong to two different RelOptInfos.

I understand that you said about the design in October where
"PartialAgg(t1 JOIN t2) and t1 JOIN PartialAgg(t2) get separate
RelOptInfos", because "it's less clear whether it's fair to compare
across the two categories".  I've shared my thoughts on this in [5].

Furthermore, even if we separate these grouped paths into two
different RelOptInfos, we still face the issue that "different paths
of a grouped relation can have very different row counts", and we need
a way to handle this.  One could argue that we can separate the
grouped paths where partial aggregation is placed at different
locations into different RelOptInfos, but this would lead to an
explosion in the number of RelOptInfos for grouped relations as we
climb up the join tree.  I think this is neither realistic nor
necessary.

[1] https://postgr.es/m/[email-masked]
[2] https://postgr.es/m/CAMbWs48OS3Z0G5u3fhar1=H_ucmEcUaX0tRUNpcLQxHt=[email-masked]
[3] https://postgr.es/m/[email-masked]
[4] https://www.vldb.org/conf/1995/P345.PDF
[5] https://postgr.es/m/[email-masked]

Thanks
Richard

---

From: Robert Haas <[email-masked]>
Date: Wed, 15 Jan 2025 09:40:42 -0500

> avoid any cost-based optimizations above that node.

Well, I agree with that last sentence, for sure. But I don't think
it's true that the situations with joins and aggregates are
comparable. We are much better able to estimate the number of rows
that will come out of a join than we are to estimate the number of
rows that come out of an aggregate. It's certainly true that in some
cases we get join estimates badly wrong, and I'd like to see us do
better there, but our estimates of the number of distinct values that
exist in a column are the least reliable part of our statistics system
by far.

Also, we look at the underlying statistics for a column variable even
after joins and aggregates and assume (not having any other
information) that the distribution after that operation is likely to
be similar to the distribution before that operation. Consider a table
A with columns x and y. Let's say x is a unique ID and y is a
dependent value with some distribution over a finite range of
possibilities (e.g. a person's age). If we join table A to some other
table B on A.x = B.x and filter out some of the rows via that join,
the distribution of values in column y is likely to be altered. If the
rows are removed at random, the original distribution will prevail,
but often it won't be random and so the distribution will change in a
way we can't predict. However, guessing pre-join distribution of A.y
is still prevails isn't crazy, and it's better than assuming we can
say nothing about the distribution.

But now let's say that after joining to B, we perform an aggregation
operation, computing the minimum value of A.y for each value of B.z. A
this point, we have no usable statistics for either output column. The
result must be unique on B.z, and the distribution of MIN(A.y) is
going to be entirely different from the distribution of B.y. Any
future joins that we perform here will have to be estimated without
any MCVs, which is going to reduce the accuracy of the estimation by a
lot. In summary, the join makes relying on our MCV information less
likely to be accurate, but the aggregate makes it impossible to rely
on our MCV information at all. In terms of the accuracy of our
results, that is a lot worse.

> should disregard considering JOIN_UNIQUE_OUTER/INNER.

Fair point.

> improvements of 3–4 times.  Please see [1].

I had forgotten about that, and again, fair point, but I'm concerned
that it might not be a broad enough base of queries to test against.
(7 isn't a very large number.)

> to limit the planning effort for eager aggregation.  For more details,

OK, I missed this, but...

> aggregation would not help a lot.

...it looks to me like EAGER_AGGREGATE_RATIO is used to set the
RelAggInfo's agg_useful field, which seems like it happens after the
RelOptInfo has already been created. I had been looking for something
that would avoid creating the RelOptInfo in the first place and I
didn't see it.

> identify any potential bugs.

If you're going to commit this, I think it would be a good idea to
enable it by default at least for now. If there are problems, it's
better to find out about them sooner rather than later. If they are
minor they can be fixed; if they are major, we can consider whether it
is better to fix them, disable the feature by default, or revert. We
can add an open item to reconsider the default setting during beta.

> climb up the join tree.  I think this is neither realistic nor

It's possible you're right, but it does make me nervous. I do agree
that making the number of RelOptInfos explode would be really bad.

---

From: Richard Guo <[email-masked]>
Date: Thu, 16 Jan 2025 17:18:18 +0900

> exist in a column are the least reliable part of our statistics system

I totally understand that the situation with joins is better than with
aggregates, which is why I said that we're also bad at join estimates
"in some cases" - especially in the cases where we fall back to use
default selectivity estimates.  A simple example:

create table t1 (a int, b int);
create table t2 (a int, b int);

insert into t1 select i, i from generate_series(1,1000)i;
insert into t2 select i, i from generate_series(1000, 1999)i;

analyze t1, t2;

explain analyze select * from t1 join t2 on t1.a > t2.a;

And here is what I got:

 Nested Loop  (cost=0.00..15032.50 rows=333333 width=16)
              (actual time=392.841..392.854 rows=0 loops=1)

If this t1/t2 join is part of a larger SELECT query, I think the cost
estimates for the upper join nodes would likely be quite inaccurate.

> (7 isn't a very large number.)

Yeah, I know 7 is not a large number, but this is the result I got
from running the TPC-DS benchmark.  For the remaining 92 queries in
the benchmark, either the logic in this patch determines that eager
aggregation is not applicable, or the path with eager aggregation is
not the optimal one.  I'd be more than happy if a benchmark query
showed significant performance regression, so it would provide an
opportunity to investigate how the cost estimates are negatively
impacting the final plan and explore ways to avoid or improve that.
If anyone can provide such a benchmark query, I'd be very grateful.

Perhaps this is another reason why we should enable this feature by
default, so we can identify such regression issues sooner rather than
later.

> that would avoid creating the RelOptInfo in the first place and I

Well, from the perspective of planning effort, what really matters is
whether the RelOptInfo for the grouped relation is added to the
PlannerInfo, as it is only then available for further joining in the
join search routine, not whether the RelOptInfo is built or not.
Building the RelOptInfo for a grouped relation is simply a makeNode
call followed by a flat copy; it doesn't require going through the
full process of determining its target list, or constructing its
restrict and join clauses, or calculating size estimates, etc.

Now, let's take a look at how the EAGER_AGGREGATE_RATIO mechanism is
used.  As you mentioned, EAGER_AGGREGATE_RATIO is used to set the
agg_useful field of the RelAggInfo.  For a base rel where we've
decided that aggregation can be pushed down, if agg_useful is false,
we skip building the grouped relation for it in the first place, not
to mention adding the grouped relation to the PlannerInfo.  For a join
rel where aggregation can be pushed down, if agg_useful is false, we
will create a temporary RelOptInfo for its grouped relation, but we
only add this RelOptInfo to the PlannerInfo if we can generate any
grouped paths by joining its input relations.  We could easily modify
make_grouped_join_rel() to create this temporary RelOptInfo only when
needed, but I'm not sure if that's necessary, since I don't have data
to suggest that the creation of this temporary RelOptInfo is a factor
in causing planning regressions.

> can add an open item to reconsider the default setting during beta.

Agreed.  And I like the suggestion of adding an open item about the
default setting during beta.

> that making the number of RelOptInfos explode would be really bad.

Based on my explanation in [1], I think it's acceptable to compare
grouped paths for the same grouped rel, regardless of where the
partial aggregation is placed.

I fully understand that I could be wrong about this, but I don't think
it would break anything in regular planning (i.e., planning without
eager aggregation).  We would never compare a grouped path with a
non-grouped path during scan/join planning.  As far as I can see, the
only consequence in that case would be that we might fail to select
the optimal grouped path and miss out on fully leveraging the benefits
of eager aggregation.

Back in November, I considered the possibility of introducing a
GroupPathInfo into the Path structure to store the location of the
partial aggregation as well as the estimated rowcount for this grouped
path, similar to how ParamPathInfo functions for parameterized paths.
However, after some exploration, I determined that this was
unnecessary.

But in any case, I don't think it's an option to separate the grouped
paths of the same grouped relation into different RelOptInfos based on
the location of the partial aggregation within the path tree.

[1] https://postgr.es/m/[email-masked]

Thanks
Richard

---

From: Tom Lane <[email-masked]>
Date: Thu, 16 Jan 2025 16:40:20 -0500

I'm very sorry for not having had any time to look at this patch
before --- it's been on my radar screen for awhile, but $LIFE has
been rather demanding lately.

Anyway, I've now read through the mail thread and portions of the
v16 patch, and I have to concur with Robert's qualms about whether
this is ready.  A few observations:

* The README addition, and the basically identical text in the
commit message, don't even provide a reason to believe that the
transformation is correct let alone that it will result in faster
execution.  I don't understand why it's so hard to provide a solid
correctness argument.  This work was supposedly based on an academic
paper; surely that paper must have included a correctness proof?
PG might need a few refinements, like being specific about what we
expect from the equality operators.  But an EXPLAIN plan is not an
argument.

* As for the performance aspect, we're given

 Finalize HashAggregate
   Group Key: a.i
   ->  Nested Loop
         ->  Partial HashAggregate
               Group Key: b.j
               ->  Seq Scan on b
         ->  Index Only Scan using a_pkey on a
               Index Cond: (i = b.j)

As far as I can see, this will require aggregation to be performed
across every row of "b", whereas the naive way would have aggregated
across only rows having join partners in "a".  If most "b" rows lack
a join partner then this will be far slower than the naive way.
I do see that it can be better if most "b" rows have multiple join
partners, because we'll re-use partial aggregation results instead
of (effectively) recalculating them.  But the README text makes it
sound like this is an unconditional win, which is not the right
mindset.  (In fact, in this specific example where a.i is presumed
unique, how's it a win at all?)

* I'm also concerned about what happens with aggregates that can have
large partial-aggregation values, such as string_agg().  With the
existing usage of partial aggregation for parallel queries, it's
possible to be confident that there are not many partial-aggregation
values in existence at the same time.  I don't think that holds for
pushed-down aggregates: for example, I wouldn't be surprised if the
planner chooses a join plan that requires stuffing all those values
into a hash table, or "materializes" the output of the partial
aggregation step.  Do we have logic that will avoid blowing out
memory during such queries?

* I am just as worried as Robert is about the notion of different
paths for the same RelOptInfo having different rowcount estimates.
That is an extremely fundamental violation of basic planner
assumptions.  We did bend it for parameterized paths by restating
those assumptions as (from optimizer/README):

  To keep cost estimation rules relatively simple, we make an implementation
  restriction that all paths for a given relation of the same parameterization
  (i.e., the same set of outer relations supplying parameters) must have the
  same rowcount estimate.  This is justified by insisting that each such path
  apply *all* join clauses that are available with the named outer relations.

I don't see any corresponding statement here, and it's not clear
to me that the point has been thought through adequately.

Another aspect that bothers me is that a RelOptInfo is understood
to contain a bunch of paths that all yield the same data (the same
set of columns), and it seems like that might not be the case here.
Certainly partially-aggregated paths will output something different
than unaggregated ones, but mightn't different join orders mutate the
column set even further?

I think that we might be better off building a separate RelOptInfo for
each way of pushing down the aggregates, in order to preserve the
principle that all the paths in any one RelOptInfo have the same
output.  This'll mean more RelOptInfos, but not more paths, so
I doubt it adds that much performance overhead.

> However, after some exploration, I determined that this was

Why did you determine that was unnecessary?  The principal function
of ParamPathInfo IMV is to ensure that we use exactly the same
rowcount estimate for all the paths that should have the same
estimate, and that problem seems to exist here as well.  If you
don't have a forcing mechanism then paths' estimates will diverge
as a result of things like different roundoff errors in different
join sequences.

Anyway, I agree with Robert that this isn't ready.  I don't feel
that I can even review it adequately without a lot better internal
documentation, specifically a clearer statement of what query shapes
the optimization applies to and what's the rationale for the
transformation being correct.  The commentary in pathnodes.h for the
new data structures is likewise so skimpy as to be near useless.

			regards, tom lane

---

From: Richard Guo <[email-masked]>
Date: Fri, 17 Jan 2025 21:19:43 +0900

> expect from the equality operators.  But an EXPLAIN plan is not an

Thank you for taking a look at this patch!

In README, I provided the justification for the correctness of this
transformation as follows:

  For the partial aggregation that is pushed down to a non-aggregated
  relation, we need to consider all expressions from this relation that
  are involved in upper join clauses and include them in the grouping
  keys, using compatible operators.  This is essential to ensure that an
  aggregated row from the partial aggregation matches the other side of
  the join if and only if each row in the partial group does.  This
  ensures that all rows within the same partial group share the same
  'destiny', which is crucial for maintaining correctness.

I believed that this explanation would make it clear why this
transformation is correct.

Yeah, this work implements one of the transformations introduced in
paper "Eager Aggregation and Lazy Aggregation".  In the paper, Section
4 presents the formalism, Section 5 proves the main theorem, and
Section 6 introduces corollaries related to this specific
transformation.  I'm just not sure how to translate these theorems and
corollaries into natural language that would be suitable to be
included in the README.  I can give it another try if you find the
above justification not clear enough, but it would be really helpful
if I could get some assistance with this.

And I'd like to clarify that the EXPLAIN plan included in the README
is only meant to illustrate how this transformation looks like, and is
not intended to serve as an argument for its correctness.

> across only rows having join partners in "a".

Yes, that's correct.

> If most "b" rows lack

No, this is not correct.  The partial aggregation may reduce the
number of input rows to the join, and the resulting data reduction
could justify the cost of performing the partial aggregation.  As an
example, please consider:

create table t1 (a int, b int, c int);
create table t2 (a int, b int, c int);

insert into t1 select i%3, i%3, i from generate_series(1,1000000)i;
insert into t2 select i%3+3, i%3+3, i from generate_series(1,1000000)i;

analyze t1, t2;

explain analyze
select sum(t2.c) from t1 join t2 on t1.b = t2.b group by t1.a;

So for this query, most (actually all) t2 rows lack a join partner.

Running it with and without eager aggregation, I got (best of 3):

-- with eager aggregation
 Execution Time: 496.856 ms

-- without eager aggregation
 Execution Time: 1723.844 ms

> of (effectively) recalculating them.

Not only because we'll re-use partial aggregation results, but also
(and perhaps more importantly) because the number of input rows to the
join could be significantly reduced.

> But the README text makes it
> mindset.

I'm sorry if the README text gives that impression.  The README says:

 If the partial aggregation on table B significantly reduces the number
 of input rows, the join above will be much cheaper, leading to a more
 efficient final plan.

Perhaps I should use "could" or "might" instead of "will" to make it
less misleading.

But as you can see from the implementation, the decision is entirely
based on cost, not on rules.  There is no part of the code that ever
assumes this transformation is an unconditional win.

> (In fact, in this specific example where a.i is presumed

It seems to me that whether it's a win depends on whether b.j is a
column with low cardinality (i.e., relatively few unique values).  I
don't really see how a.i being unique would change that.  Please
see the example below:

create table a (i int primary key, x int);
create table b (j int, y int);

insert into a select i, i%3 from generate_series(1,10000)i;
insert into b select i%3, i from generate_series(1,10000)i;

analyze a, b;

set enable_eager_aggregate to off;

EXPLAIN (ANALYZE, COSTS OFF)
SELECT a.i, avg(b.y)
FROM a JOIN b ON a.i > b.j
GROUP BY a.i;
                                            QUERY PLAN
--------------------------------------------------------------------------------------------------
 HashAggregate (actual time=100257.254..100268.841 rows=10000 loops=1)
   Group Key: a.i
   Batches: 1  Memory Usage: 2193kB
   Buffers: shared hit=133
   ->  Nested Loop (actual time=2.629..40849.630 rows=99990000 loops=1)
         Buffers: shared hit=133
         ->  Seq Scan on b (actual time=0.450..10.066 rows=10000 loops=1)
               Buffers: shared hit=45
         ->  Memoize (actual time=0.002..0.752 rows=9999 loops=10000)
               Cache Key: b.j
               Cache Mode: binary
               Hits: 9997  Misses: 3  Evictions: 0  Overflows: 0
Memory Usage: 1055kB
               Buffers: shared hit=88
               ->  Index Only Scan using a_pkey on a (actual
time=0.752..8.100 rows=9999 loops=3)
                     Index Cond: (i > b.j)
                     Heap Fetches: 0
                     Buffers: shared hit=88
 Planning Time: 1.681 ms
 Execution Time: 100273.011 ms
(19 rows)

set enable_eager_aggregate to on;

EXPLAIN (ANALYZE, COSTS OFF)
SELECT a.i, avg(b.y)
FROM a JOIN b ON a.i > b.j
GROUP BY a.i;
                                         QUERY PLAN
--------------------------------------------------------------------------------------------
 Finalize HashAggregate (actual time=77.701..90.680 rows=10000 loops=1)
   Group Key: a.i
   Batches: 1  Memory Usage: 2193kB
   Buffers: shared hit=133
   ->  Nested Loop (actual time=27.586..52.352 rows=29997 loops=1)
         Buffers: shared hit=133
         ->  Partial HashAggregate (actual time=27.408..27.419 rows=3 loops=1)
               Group Key: b.j
               Batches: 1  Memory Usage: 24kB
               Buffers: shared hit=45
               ->  Seq Scan on b (actual time=0.173..3.767 rows=10000 loops=1)
                     Buffers: shared hit=45
         ->  Index Only Scan using a_pkey on a (actual
time=0.108..5.277 rows=9999 loops=3)
               Index Cond: (i > b.j)
               Heap Fetches: 0
               Buffers: shared hit=88
 Planning Time: 1.739 ms
 Execution Time: 93.003 ms
(18 rows)

There is a performance improvement of ~1000 times, even though a.i is
unique.

# select 100273.011/93.003;
       ?column?
-----------------------
 1078.1696396890422890
(1 row)

(I used 'a.i > b.j' instead of 'a.i = b.j' to make the performance
difference more noticeable.  I believe this is fine, as it doesn't
undermine the fact that a.i is unique.)

> memory during such queries?

Good point!  Thank you for bringing this up.  I hadn't considered it
before, and it seems no one else has raised this issue.  I'll look
into it.

> I doubt it adds that much performance overhead.

Hmm, IIUC, this means that we would separate the grouped paths of the
same grouped relation into different RelOptInfos based on the location
of the partial aggregation within the path tree.  Let's define the
"location" as the relids of the relation on top of which we place the
partial aggregation.  For grouped relation {A B C D}, if we perform
some aggregation on C, we would end up with 8 diffent grouped paths:

{A B D PartialAgg(C)}
{B D PartialAgg(A C)}
{A D PartialAgg(B C)}
{A B PartialAgg(D C)}
{D PartialAgg(A B C)}
{B PartialAgg(A D C)}
{A PartialAgg(B D C)}
{PartialAgg(A B D C)}

That means we would need to create 8 RelOptInfos for this grouped
relation.  If my math doesn't fail me, for a relation containing n
base rels, we would need to create 2^(n-1) different RelOptInfos.

When building grouped relation {A B C D E} by joining {A B C D} with
{E}, we would need to call make_grouped_join_rel() 8 times, each time
joining {E} with one of the 8 RelOptInfos mentioned above.  And at
last, considering other join orders such as joining {A B C E} with
{D}, this new grouped relation would end up with 16 new RelOptInfos.

And then we proceed with building grouped relation {A B C D E F}, and
end up with 32 new RelOptInfos, and this process continues...

It seems to me that this doesn't only result in more RelOptInfos, it
can also lead to more paths.  Consider two grouped paths, say P1 and
P2, for the same grouped relation, but with different locations of the
partial aggregation.  Suppose P1 is cheaper, at least as well ordered,
generates no more rows, requires no outer rels not required by P2, and
is no less parallel-safe.  If these two paths are kept in the same
RelOptInfo, P2 will be discarded and not considered in further
planning.  However, if P1 and P2 are separated into different
RelOptInfos, and P2 happens to have survived the add_path() tournament
for the RelOptInfo it is in, then it will be considered in subsequent
planning steps.

So in any case, this doesn't seem like a feasible approach to me.

I also have some thoughts on grouped paths and parameterized paths,
but I've run out of time for today.  I'll send a separate email.

I'm really glad you're taking a look at this patch.  Thank you!

Thanks
Richard

---

From: Robert Haas <[email-masked]>
Date: Fri, 17 Jan 2025 16:16:19 -0500

> estimates for the upper join nodes would likely be quite inaccurate.

That's definitely true. However, the question is not whether the
planner has problems today (it definitely does) but whether it's OK to
make this change without improving our ability to estimate the effects
of aggregation operations. I understand that you (quite rightly) don't
want to get sucked into fixing unrelated planner problems, and I'm
also not sure to what extent these problems are actually fixable.
However, major projects sometimes require such work. For instance,
commit 5edc63bda68a77c4d38f0cbeae1c4271f9ef4100 was motivated by the
discovery that it was too easy to get a Parallel Bitmap Heap Scan plan
even when it wasn't best. The fact that the costing wasn't right
wasn't the fault of parallel query, but parallel query still needed to
do something about it to get good results.

> If anyone can provide such a benchmark query, I'd be very grateful.

Yes, having more people test this and look for regressions would be
quite valuable.

> restrict and join clauses, or calculating size estimates, etc.

That's probably mostly true, but the overhead of memory allocations in
planner routines is not trivial. There are previous cases of changing
things or declining to change this purely on the number of palloc
cycles involved.

> eager aggregation).

I think you might be taking too narrow a view of the problem. As Tom
says, the issue is that this breaks a bunch of assumptions that hold
elsewhere. One place that shows up in the patch is in the special-case
logic you've added to set_cheapest(), but I fear that won't be the end
of it. It seems a bit surprising to me that you didn't also need to
adjust add_path(), for example. Even if you don't, there's lots of
places that rely on the assumption that all paths for a RelOptInfo are
returning the same set of rows. If it turns out that a bunch of those
places need to be adjusted to work with this, then the code could
potentially end up quite messy, and that might also have performance
consequences, even when this feature is disabled. Many of the code
paths that deal with paths in the planner are quite hot.

To say that another way, I'm not so much worried about the possibility
that the patch contains a bug. Patches contain bugs all the time and
we can just fix them. It's not wonderful, but that's how software
development goes. What I am worried about is whether the architecture
is right. If we go with one RelOptInfo when the "right answer" is
many, or for that matter if we go with many when the right answer is
one, those are things that cannot be easily and reasonably patched
post-commit, and especially not post-release.

---

From: Richard Guo <[email-masked]>
Date: Sun, 19 Jan 2025 21:53:05 +0900

> do something about it to get good results.

Yeah, it's true that we have problems in aggregate estimates today.
And it has been the case for a long time.  In the past, we made some
improvements in this area, such as in 84f9a35e3, where we adapted a
new formula that is based on the random selection probability,
inspired by two papers from Yao and Dell'Era.  But we still have
problems with aggregate estimates.  I'm not sure when we could fix
these problems, but I doubt that it will happen in the near future.
(Sorry to be pessimistic.)

If, at last, the conclusion of this discussion is that we should not
apply this change until we fix those problems in aggregate estimates,
I'd be very sad.  This conclusion is absolutely correct, for sure, in
an ideal world, but in the real world, it feels like a death sentence
for this patch, and for any future patches that attempt to apply some
optimizations above aggregate nodes - unless, of course, the day
arrives when we finally fix those aggregate estimate problems, which
doesn't seem likely in the near future.

And if that's the case, can I then argue that the same principle
should apply to joins?  Specifically, should we refrain from applying
any optimizations above join nodes until we've fixed the join estimate
problems, particularly in cases where we fall back on default
selectivity estimates?

Please do not get me wrong.  I'm not saying that we should not fix the
problems in our current aggregate estimates.  I think, as I said
previously, that the realistic approach is to first identify some
real-world queries where this patch causes significant performance
regressions.  This would give us the opportunity to investigate these
regressions and understand how the bad cost estimates contributed to
them.  From there, we could figure out where to start fixing the cost
estimates.  And if we find that the problem is not entirely fixable,
we could then explore the possibility of introducing new heuristics to
avoid the performance regressions as much as possible.  In my opinion,
it's not very possible to make cost estimation perfect in all cases.
In a sense, cost estimation is an art of compromise.

I believe this is also the approach that commit 5edc63bda followed.
First, it was found that Bitmap Heap Scans caused performance
regressions in many TPCH queries in cases where work_mem was low.
Then, this issue was thoroughly discussed, and eventually it was
figured out that the impact of lossy pages needed to be accounted for
when estimating the cost of bitmap scans, which became 5edc63bda.

> things or declining to change this purely on the number of palloc

Hmm, I think you are right.  I will modify make_grouped_join_rel() to
create the RelOptInfo for a grouped join relation only if we can
generate any grouped paths by joining its input relations.

> paths that deal with paths in the planner are quite hot.

Yeah, one of the basic assumptions in the planner is that all paths
for a given RelOptInfo return the same set of rows.  One exception
to this is parameterized paths.  As an example, please consider:

create table t (a int, b int);
create table t3 (a int, b int);

insert into t select i, i from generate_series(1,1000)i;
insert into t3 select i, i from generate_series(1,1000)i;

create index on t3(a, b);
analyze t, t3;

explain (costs off)
select * from t t1 join t t2 on true join t3 on t3.a > t1.a and t3.b > t2.b;

With gdb, I found the following 4 paths in the pathlist of RelOptInfo
of {t3}:

   {INDEXPATH
   :path.pathtype 341
   :parent_relids (b 4)
   :required_outer (b 1 2)
   :path.parallel_aware false
   :path.parallel_safe true
   :path.parallel_workers 0
   :path.rows 111
   :path.disabled_nodes 0
   :path.startup_cost 0.275
   :path.total_cost 4.755000000000001

   {INDEXPATH
   :path.pathtype 341
   :parent_relids (b 4)
   :required_outer (b 1)
   :path.parallel_aware false
   :path.parallel_safe true
   :path.parallel_workers 0
   :path.rows 333
   :path.disabled_nodes 0
   :path.startup_cost 0.275
   :path.total_cost 6.1425

   {INDEXPATH
   :path.pathtype 341
   :parent_relids (b 4)
   :required_outer (b 2)
   :path.parallel_aware false
   :path.parallel_safe true
   :path.parallel_workers 0
   :path.rows 333
   :path.disabled_nodes 0
   :path.startup_cost 0.275
   :path.total_cost 11.145

   {PATH
   :pathtype 338
   :parent_relids (b 4)
   :required_outer (b)
   :parallel_aware false
   :parallel_safe true
   :parallel_workers 0
   :rows 1000
   :disabled_nodes 0
   :startup_cost 0
   :total_cost 15

None of them are returning the same set of rows.  This is fine because
we have revised the assumption to that all paths for a RelOptInfo of
the same parameterization return the same set of rows.  That is to
say, it's OK that paths for the same RelOptInfo return different sets
of rows if they have different parameterizations.

Now we have the grouped paths.  I had previously considered further
revising this assumption to that all paths for a RelOptInfo of the
same parameterization and the same location of partial aggregation
return the same set of rows.  That's why, back in November, I proposed
the idea of introducing a GroupPathInfo into the Path structure to
store the location of the partial aggregation and the estimated
rowcount for each grouped path, similar to how ParamPathInfo functions
for parameterized paths.

However, I gave up on this idea in December after realizing an
important difference from the parameterized path case.  For a
parameterized path, the fewer the required outer rels, the better, as
more outer rels imply more join restrictions.  In other words, the
number of required outer rels is an important factor when comparing
two paths in add_path().  For a grouped path, however, the location of
partial aggregation does not impose such restrictions for further
planning.  As long as one grouped path is cheaper than another based
on the current merits of add_path(), we don't really care where the
partial aggregation is placed within the path tree.

I can take up the idea of GroupPathInfo again.  Before I start
implementing it (which is not trivial), I'd like to hear others'
thoughts on this approach - whether it's necessary and whether this is
the right direction to pursue.

> post-commit, and especially not post-release.

Fair point.  We should make sure the architecture of this patch is
solid before committing it.

Regarding whether we should use a single RelOptInfo or separate
RelOptInfos for the same grouped relation: If we choose to separate
the grouped paths of the same grouped relation into different
RelOptInfos based on the location of the partial aggregation within
the path tree, then, based on my calculation from the previous email,
for a relation containing n base rels, we would need to create 2^(n-1)
different RelOptInfos, not to mention that this can also lead to more
paths.  I still struggle to see how this is feasible.  Could you
please elaborate on why you believe this is a viable option?

Thanks
Richard

---

From: Robert Haas <[email-masked]>
Date: Mon, 20 Jan 2025 11:28:17 -0500

> doesn't seem likely in the near future.

Well, such conclusions should be based on evidence. So far, the
evidence you've presented suggests that the optimization works, so
there's no reason to leap to the conclusion that we shouldn't move
forward. On the other hand, the amount of evidence you've presented
does not seem to me to be all that large. And I'm not sure that you've
gone looking for adversarial cases.

> problems, particularly in cases where we fall back on default

I am having a hard time figuring out how to write back to this. I
mean, I don't think that what you write here is a serious proposal,
and I think you already know that I was not proposing any such thing.
But it upsets me that you think that this hypothetical argument is
equivalent to the ones I've actually been making. Apparently, you
consider my concerns quite groundless and foolish.

> to this is parameterized paths.

Good point. I had not considered this parallel.

> rowcount for each grouped path, similar to how ParamPathInfo functions

Interesting.

> the right direction to pursue.

Yes, I would, too. Tom, do you have any thoughts on this point? Anybody else?

An advantage of this approach could be that it would avoid any
explosion in the number of RelOptInfo structures, since presumably all
the partially aggregated paths could be attached to the same
RelOptInfo as the unaggregated paths, just with a GroupPathInfo to
mark them as partially aggregated. I have to admit that I'm not sure
it was the right idea to mix parameterized and unparameterized paths
in the same path list, and I'm even less sure that it would be a good
idea to mix in partially-aggregated paths. That's because a
parameterized path behaves like a regular path with a join
order/method restriction: as long as we only create valid joins from
parameterized paths, we'll eventually end up with unparameterized
paths without doing anything else. A partially aggregated path behaves
more like a partial path, which requires a Gather or Gather Merge node
to terminate parallelism. Likewise, a partially aggregated path will
require a FinalizeAggregate step to complete the aggregation. Maybe
that's the wrong way of thinking about it, though, since the
FinalizeAggregate node must (I think) go at the top of the join tree,
whereas a Gather can go anywhere.

I felt it best when implementing parallel query to put partial paths
into a separate list, rather than mixing them into the regular path
list. I am vaguely under the impression that Tom thinks that was a
poor decision on my part. And I can sort of see that there is a
problem brewing here. If we handled this case like that one, then we'd
go from 2 lists to 4: normal paths, paths needing a FinalizeAggregate,
paths needing a Gather(Merge), paths needing both. And if we handled
one more future thing in the same way, then the number of combinations
doubles again to 8. Clearly, that way lies madness. On the other hand,
there's another kind of madness in thinking that we can just stick a
whole bunch of paths that are different from each other in an
increasing number of ways into a single path list and suffer no
adverse consequences. The growing complexity of add_path() is one
fairly obvious one.

So I don't quite know which way to jump here. It now seems to me that
we have three similar features with three different designs.
Parameterization added non-comparable paths to the same path list;
parallel query added them to a different path list in the same
RelOptInfo; and this patch currently adds them a separate RelOptInfo.
That's quite a bit of diversity. Really, if we wanted to stick
strictly to the idea of paths associated with the same RelOptInfo
being directly comparable, then parameterization should have spawned a
separate RelOptInfo for each workable parameterization, but that
wasn't done, possibly (though I'm not sure) for the same reasons that
you don't want to do it here.

> please elaborate on why you believe this is a viable option?

I agree that creating an exponential number of RelOptInfos is not
going to work out well. I haven't been quite as certain as you seem to
be that it's an unavoidable reality, but maybe it is. For instance, my
intuition is that if PartialAgg(t1) JOIN t2 and PartialAgg(t1 JOIN t2)
produce very different numbers of rows, we could probably just take
the one with the smaller row count regardless of cost, because the
whole selling point of this optimization is that we reduce the number
of rows that are being fed to higher level plan nodes. I don't quite
see how it can make sense to keep a less costly path that produces
more rows on the theory that maybe it's going to work out better later
on. Why is the path cheaper, after all? It feels like the savings must
come from not reducing the row count so much, but that is a cost we're
going to have to repay at a higher plan level. Moreover, we'll be
repaying it with interest, because more rows will have filtered
through every level of plan over which we postponed partial
aggregation.

I admit it's not so clear-cut when the row counts are close. If
PartialAgg(t1 JOIN t2) JOIN t3 has a very similar to PartialAgg(t1
JOIN t3) JOIN t2, can we categorically pick whichever one has the
lower row count and forget about the other? I'm not sure. But I have
an uncomfortable feeling that if we can't, we're going to have an
explosion in the number of paths we have to generate even if we avoid
an explosion in the number of RelOptInfos we generate.

For example, consider:

SELECT ... FROM fact f, dim1, dim2, dim3, dim4
WHERE f.dim1_id = dim1.id AND f.dim2_id = dim2.id
AND f.dim3_id = dim3.id AND f.dim4_id = dim4.id
GROUP BY f.something;

Let's assume that each dimN table has PRIMARY KEY (id). Because of the
primary keys, it's only sensible to consider partial aggregation for
subsets of rels that include f; and it doesn't make sense to consider
partially aggregating after joining all 5 tables because at that point
we should just do a single-step aggregation. So, the partially
grouped-rel for {f,dim1,dim2,dim3,dim4} can contain paths generated in
15 different ways, because we can join f to any proper subset of
{dim1,dim2,dim3,dim4} before partially aggregating and then to the
remainder after partially aggregating. But that feels like we're
re-performing essentially the same join search 16 times which seems
super-expensive. I can't quite say that the work is useless or that I
have a better idea, but I guess there will be a lot of cases where all
16 join searches produce the same results, or most of them do. It
doesn't feel to me like checking through all of those possibilities is
a good expenditure of planner effort.

I took a look at the paper you linked in the original post, but
unfortunately it doesn't seem to say much about how to search the plan
space efficiently. I wonder if other systems perform a search that as
exhaustive as the one that you are proposing to perform here or
whether they apply some heuristics to limit the search space, and if
so, what those heuristics are.

---

From: Tom Lane <[email-masked]>
Date: Mon, 20 Jan 2025 12:57:20 -0500

> RelOptInfo; and this patch currently adds them a separate RelOptInfo.

Yeah, this.  I don't think that either of those first two decisions
was wrong, but it does seem annoying that this patch wants to do it
yet a third way.  Still, it may be the right thing.  Bear with me a
moment:

We dealt with parameterized paths being in the same list as
non-parameterized paths by treating the set of parameter rels as a
figure-of-merit that add_path can compare.  This works because if,
say, a nonparameterized path dominates a parameterized one on every
other figure of merit then there's no point in keeping the
parameterized one.  It is squirrely that the parameterized paths
typically don't yield the same number of rows as others for the same
RelOptInfo, but at least so far that hasn't broken anything.  I think
it's important that the parameterized paths do yield the same column
set as other paths for the rel; and the rows they do yield will be a
subset of the rows that nonparameterized paths yield.

On the other hand, it's not sensible for partial paths to compete
in an add_path tournament with non-partial ones.  If they did, neither
group could be allowed to dominate the other group, so add_path would
just be wasting its time making those path comparisons.  So I do think
it was right to put them in a separate path list.  Importantly, they
generate the same column set and some subset of the same rows that
the non-partial ones do, which I think is what justifies putting
them into the same RelOptInfo.

However, a partial-aggregation path does not generate the same data
as an unaggregated path, no matter how fuzzy you are willing to be
about the concept.  So I'm having a very hard time accepting that
it ought to be part of the same RelOptInfo, and thus I don't really
buy that annotating paths with a GroupPathInfo is the way forward.

What this line of analysis doesn't tell us though is whether paths
that did their partial aggregations at different join levels can be
considered as enough alike that they should compete on cost terms.
If they are, we need to put them into the same RelOptInfo.  So
while I want to have separate RelOptInfos for partially aggregated
paths, I'm unclear on how many of those we need or what their
identifying property is.

Also: we avoid generating parameterized partial paths, because
combining those things would be too much of a mess.  There's some
handwaving in the comments for add_partial_path to the effect that
it wouldn't be a win anyway, but I think the real reason is that
it'd be far too complicated for the potential value.  Can we make
a similar argument for partial aggregation?  I sure hope so.

> going to work out well.

FWIW, I'm way more concerned about the number of Paths considered
than I am about the number of RelOptInfos.  This relates to your
question about whether we want to use some heuristics to limit
the planner's search space.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 20 Jan 2025 13:39:15 -0500

> buy that annotating paths with a GroupPathInfo is the way forward.

Seems like a fair argument. I'm not sure it's dispositive if practical
considerations merited the opposite treatment, but that doesn't seem
to be the case.

> a similar argument for partial aggregation?  I sure hope so.

I think your hopes will be dashed in this instance. Suppose we have:

SELECT m.mapped_value, SUM(g.summable_quantity)
FROM mapping_table m JOIN gigantic_dataset g
WHERE m.raw_value = g.raw_value GROUP BY 1;

If the mapping_table is small, we can do something like this:

FinalizeAggregate
-> Gather
  -> PartialAggregate
    -> Hash Join

But if mapping_table is big but relatively few of the keys appear as
raw values in gigantic_dataset, being able to do the PartialAggregate
before the join would be rather nice; and you wouldn't want to give up
on parallel query in such a case.

P.S. I'm not so sure you're right about the reason why this is
supported. We can create a partial path for a joinrel by picking a
partial path on one side and a non-partial path on the other side, so
we can get NestLoops below Gather just fine using the parameterized
paths that we're generating anyway to support non-parallel cases. But
what would the plan look like if we were using a partial,
parameterized path? That path would have to be on the inner side of a
nested loo, and as far as I can see it would need to have a Gather
node on top of it and below the Nested Loop, so you're talking about
something like this:

Nested Loop
-> Seq Scan on something
-> Gather
  -> Nested Loop
    -> Index Scan on otherthing
       Index Cond: otherthing.x = something.x
    -> Whatever Scan on whatever

But putting Gather on the inner side of a nested loop like that would
mean repeatedly starting up workers and shutting them down again which
seems no fun at all. If there's some way of using a partial,
parameterized path that doesn't involve sticking a Gather on the inner
side of a Nested Loop, then the technique might have some promise and
the existing comment (which I probably wrote) is likely bunk.

> the planner's search space.

I had that instinct, too, but I'm not 100% sure whether it was a
correct instinct. If we create too many Paths, it's possible that most
of them will be thrown away before we really do anything with them, in
which case they cost CPU cycles but there's no memory accumulation.
Mixing paths that perform the partial aggregation at different levels
into the same RelOptInfo also increases the chances that you're going
to throw away a lot of stuff early. On the other hand, if we create
too many RelOptInfos, that memory can't be freed until the end of the
planning cycle. If you wouldn't have minded waiting a long time for
the planner, but you do mind running out of memory, the second one is
worse. But of course, the best option is to consider neither too many
Paths nor too many RelOptInfos.

I have heard rumors that in some other systems, they decide on the
best serial plan first and then insert parallel operators afterward.
Something like that could potentially be done here, too: only explore
eager aggregation for join orders that are sub-parts of the best
non-eagerly-aggregated join order. But I am sort of hesitant to
propose it as a development direction because we've never done
anything like that before and I don't think it would be at all easy to
implement. Nonetheless, I can't help feeling like we're kidding
ourselves a little bit, not just with this patch but in general. We
talk about "pushing down" aggregates or sorts or operations that can
be done on foreign nodes, but that implies that we start with them at
the top and then try to move them downward. In fact, we consider them
everywhere and expect the pushed-down versions to win out on cost.
While that feels sensible to some degree, it means every major new
query planning technique tends to multiply the amount of planner work
we're doing rather than adding to it. I'm fairly sure that the best
parallel plan need not be a parallelized version of the best
non-parallel plan but it often is, and the more things parallelism
supports, the more likely it is that it will be (I think). With eager
aggregation, it feels like we're multiplying the number of times that
we replan the same join tree by a number that is potentially MUCH
larger than 2, yet it seems to me that much of that extra work is
unlikely to find anything. Even if we find a way to make it work here
without too much pain, I wonder what happens when the next interesting
optimization comes along. Multiplication by a constant greater than or
equal to two isn't an operation one can do too many times, generally
speaking, without ending up with a big number.

---

From: Richard Guo <[email-masked]>
Date: Tue, 21 Jan 2025 17:33:29 +0900

> consider my concerns quite groundless and foolish.

I'm really sorry if my previous response upset you or gave the wrong
impression.  That was never my intention, and I certainly do not
consider your concerns to be groundless or foolish.  I can see how my
message may have come across differently than I intended.  To clarify,
I wasn't suggesting that your concerns about the estimates weren't
valid.  Rather, I was trying to express that it might be more
effective to fix the cost estimates based on specific regressions.

> through every level of plan over which we postponed partial

I've been thinking about this proposal, and it's quite appealing.  It
would significantly reduce both the planning effort and implementation
complexity, while still yielding reasonable planning results.

One concern I have with this proposal is that, as we climb up higher
and higher in the join tree, the assumption that a path with smaller
row count and higher cost is better than one with larger row count and
lower cost may gradually no longer hold.  It's true that a path with a
smaller row count is generally better for upper join nodes, as it
feeds fewer rows to upper join nodes.  However, as there are fewer and
fewer upper join nodes left, the efficiency gained from the smaller
row count could likely no longer justify the high cost of that path
itself.

Here's an example I found that can help illustrate what I mean.

create table t (a int, b int, c int);
insert into t select i%3, i%3, i from generate_series(1,500)i;
analyze t;
set enable_eager_aggregate to on;

And here are two plans for the same query:

-- Plan 1
explain (costs on)
select sum(t4.c) from t t1 join
  (t t2 join t t3 on t2.b != t3.b join t t4 on t3.b = t4.b)
  on t1.b = t2.b
group by t1.a;
                                        QUERY PLAN
------------------------------------------------------------------------------------------
 Finalize HashAggregate  (cost=4135.19..4135.22 rows=3 width=12)
   Group Key: t1.a
   ->  Hash Join  (cost=1392.13..3301.85 rows=166668 width=12)
         Hash Cond: (t2.b = t1.b)
         ->  Nested Loop  (cost=1377.88..1409.66 rows=1000 width=12)
               Join Filter: (t2.b <> t3.b)
               ->  Partial HashAggregate  (cost=1377.88..1377.91
rows=3 width=12)
                     Group Key: t3.b
                     ->  Hash Join  (cost=14.25..961.22 rows=83334 width=8)
                           Hash Cond: (t3.b = t4.b)
                           ->  Seq Scan on t t3  (cost=0.00..8.00
rows=500 width=4)
                           ->  Hash  (cost=8.00..8.00 rows=500 width=8)
                                 ->  Seq Scan on t t4
(cost=0.00..8.00 rows=500 width=8)
               ->  Materialize  (cost=0.00..10.50 rows=500 width=4)
                     ->  Seq Scan on t t2  (cost=0.00..8.00 rows=500 width=4)
         ->  Hash  (cost=8.00..8.00 rows=500 width=8)
               ->  Seq Scan on t t1  (cost=0.00..8.00 rows=500 width=8)
(17 rows)

-- Plan 2
explain (costs on)
select sum(t4.c) from t t1 join
  (t t2 join t t3 on t2.b != t3.b join t t4 on t3.b = t4.b)
  on t1.b = t2.b
group by t1.a;
                                           QUERY PLAN
------------------------------------------------------------------------------------------------
 Finalize HashAggregate  (cost=455675.44..455675.47 rows=3 width=12)
   Group Key: t1.a
   ->  Hash Join  (cost=455658.07..455672.94 rows=500 width=12)
         Hash Cond: (t1.b = t2.b)
         ->  Seq Scan on t t1  (cost=0.00..8.00 rows=500 width=8)
         ->  Hash  (cost=455658.03..455658.03 rows=3 width=12)
               ->  Partial HashAggregate  (cost=455658.00..455658.03
rows=3 width=12)
                     Group Key: t2.b
                     ->  Hash Join  (cost=14.25..316768.56
rows=27777887 width=8)
                           Hash Cond: (t3.b = t4.b)
                           ->  Nested Loop  (cost=0.00..3767.25
rows=166666 width=8)
                                 Join Filter: (t2.b <> t3.b)
                                 ->  Seq Scan on t t2
(cost=0.00..8.00 rows=500 width=4)
                                 ->  Materialize  (cost=0.00..10.50
rows=500 width=4)
                                       ->  Seq Scan on t t3
(cost=0.00..8.00 rows=500 width=4)
                           ->  Hash  (cost=8.00..8.00 rows=500 width=8)
                                 ->  Seq Scan on t t4
(cost=0.00..8.00 rows=500 width=8)
(17 rows)

For the grouped relation {t2 t3 t4}, Plan 1 chose the path
"PartialAgg(t3/t4) JOIN t2", while Plan 2 chose the path
"PartialAgg(t2/t3/t4)".

The first path has larger row count (1000) and lower cost (1409.66).
The second path has smaller row count (3) and higher cost (455658.03).

Executing these two plans shows that Plan 2 is slower than Plan 1.

-- Plan 1
 Execution Time: 286.860 ms

-- Plan 2
 Execution Time: 27109.744 ms

I think we may need to take the position in the join tree into account
when applying this heuristic.  At lower levels, we should prefer paths
with smaller row counts, while at higher levels, we should prefer
paths with lower costs.  However, it's unclear to me how we should
define "lower" and "higher" - how low is 'low' and how high is 'high'.

> a good expenditure of planner effort.

Yeah, you're right that the join search process for grouped paths
basically mirrors what we do for non-grouped paths, which indeed
involves a lot of planner effort.  I've been exploring potential
heuristics to limit the search space for grouped paths, but so far, I
haven't found any effective solutions.  Currently, the heuristic used
in the patch is to only consider grouped paths that dramatically
reduce the number of rows.  All others are just discarded.  The
rationale is that if a grouped path does not reduce the number of rows
enough, it is highly unlikely to result in a competitive final plan
during the upper planning stages, so it doesn't make much sense to
consider it.  The current threshold is set to 50%, meaning that if the
number of rows returned by PartialAgg(t1 JOIN t2) is not less than 50%
of the rows returned by (t1 JOIN t2), no Aggregate paths will be
generated on top of the t1/t2 join.  If we notice significant
regressions in planning time, we might consider further increasing
this threshold, say, to 80%, so that only grouped paths that reduce
the rows by more than 80% will be considered.  This heuristic also
ensures that, once a plan with eager aggregation is chosen, it is
highly likely to result in performance improvements, due to the
significant data reduction before joins.

> so, what those heuristics are.

Unfortunately, I don't have much knowledge about other systems.  It
would be really helpful if anyone could share some insights on how
other systems handle this.

Thanks
Richard

---

From: Richard Guo <[email-masked]>
Date: Tue, 21 Jan 2025 23:13:14 +0900

> buy that annotating paths with a GroupPathInfo is the way forward.

Agreed.  I think one point I failed to make myself clear on is that
I've never intended to put a partial-aggregation path and an
unaggregated path into the same RelOptInfo.  One of the basic designs
of this patch is that partial-aggregation paths are placed in a
separate category of RelOptInfos, which I call "grouped relations"
(though I admit that's not the best name).  This ensures that we never
compare a partial-aggregation path with an unaggregated path during
scan/join planning, because I am certain that the two categories of
paths are not comparable.

Regarding the GroupPathInfo proposal, my intention is to add a valid
GroupPathInfo only for the partial-aggregation paths.  The goal is to
ensure that partial-aggregation paths within this category are
compared only if their partial aggregations are at the same location.

To be honest, I still doubt that this is necessary.  I have two main
reasons for this.

1.
For a partial-aggregation path, the location where we place the
partial aggregation does not impose any restrictions on further
planning.  This is different from the parameterized path case.  If two
parameterized paths are equal on very other figure of merit, we will
choose the one with fewer required outer rels, as it means fewer join
restrictions on upper planning.  However, for partial-aggregation
paths, we do not have a preference regarding the location of the
partial aggregation.  For instance, for path "A JOIN PartialAgg(B)
JOIN C" and path "PartialAgg(A JOIN B) JOIN C", if one path dominates
the other on every figure of merit, it seems to me that there's no
point in keeping the less favorable one, although they have their
partial aggregations at different join levels.

2.
A partial-aggregation path of a rel essentially yields an aggregated
form of that rel's row set.  The difference between the row sets
yielded by paths with different locations of partial aggregation is
primarily about the different degrees to which the rows are
aggregated.  These sets are fundamentally homogeneous.

In summary, in my own opinion, I think the partial-aggregation paths
of the same "grouped relation" are comparable, regardless of the
position of the partial aggregation within the path tree.  So I think
we should put them into the same RelOptInfo.

Of course, I could be very wrong about this.  I would greatly
appreciate hearing others' thoughts on this.

Thanks
Richard

---

From: Robert Haas <[email-masked]>
Date: Tue, 21 Jan 2025 11:36:16 -0500

> Here's an example I found that can help illustrate what I mean.

Thanks for the example. What seems to be happening here is that each
of the three joins increases the number of rows by a multiple of
either 166 or 333. Aggregating reduces the number of rows to 3. I am
not sure that we should be too concerned about this kind of case,
because I don't think it will be common to have multiple joins that
dramatically increase the row count. If you did have that, you must
want to aggregate multiple times. We don't have the code for an
IntermediateAggregate or CombineAggregate node right now, I believe,
but in this query it would likely make sense to apply such a step
after every join; then you'd never have more than three rows.

Honestly, I'm not sure how much we should worry about a case like
this. I think that if a user is writing queries that use joins to
vastly inflate the row count and then aggregate the result, perhaps
they need to think about rewriting the queries. In this instance, it
feels a bit like the user is emulating multiplication using an
iterated SUM(), which is probably never going to work out all that
well.

But I bet it's possible to construct an example using only
row-reducing joins. Let's say we start with 10k rows that aggregate to
10 rows; after performing a join, we end up with 9k rows that
aggregate to 9 rows. So if we partially aggregate first, we have to
aggregate 1000 extra rows, but if we join first, we have to join 1000
extra rows. I don't think we can say a priori which will be cheaper,
but my idea would make the path that partially aggregates after the
join win unconditionally.

> significant data reduction before joins.

To be honest, I was quite surprised this was a percentage like 50% or
80% and not a multiple like 2 or 5. And I had thought the multiplier
might even be larger, like 10 or more. The thing is, 50% means we only
have to form 2-item groups in order to justify aggregating twice.
Maybe SUM() is cheap enough to justify that treatment, but a more
expensive aggregate might not be, especially things like string_agg()
or array_agg() where aggregation creates bigger objects.

Another thing to consider is that when the number of groups is small
enough that we don't need to do a Sort+GroupAggregate, it doesn't seem
so bad to perform marginally-useful partial aggregation, but sometimes
that won't be the case. For example, imagine that the user wants to
join orders to order_lines and then compute SUM(order_lines.quantity)
for each orders.customer_id. If the size of the order_lines tables is
large relative to  work_mem, we're going to need to sort it in order
to partially aggregate, which is expensive. If it turns out that the
orders table is also quite big, then maybe we'll end up performing a
merge join and the same sort order can be used for both operations,
but if not, we could've just done a hash join with orders as the build
table. In that kind of case, partial aggregation has to save quite a
lot to justify itself.

Now, maybe we shouldn't worry about that when applying this heuristic
cutoff; after all, it's the job of the cost model to understand that
sorting is expensive, and this cutoff should just be there to make
sure we don't even try the cost model in cases where it's clearly
unpromising. But I do suspect that in queries where the average group
size is 2, this will often be a marginal technique. In addition to the
problems already mentioned, it could be that the average group size is
2 but a lot of groups are actually of size 1 and then there are some
larger groups. In such cases I'm even less sure that the partial
aggregation technique will be a winner. Building many 1-element groups
sounds inefficient.

---

From: Richard Guo <[email-masked]>
Date: Wed, 22 Jan 2025 15:48:43 +0900

> after every join; then you'd never have more than three rows.

Haha, I did once think about the concept of multi-stage aggregations
while working on this patch.  While testing this patch and trying to
figure out where placing the partial aggregation would bring the most
benefit, I noticed that a potentially effective approach could be
this: every time the row count increases to a certain point as we join
more and more tables, we perform one aggregation to deflate it, and
then wait for it to grow again before deflating it once more.

This approach would require injecting multiple intermediate
aggregation nodes into the path tree, for which we currently lack the
necessary architecture.  As a result, I didn't pursue this idea
further.  However, I'm really glad you mentioned this approach, though
it's still unclear whether it's a feasible or reasonable idea.

> iterated SUM(), which is probably never going to work out all that

I don't have much experience with end-user scenarios, so I'm not sure
if it's common to have queries where the row count increases with more
and more tables joined.

> join win unconditionally.

Yeah, this is the concern I raised upthread: the efficiency gained
from a path having a smaller row count may not always justify the high
cost of the path itself, especially as we move higher in the join
tree.

> or array_agg() where aggregation creates bigger objects.

Hmm, if I understand correctly, the "percentage" and the "multiple"
work in the same way.  Percentage 50% and multiple 2 both mean that
the average group size is 2, and percentage 90% and multiple 10 both
mean that the average group size is 10.  In general, this relationship
should hold: percentage = 1 - 1/multiple.  However, I might not have
grasped your point correctly.

> aggregation technique will be a winner. Building many 1-element groups

Yeah, as you summarized, this heuristic is primarily used to discard
unpromising paths, ensuring they aren't considered further.  For the
paths that pass this heuristic, the cost model will then determine the
appropriate aggregation and join methods.  If we take this into
consideration when applying the heuristic, it seems to me that we
would essentially be duplicating the work that the cost model
performs, which doesn't seem necessary.

I think you are right that in cases where a lot of groups are actually
of size 1 and then there are some larger groups, the partial
aggregation may not be a win.  Perhaps we can do better in this if we
have the techniques to estimate the distribution of data across
different groups or to predict how skewed the data might be.  It seems
that we don't have such techniques at the moment.  This also reminds
me of a similar challenge when calculating the startup cost of
incremental sort.  I looked into cost_incremental_sort() and found
that we're currently using the average group size to estimate the
startup cost (please correct me if I'm wrong).

    group_tuples = input_tuples / input_groups;

I think this may also suffer from data skew across different groups.
With the mentioned techniques, I believe we could improve the cost
estimation for incremental sort as well.

If I understand correctly, your main concern is the threshold being
set to 2, rather than the heuristic itself, right?  Do you think
increasing this threshold to 10 or a larger value would help mitigate
the issue?

Thanks
Richard

---

From: Robert Haas <[email-masked]>
Date: Fri, 24 Jan 2025 15:53:42 -0500

> it's still unclear whether it's a feasible or reasonable idea.

I think the biggest question in my mind is really whether we can
accurately judge when such a strategy is likely to be a win. In this
instance it looks like we could have figured it out, but as we've
discussed, I fear a lot of estimates will be inaccurate. If we knew
they were going to be good, then I see no reason not to apply the
technique when it's sensible.

> if it's common to have queries where the row count increases with more

I don't think it's very common to see it increase as dramatically as
in your test case.

> grasped your point correctly.

Yes, they're equivalent. However, a percentage to me suggests that we
think that the meaningful values might be something like 20%, 50%,
80%; whereas with a multiplier someone might be more inclined to think
of values like 10, 100, 1000. You can definitely write those values as
90%, 99%, 99.9%; however, it seems less natural to me to express it
that way when we think the value will be quite close to 1. The fact
that you chose a percentage suggested to me that you were aiming for a
less-strict threshold than I had supposed we would want.

> performs, which doesn't seem necessary.

Well, I think we do ideally want heuristics that can reject
unpromising paths earlier. The planning cost of this is really quite
high. But I'm not sure how far we can get with this particular
heuristic. True, we could raise it to a larger value, and that might
help to rule out unpromising paths earlier. But I fear you'll quickly
find examples where it also rules out promising paths early. A good
heuristic is easy to compute and highly accurate. This heuristic is
easy to compute, but the accuracy is questionable.

---

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.

---

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

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

On Fri, 29 May 2026 at 17:25, Richard Guo <[email-masked]> wrote:

> 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: 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