Re: Eager aggregation, take 3
Tom Lane <tgl@sss.pgh.pa.us>
Robert Haas <robertmhaas@gmail.com> writes: > So I don't quite know which way to jump here. It now seems to me that > we have three similar features with three different designs. > Parameterization added non-comparable paths to the same path list; > parallel query added them to a different path list in the same > RelOptInfo; and this patch currently adds them a separate RelOptInfo. Yeah, this. I don't think that either of those first two decisions was wrong, but it does seem annoying that this patch wants to do it yet a third way. Still, it may be the right thing. Bear with me a moment: We dealt with parameterized paths being in the same list as non-parameterized paths by treating the set of parameter rels as a figure-of-merit that add_path can compare. This works because if, say, a nonparameterized path dominates a parameterized one on every other figure of merit then there's no point in keeping the parameterized one. It is squirrely that the parameterized paths typically don't yield the same number of rows as others for the same RelOptInfo, but at least so far that hasn't broken anything. I think it's important that the parameterized paths do yield the same column set as other paths for the rel; and the rows they do yield will be a subset of the rows that nonparameterized paths yield. On the other hand, it's not sensible for partial paths to compete in an add_path tournament with non-partial ones. If they did, neither group could be allowed to dominate the other group, so add_path would just be wasting its time making those path comparisons. So I do think it was right to put them in a separate path list. Importantly, they generate the same column set and some subset of the same rows that the non-partial ones do, which I think is what justifies putting them into the same RelOptInfo. However, a partial-aggregation path does not generate the same data as an unaggregated path, no matter how fuzzy you are willing to be about the concept. So I'm having a very hard time accepting that it ought to be part of the same RelOptInfo, and thus I don't really buy that annotating paths with a GroupPathInfo is the way forward. What this line of analysis doesn't tell us though is whether paths that did their partial aggregations at different join levels can be considered as enough alike that they should compete on cost terms. If they are, we need to put them into the same RelOptInfo. So while I want to have separate RelOptInfos for partially aggregated paths, I'm unclear on how many of those we need or what their identifying property is. Also: we avoid generating parameterized partial paths, because combining those things would be too much of a mess. There's some handwaving in the comments for add_partial_path to the effect that it wouldn't be a win anyway, but I think the real reason is that it'd be far too complicated for the potential value. Can we make a similar argument for partial aggregation? I sure hope so. > I agree that creating an exponential number of RelOptInfos is not > going to work out well. FWIW, I'm way more concerned about the number of Paths considered than I am about the number of RelOptInfos. This relates to your question about whether we want to use some heuristics to limit the planner's search space. regards, tom lane
Commits
-
Fix eager aggregation for semi/antijoin inner rels
- ffeda04259bb 19 (unreleased) landed
-
Cover additional errors and corner conditions in repack.c
- 2670cc298f42 19 (unreleased) cited
-
Fix volatile function evaluation in eager aggregation
- 3a08a2a8b4fd 19 (unreleased) landed
-
Fix collation handling for grouping keys in eager aggregation
- bd94845e8c90 19 (unreleased) landed
-
Rename apply_at to apply_agg_at for clarity
- 1206df04c200 19 (unreleased) landed
-
Fix comment in eager_aggregate.sql
- 36fd8bde1b77 19 (unreleased) landed
-
Remove unnecessary include of "utils/fmgroids.h"
- f997d777adf7 19 (unreleased) landed
-
Implement Eager Aggregation
- 8e11859102f9 19 (unreleased) landed
-
Allow negative aggtransspace to indicate unbounded state size
- 185e30426334 19 (unreleased) landed
-
Add macros for looping through a List without a ListCell.
- 14dd0f27d7cd 17.0 cited
-
Account for the effect of lossy pages when costing bitmap scans.
- 5edc63bda68a 11.0 cited
-
Fix a thinko in join_is_legal: when we decide we can implement a semijoin
- a43b190e3c71 9.0.0 cited