Re: issues with eager aggregation

On Thu, Sep 17, 2026 at 9:13 PM Robert Haas <robertmhaas(at)gmail(dot)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