Re: Eager aggregation, take 3 - Mailing list pgsql-hackers

On Fri, Sep 5, 2025 at 11:37 PM Robert Haas <robertmhaas@gmail.com> wrote:
> I spent a bit of time scrolling through this today. Here are a few
> observations/review comments.

Thanks for all the 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.

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.

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

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.

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

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.

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

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.

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

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