Re: Default setting for enable_hashagg_disk
Peter Geoghegan <pg@bowt.ie>
From: Peter Geoghegan <pg@bowt.ie>
To: Stephen Frost <sfrost@snowman.net>
Cc: Jeff Davis <pgsql@j-davis.com>, Alvaro Herrera <alvherre@2ndquadrant.com>, Andres Freund <andres@anarazel.de>, Bruce Momjian <bruce@momjian.us>, Robert Haas <robertmhaas@gmail.com>, David Rowley <dgrowleyml@gmail.com>, Justin Pryzby <pryzby@telsasoft.com>,
Melanie Plageman <melanieplageman@gmail.com>, Tomas Vondra <tomas.vondra@2ndquadrant.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2020-07-09T22:32:20Z
Lists: pgsql-hackers, pgsql-docs
On Thu, Jul 9, 2020 at 7:03 AM Stephen Frost <sfrost@snowman.net> wrote: > > The one for the planner is already there, and it looks like we need one > > for the executor as well (to tell HashAgg to ignore the memory limit > > just like v12). > > No, ignoring the limit set was, as agreed above, a bug, and I don't > think it makes sense to add some new user tunable for this. It makes more sense than simply ignoring what our users will see as a simple regression. (Though I still lean towards fixing the problem by introducing hash_mem, which at least tries to fix the problem head on.) > If folks > want to let HashAgg use more memory then they can set work_mem higher, > just the same as if they want a Sort node to use more memory or a > HashJoin. Yes, that comes with potential knock-on effects about other > nodes (possibly) using more memory but that's pretty well understood for > all the other cases and I don't think that it makes sense to have a > special case for HashAgg when the only justification is that "well, you > see, it used to have this bug, so...". That's not the only justification. The other justification is that it's generally reasonable to prefer giving hash aggregate more memory. This would even be true in a world where all grouping estimates were somehow magically accurate. These two justifications coincide in a way that may seem a bit too convenient to truly be an accident of history. And if they do: I agree. It's no accident. It seems likely that we have been "complicit" in enabling "applications that live beyond their means", work_mem-wise. We knew that hash aggregate had this "bug" forever, and yet we were reasonably happy to have it be the common case for years. It's very fast, and didn't actually explode most of the time (even though grouping estimates are often pretty poor). Hash agg was and is the common case. Yes, we were concerned about the risk of OOM for many years, but it was considered a risk worth taking. We knew what the trade-off was. We never quite admitted it, but what does it matter? Our own tacit attitude towards hash agg + work_mem mirrors that of our users (or at least the users that will be affected by this issue, of which there will be plenty). Declaring this behavior a bug with no redeeming qualities now seems a bit rich. -- Peter Geoghegan
Commits
-
Add hash_mem_multiplier GUC.
- d6c08e29e7bc 14.0 landed
- 78530c8e7a5a 13.0 landed
-
HashAgg: use better cardinality estimate for recursive spilling.
- 3a232a3183d5 13.0 landed
- 9878b643f37b 14.0 landed
-
Remove hashagg_avoid_disk_plan GUC.
- bcbf9446a298 14.0 landed
- 5a6cc6ffa914 13.0 landed
-
Doc fixup for hashagg_avoid_disk_plan GUC.
- d33f33539d7f 13.0 landed
- 7ce461560159 14.0 landed
-
Rework HashAgg GUCs.
- 13e0fa7ae50c 13.0 landed
- 92c58fd94801 14.0 landed
-
Disk-based Hash Aggregation.
- 1f39bce02154 13.0 cited
-
Implement partition-wise grouping/aggregation.
- e2f1eb0ee30d 11.0 cited
-
Defer creation of partially-grouped relation until it's needed.
- 4f15e5d09de2 11.0 cited