Re: issues with eager aggregation

On Thu, Sep 17, 2026 at 11:58 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> On Thu, Sep 17, 2026 at 9:13 PM Robert Haas <robertmhaas(at)gmail(dot)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