issues with eager aggregation
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253824psql -h localhost -U postgresBuilt from patchset v7 (message #7), October 05, 2026 at 01:57 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253824_7 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253824_7 && git checkout t253824_7Patchset v7 (message #7) is on t253824_7
Hi,
I noticed that in the flurry of LLM-driven bug hunting, Richard's work
in this release has remained relatively lightly impacted, which is
probably a testament to him having done a good job with the work.
However, I thought it would be a good idea to probe for problems and
unfortunately Claude was able to find a few. Three of the four
findings are just bugs; they need to be fixed, but they're not really
a big deal. The fourth one is much more debatable: it's not a bug, but
a question about whether the eager aggregation patch implements
correct behavior.
Here's the problem: in our normal mental model of how queries work,
aggregation happens after joins. The whole idea of eager aggregation
is to do part of the aggregation work before performing all of the
joins. Normally, this is invisible to the user, except to the extent
that it influences query performance. But when some operator or
function involved in the query has side effects, the change becomes
user-visible. Here is an example:
Setup:
CREATE TABLE t1 (a int, b int);
CREATE TABLE t2 (b int, c int);
INSERT INTO t1 SELECT i, i FROM generate_series(1, 100) i;
INSERT INTO t2 SELECT i % 10, CASE WHEN i % 10 = 0 THEN 0 ELSE i END
FROM generate_series(1, 1000) i;
ANALYZE t1, t2;
Test query:
SELECT t1.a, sum(100 / t2.c) FROM t1 JOIN t2 ON t1.b = t2.b GROUP BY t1.a;
Without eager aggregation, the division operator is only applied to
rows that survive the join, so the query completes successfully. With
eager aggregation, the operator can be applied to rows that won't end
up finding a join partner, so the query errors out with "ERROR:
division by zero". This runs contrary to my mental model, and also to
the statements on the "SELECT" reference page that aggregation happens
after joins: step 3 eliminates rows that do not match the WHERE
clause, and grouping happens in step 4. I think you can make an
argument that what this page describes is not a totally absolute
categorization scheme, and/or that it only covers results and not side
effects, but in general we do mostly follow that ordering from the
point of user-visible effects, and here we don't.
3a08a2a8b4fd36a9fa0da0253d1ca053c19047d5 established the precedent
that we shouldn't alter the execution count of functions by pushing
them below a join, but erred in believing that only volatile functions
were an issue. Hence, while I don't think it's a complete slam-dunk
that this behavior is definitively and undeniably wrong, I do think
that it is generally contrary to the query behavior we usually try to
deliver.
I attach the SQL scripts for the other problems. finding1.sql produces
the wrong answer and finding2.sql and finding3.sql produce internal
errors.
--
Robert Haas
On Thu, Sep 17, 2026 at 9:13 PM Robert Haas <robertmhaas@gmail.com> wrote:
I noticed that in the flurry of LLM-driven bug hunting, Richard's work
in this release has remained relatively lightly impacted, which is
probably a testament to him having done a good job with the work.
Thanks! :-)
However, I thought it would be a good idea to probe for problems and
unfortunately Claude was able to find a few. Three of the four
findings are just bugs; they need to be fixed, but they're not really
a big deal. The fourth one is much more debatable: it's not a bug, but
a question about whether the eager aggregation patch implements
correct behavior.
Thanks for the report. All three reproduce here.
finding1: I found the cause. t2.b (int4) was grouped using the
equality of t1.a (int2), so 5 and 65541 fell into the same group.
I think the culprit is that get_expression_sortgroupref() reuses the
grouping key's SortGroupClause for a Var that is equal to that
grouping key via EC, even if the Var is of a different type.
I think we can fix it by:
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -3329,6 +3329,10 @@ get_expression_sortgroupref(PlannerInfo *root,
Expr *expr)
!bms_is_member(((Var *) expr)->varno, ge_info->ec->ec_relids))
continue;+ /* The grouping operators can't be applied to a cross-type member */
+ if (exprType((Node *) expr) != exprType((Node *) ge_info->expr))
+ continue;finding2: The cause is that when we flat-copy a join rel in
build_grouped_rel(), we also copy its fdwroutine, which is wrong.
This can be fixed by simply clearing the FDW fields in
build_grouped_rel().
finding3: The cause is that a join key that is not a GROUP BY column
(p1.z here) becomes an extra grouping key for partial aggregation, and
its sort EC is created during join search. That is too late for child
rels to get EC members, so the sort on a child rel fails. I guess we
can fix this by creating these ECs in setup_eager_aggregation(),
before the appendrels are expanded. Will have a try later.
As for the division-by-zero case, I agree it's user-visible, but I
think eager aggregation should follow the same rules as our existing
transformations, which already evaluate expressions on rows that a
join removes. With eager aggregation off, each of these fails the
same way:
1. A WHERE clause is pushed down to the scan of t2:
SELECT count(*) FROM t1 JOIN t2 ON t1.b = t2.b WHERE 100 / t2.c > 0;
ERROR: division by zero
2. A HAVING clause can be moved to WHERE and then pushed down
likewise; kept in HAVING, it would only see joined rows:
SELECT t2.c, count(*) FROM t1 JOIN t2 ON t1.b = t2.b
GROUP BY t2.c HAVING 100 / t2.c > 0;
ERROR: division by zero
3. A subquery is pulled up and its outer qual pushed down, while
OFFSET 0 keeps the qual above the join:
SELECT count(*) FROM (SELECT t2.c FROM t1 JOIN t2 ON t1.b = t2.b) s
WHERE 100 / s.c > 0;
ERROR: division by zero
SELECT count(*) FROM (SELECT t2.c FROM t1 JOIN t2 ON t1.b = t2.b OFFSET 0) s
WHERE 100 / s.c > 0;
count
-------
90
(1 row)
4. Maybe more ...
3a08a2a8b was about how many times a volatile function is called,
which is a different concern from whether an error is raised.
Alternatively, we could apply eager aggregation only when the
aggregate's arguments (and FILTER clause) are known not to raise
errors. But that would greatly limit its applicability.
So I'm inclined to keep the current behavior. If people feel this
goes too far, restricting the arguments is the fallback. What do you
think?
- Richard
On Thu, Sep 17, 2026 at 10:58 AM Richard Guo <guofenglinux@gmail.com> wrote:
As for the division-by-zero case, I agree it's user-visible, but I
think eager aggregation should follow the same rules as our existing
transformations, which already evaluate expressions on rows that a
join removes.
OK, good. So this isn't really doing something that is fundamentally
new, which is what I was fearing. Do you think this section should be
adjusted at all?
https://www.postgresql.org/docs/current/sql-expressions.html#SYNTAX-EXPRESS-EVAL
--
Robert Haas
On Fri, Sep 18, 2026 at 12:55 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Sep 17, 2026 at 10:58 AM Richard Guo <guofenglinux@gmail.com> wrote:
As for the division-by-zero case, I agree it's user-visible, but I
think eager aggregation should follow the same rules as our existing
transformations, which already evaluate expressions on rows that a
join removes.
OK, good. So this isn't really doing something that is fundamentally
new, which is what I was fearing. Do you think this section should be
adjusted at all?
Yes, I think it's worth saying that an expression can be evaluated on
rows that don't appear in the result.
- Richard
On 9/18/26 01:58, Richard Guo wrote:
On Fri, Sep 18, 2026 at 12:55 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Sep 17, 2026 at 10:58 AM Richard Guo <guofenglinux@gmail.com> wrote:
As for the division-by-zero case, I agree it's user-visible, but I
think eager aggregation should follow the same rules as our existing
transformations, which already evaluate expressions on rows that a
join removes.OK, good. So this isn't really doing something that is fundamentally
new, which is what I was fearing. Do you think this section should be
adjusted at all?Yes, I think it's worth saying that an expression can be evaluated on
rows that don't appear in the result.
How likely is it that a working query starts failing due to flipping to
a plan with eager aggregate?
I realize it's hard to estimate this, but Robert's example with a join
filtering rows may not be all that uncommon. Say, you might be asking
for average price of products currently offered in shops, and it's fine
that a product has no price if it's not being sold. And so on.
And if it happens, how can the user remedy it? I can think of disabling
the eager aggregation, or adding an "OFFSET 0" ...
FWIW I suspect 99.999% of conditions won't have this problem, because
most clauses won't have this type of failures. And as demonstrated,
there are preexisting cases with similar behavior.
regards
--
Tomas Vondra
On Fri, Sep 18, 2026 at 9:20 AM Tomas Vondra <tomas@vondra.me> wrote:
How likely is it that a working query starts failing due to flipping to
a plan with eager aggregate?
It needs three things at once: an aggregate argument that can raise an
error (such as division by zero), values that fail only in rows the
join removes, and the planner actually choosing eager aggregation,
which requires partial groups of at least min_eager_agg_group_size
rows and a cheaper plan. So I agree it should be rare, but your
products example shows it's not unrealistic.
And if it happens, how can the user remedy it? I can think of disabling
the eager aggregation, or adding an "OFFSET 0" ...
Those two work. The expression can also be guarded with CASE, which
is what the docs already recommend for forcing evaluation order, e.g.
sum(CASE WHEN c <> 0 THEN 100 / c END)
For comparison, the existing cases have no GUC at all. The HAVING
case upthread is probably the closest one: HAVING is supposed to apply
to groups formed after the join, yet a HAVING clause can be pushed
below the join, and see rows that never form a group. CTEs are
another: since v12 they are inlined by default, so a WHERE clause on a
CTE can be pushed below a join inside it, and a query that worked in
v11 can fail in v12:
WITH s AS (SELECT t2.c FROM t1 JOIN t2 ON t1.b = t2.b)
SELECT count(*) FROM s WHERE 100 / s.c > 0;
ERROR: division by zero
The remedy there is AS MATERIALIZED, and elsewhere OFFSET 0 or
guarding the expression with CASE. So I think eager aggregation is no
worse off, and has one more way out, the enable_eager_aggregate GUC.
FWIW I suspect 99.999% of conditions won't have this problem, because
most clauses won't have this type of failures. And as demonstrated,
there are preexisting cases with similar behavior.
Agreed.
- Richard
On Thu, Sep 17, 2026 at 11:58 PM Richard Guo <guofenglinux@gmail.com> wrote:
On Thu, Sep 17, 2026 at 9:13 PM Robert Haas <robertmhaas@gmail.com> wrote:
However, I thought it would be a good idea to probe for problems and
unfortunately Claude was able to find a few. Three of the four
findings are just bugs; they need to be fixed, but they're not really
a big deal. The fourth one is much more debatable: it's not a bug, but
a question about whether the eager aggregation patch implements
correct behavior.
Thanks for the report. All three reproduce here.
finding1: I found the cause. t2.b (int4) was grouped using the
equality of t1.a (int2), so 5 and 65541 fell into the same group.
I think the culprit is that get_expression_sortgroupref() reuses the
grouping key's SortGroupClause for a Var that is equal to that
grouping key via EC, even if the Var is of a different type.
finding2: The cause is that when we flat-copy a join rel in
build_grouped_rel(), we also copy its fdwroutine, which is wrong.
This can be fixed by simply clearing the FDW fields in
build_grouped_rel().
finding3: The cause is that a join key that is not a GROUP BY column
(p1.z here) becomes an extra grouping key for partial aggregation, and
its sort EC is created during join search. That is too late for child
rels to get EC members, so the sort on a child rel fails. I guess we
can fix this by creating these ECs in setup_eager_aggregation(),
before the appendrels are expanded. Will have a try later.
Here are the patches for the three bugs.
0001 and 0002 fix finding1 and finding2 as described upthread.
0003 fixes finding3, but not the way I suggested earlier. Creating
the ECs in setup_eager_aggregation() turned out not to be enough: if
the extra grouping key is nullable by an outer join below the grouped
join rel, it's a different Var, and the child join still has no EC
member to sort by. So instead the patch adds the missing child
members at the point where the grouping pathkeys of a child rel are
built, much like add_child_join_rel_equivalences() does for child
joins. The second test case in 0003 covers the outer join case.
I plan to push these and back-patch to v19 soon, barring objections.
- Richard