issues with eager aggregation
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