Re: Eager aggregation, take 3

On Thu, Jan 16, 2025 at 3:18 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> If this t1/t2 join is part of a larger SELECT query, I think the cost
> estimates for the upper join nodes would likely be quite inaccurate.

That's definitely true. However, the question is not whether the
planner has problems today (it definitely does) but whether it's OK to
make this change without improving our ability to estimate the effects
of aggregation operations. I understand that you (quite rightly) don't
want to get sucked into fixing unrelated planner problems, and I'm
also not sure to what extent these problems are actually fixable.
However, major projects sometimes require such work. For instance,
commit 5edc63bda68a77c4d38f0cbeae1c4271f9ef4100 was motivated by the
discovery that it was too easy to get a Parallel Bitmap Heap Scan plan
even when it wasn't best. The fact that the costing wasn't right
wasn't the fault of parallel query, but parallel query still needed to
do something about it to get good results.

> Yeah, I know 7 is not a large number, but this is the result I got
> from running the TPC-DS benchmark. For the remaining 92 queries in
> the benchmark, either the logic in this patch determines that eager
> aggregation is not applicable, or the path with eager aggregation is
> not the optimal one. I'd be more than happy if a benchmark query
> showed significant performance regression, so it would provide an
> opportunity to investigate how the cost estimates are negatively
> impacting the final plan and explore ways to avoid or improve that.
> If anyone can provide such a benchmark query, I'd be very grateful.

Yes, having more people test this and look for regressions would be
quite valuable.

> Well, from the perspective of planning effort, what really matters is
> whether the RelOptInfo for the grouped relation is added to the
> PlannerInfo, as it is only then available for further joining in the
> join search routine, not whether the RelOptInfo is built or not.
> Building the RelOptInfo for a grouped relation is simply a makeNode
> call followed by a flat copy; it doesn't require going through the
> full process of determining its target list, or constructing its
> restrict and join clauses, or calculating size estimates, etc.

That's probably mostly true, but the overhead of memory allocations in
planner routines is not trivial. There are previous cases of changing
things or declining to change this purely on the number of palloc
cycles involved.

> > It's possible you're right, but it does make me nervous. I do agree
> > that making the number of RelOptInfos explode would be really bad.
>
> Based on my explanation in [1], I think it's acceptable to compare
> grouped paths for the same grouped rel, regardless of where the
> partial aggregation is placed.
>
> I fully understand that I could be wrong about this, but I don't think
> it would break anything in regular planning (i.e., planning without
> eager aggregation).

I think you might be taking too narrow a view of the problem. As Tom
says, the issue is that this breaks a bunch of assumptions that hold
elsewhere. One place that shows up in the patch is in the special-case
logic you've added to set_cheapest(), but I fear that won't be the end
of it. It seems a bit surprising to me that you didn't also need to
adjust add_path(), for example. Even if you don't, there's lots of
places that rely on the assumption that all paths for a RelOptInfo are
returning the same set of rows. If it turns out that a bunch of those
places need to be adjusted to work with this, then the code could
potentially end up quite messy, and that might also have performance
consequences, even when this feature is disabled. Many of the code
paths that deal with paths in the planner are quite hot.

To say that another way, I'm not so much worried about the possibility
that the patch contains a bug. Patches contain bugs all the time and
we can just fix them. It's not wonderful, but that's how software
development goes. What I am worried about is whether the architecture
is right. If we go with one RelOptInfo when the "right answer" is
many, or for that matter if we go with many when the right answer is
one, those are things that cannot be easily and reasonably patched
post-commit, and especially not post-release.

--
Robert Haas
EDB: http://www.enterprisedb.com