Re: Eager aggregation, take 3

On Sun, Jan 12, 2025 at 9:04 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> Attached is an updated version of this patch that addresses Jian's
> review comments, along with some more cosmetic tweaks. I'm going to
> be looking at this patch again from the point of view of committing
> it, with the plan to commit it late this week or early next week,
> barring any further comments or objections.

I feel this is rushed. This is a pretty big patch touching a sensitive
area of the code. I'm the only senior hacker who has reviewed it, and
I would say that I've only reviewed it pretty lightly, and that the
concerns I raised were fairly substantial. I don't think it's
customary to go from that point to commit after one more patch
revision. This really deserves to be looked at by multiple senior
hackers familiar with the planner; or at least by Tom.

My core concerns here are still what they were in the first email I
posted to the thread: it's unclear that the cost model will deliver
meaningful answers for the grouped rels, and it doesn't seem like
you've done enough to limit the overhead of the feature.

With regard to the first, I reiterate that we are in general quite bad
at having meaningful statistics for anything above an aggregate, and
this patch greatly expands how much of a query could be above an
aggregate. I felt back in August when I did my first review, and still
feel now, that when faced with a query where aggregation could be done
at any of several levels, the chances of picking the right one are not
much better than random. Why do you think otherwise?

With regard to the second, I suggested several lines of thinking back
in August that could lead to limiting the number of grouped_rels that
we create, but it doesn't really look like much of anything has
changed. We're still creating a partially grouped rel for every
baserel in the query, and every joinrel in the query. I'm not very
happy with "let's just turn it off by default" as the answer to that
concern. A lot of people won't enable the feature, which will mean it
doesn't have much value to our users, and those who do will still see
a lot of overhead. Maybe I'm wrong, but I bet with some good
heuristics the planning cost of this could be reduced by an order of
magnitude or more. If that were done, we could imagine eventually (or
maybe even immediately) enabling this by default; without that, we
still have the burden of maintaining this code and keeping it working,
but almost nobody will benefit.

+ <term><varname>enable_eager_aggregate</varname> (<type>boolean</type>)
+ <para>
+ Enables or disables the query planner's ability to partially push
+ aggregation past a join, and finalize it once all the relations are
+ joined. The default is <literal>off</literal>.

I'm a bit concerned about the naming here. I feel like we're adding an
increasing number of planner features with an increasing number of
disabling GUCs that are all a bit random. I kind of wonder if this
should be called enable_incremental_aggregate. Maybe that's worse,
because "eager" is a word we're not using for anything yet, so using
it here improves greppability and perhaps understandability. On the
other hand, the aggregate that is pushed down by this feature is
always partial (I believe) so we still need a finalize step later,
which means we're aggregating incrementally. There's some nice parity
with incremental sort, too, perhaps.

+/* The original length and hashtable of a RelInfoList */
+typedef struct
+{
+ int savelength;
+ struct HTAB *savehash;
+} RelInfoListInfo;

Both the comment and the name of the data type are completely meaningless.

+ /*
+ * Try at least sorting the cheapest path and also try
+ * incrementally sorting any path which is partially sorted
+ * already (no need to deal with paths which have presorted
+ * keys when incremental sort is disabled unless it's the
+ * cheapest input path).
+ */

This would be the fifth copy of this comment. It's not entirely this
patch's fault, of course, but some kind of refactoring or cleanup is
probably needed here.

+ * cheapest_parameterized_paths also always includes the fewest-row
+ * unparameterized path, if there is one, for grouped relations. Different
+ * paths of a grouped relation can have very different row counts, and in some
+ * cases the cheapest-total unparameterized path may not be the one with the
+ * fewest row.

As I said back in October, this seems like mixing together in one
RelOptInfo paths that really belong to two different RelOptInfos.

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