pg.ddx.io pgsql-docs@postgresql.org mailing list archive
help / color / mirror / Atom feedDefault setting for enable_hashagg_disk
91+ messages / 13 participants
[nested] [flat]
Commits from this thread:
2020-07-29 78530c8e7a d6c08e29e7 Add hash_mem_multiplier GUC.
2020-07-28 5a6cc6ffa9 bcbf9446a2 Remove hashagg_avoid_disk_plan GUC.
2020-06-22 7ce4615601 d33f33539d Doc fixup for hashagg_avoid_disk_plan GUC.
2020-06-11 92c58fd948 13e0fa7ae5 Rework HashAgg GUCs.
* Default setting for enable_hashagg_disk
@ 2020-04-07 18:20 Jeff Davis <pgsql@j-davis.com>
0 siblings, 2 replies; 91+ messages in thread
From: Jeff Davis @ 2020-04-07 18:20 UTC (permalink / raw)
To: pgsql-hackers@postgresql.org
This is just a placeholder thread for an open item that I'm adding to
the Open Items list. We can make a decision later.
Now that we have Disk-based Hash Aggregation, there are a lot more
situations where the planner can choose HashAgg. The
enable_hashagg_disk GUC, if set to true, chooses HashAgg based on
costing. If false, it only generates a HashAgg path if it thinks it
will fit in work_mem, similar to the old behavior (though it wlil now
spill to disk if the planner was wrong about it fitting in work_mem).
The current default is true.
I expect this to be a win in a lot of cases, obviously. But as with any
planner change, it will be wrong sometimes. We may want to be
conservative and set the default to false depending on the experience
during beta. I'm inclined to leave it as true for now though, because
that will give us better information upon which to base any decision.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-04-07 22:39 Justin Pryzby <pryzby@telsasoft.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 91+ messages in thread
From: Justin Pryzby @ 2020-04-07 22:39 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-hackers@postgresql.org
On Tue, Apr 07, 2020 at 11:20:46AM -0700, Jeff Davis wrote:
> The enable_hashagg_disk GUC, if set to true, chooses HashAgg based on
> costing. If false, it only generates a HashAgg path if it thinks it will fit
> in work_mem, similar to the old behavior (though it wlil now spill to disk if
> the planner was wrong about it fitting in work_mem). The current default is
> true.
Are there any other GUCs that behave like that ? It's confusing to me when I
see "Disk Usage: ... kB", despite setting it to "disable", and without the
usual disable_cost. I realize that postgres chose the plan on the hypothesis
that it would *not* exceed work_mem, and that spilling to disk is considered
preferable to ignoring the setting, and that "going back" to planning phase
isn't a possibility.
template1=# explain (analyze, costs off, summary off) SELECT a, COUNT(1) FROM generate_series(1,999999) a GROUP BY 1 ;
HashAggregate (actual time=1370.945..2877.250 rows=999999 loops=1)
Group Key: a
Peak Memory Usage: 5017 kB
Disk Usage: 22992 kB
HashAgg Batches: 84
-> Function Scan on generate_series a (actual time=314.507..741.517 rows=999999 loops=1)
A previous version of the docs said this, which I thought was confusing, and you removed it.
But I guess this is the behavior it was trying to .. explain.
+ <term><varname>enable_hashagg_disk</varname> (<type>boolean</type>)
+ ... This only affects the planner choice;
+ execution time may still require using disk-based hash
+ aggregation. The default is <literal>on</literal>.
I suggest that should be reworded and then re-introduced, unless there's some
further behavior change allowing the previous behavior of
might-exceed-work-mem.
"This setting determines whether the planner will elect to use a hash plan
which it expects will exceed work_mem and spill to disk. During execution,
hash nodes which exceed work_mem will spill to disk even if this setting is
disabled. To avoid spilling to disk, either increase work_mem (or set
enable_hashagg=off)."
For sure the release notes should recommend re-calibrating work_mem.
--
Justin
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-04-09 11:48 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 91+ messages in thread
From: Tomas Vondra @ 2020-04-09 11:48 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-hackers@postgresql.org
On Tue, Apr 07, 2020 at 05:39:01PM -0500, Justin Pryzby wrote:
>On Tue, Apr 07, 2020 at 11:20:46AM -0700, Jeff Davis wrote:
>> The enable_hashagg_disk GUC, if set to true, chooses HashAgg based on
>> costing. If false, it only generates a HashAgg path if it thinks it will fit
>> in work_mem, similar to the old behavior (though it wlil now spill to disk if
>> the planner was wrong about it fitting in work_mem). The current default is
>> true.
>
>Are there any other GUCs that behave like that ? It's confusing to me when I
>see "Disk Usage: ... kB", despite setting it to "disable", and without the
>usual disable_cost. I realize that postgres chose the plan on the hypothesis
>that it would *not* exceed work_mem, and that spilling to disk is considered
>preferable to ignoring the setting, and that "going back" to planning phase
>isn't a possibility.
>
It it really any different from our enable_* GUCs? Even if you do e.g.
enable_sort=off, we may still do a sort. Same for enable_groupagg etc.
>template1=# explain (analyze, costs off, summary off) SELECT a, COUNT(1) FROM generate_series(1,999999) a GROUP BY 1 ;
> HashAggregate (actual time=1370.945..2877.250 rows=999999 loops=1)
> Group Key: a
> Peak Memory Usage: 5017 kB
> Disk Usage: 22992 kB
> HashAgg Batches: 84
> -> Function Scan on generate_series a (actual time=314.507..741.517 rows=999999 loops=1)
>
>A previous version of the docs said this, which I thought was confusing, and you removed it.
>But I guess this is the behavior it was trying to .. explain.
>
>+ <term><varname>enable_hashagg_disk</varname> (<type>boolean</type>)
>+ ... This only affects the planner choice;
>+ execution time may still require using disk-based hash
>+ aggregation. The default is <literal>on</literal>.
>
>I suggest that should be reworded and then re-introduced, unless there's some
>further behavior change allowing the previous behavior of
>might-exceed-work-mem.
>
Yeah, it would be good to mention this is a best-effort setting.
>"This setting determines whether the planner will elect to use a hash plan
>which it expects will exceed work_mem and spill to disk. During execution,
>hash nodes which exceed work_mem will spill to disk even if this setting is
>disabled. To avoid spilling to disk, either increase work_mem (or set
>enable_hashagg=off)."
>
>For sure the release notes should recommend re-calibrating work_mem.
>
I don't follow. Why would the recalibrating be needed?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-04-09 17:24 Justin Pryzby <pryzby@telsasoft.com>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
1 sibling, 2 replies; 91+ messages in thread
From: Justin Pryzby @ 2020-04-09 17:24 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-hackers@postgresql.org
On Thu, Apr 09, 2020 at 01:48:55PM +0200, Tomas Vondra wrote:
> On Tue, Apr 07, 2020 at 05:39:01PM -0500, Justin Pryzby wrote:
> > On Tue, Apr 07, 2020 at 11:20:46AM -0700, Jeff Davis wrote:
> > > The enable_hashagg_disk GUC, if set to true, chooses HashAgg based on
> > > costing. If false, it only generates a HashAgg path if it thinks it will fit
> > > in work_mem, similar to the old behavior (though it wlil now spill to disk if
> > > the planner was wrong about it fitting in work_mem). The current default is
> > > true.
> >
> > Are there any other GUCs that behave like that ? It's confusing to me when I
> > see "Disk Usage: ... kB", despite setting it to "disable", and without the
> > usual disable_cost. I realize that postgres chose the plan on the hypothesis
> > that it would *not* exceed work_mem, and that spilling to disk is considered
> > preferable to ignoring the setting, and that "going back" to planning phase
> > isn't a possibility.
>
> It it really any different from our enable_* GUCs? Even if you do e.g.
> enable_sort=off, we may still do a sort. Same for enable_groupagg etc.
Those show that the GUC was disabled by showing disable_cost. That's what's
different about this one.
Also.. there's no such thing as enable_groupagg? Unless I've been missing out
on something.
> > "This setting determines whether the planner will elect to use a hash plan
> > which it expects will exceed work_mem and spill to disk. During execution,
> > hash nodes which exceed work_mem will spill to disk even if this setting is
> > disabled. To avoid spilling to disk, either increase work_mem (or set
> > enable_hashagg=off)."
> >
> > For sure the release notes should recommend re-calibrating work_mem.
>
> I don't follow. Why would the recalibrating be needed?
Because HashAgg plans which used to run fine (because they weren't prevented
from overflowing work_mem) might now run poorly after spilling to disk (because
of overflowing work_mem).
--
Justin
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-04-09 18:25 Jeff Davis <pgsql@j-davis.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 91+ messages in thread
From: Jeff Davis @ 2020-04-09 18:25 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: pgsql-hackers@postgresql.org
On Thu, 2020-04-09 at 12:24 -0500, Justin Pryzby wrote:
> Also.. there's no such thing as enable_groupagg? Unless I've been
> missing out
> on something.
I thought about adding that, and went so far as to make a patch. But it
didn't seem right to me -- the grouping isn't what takes the time, it's
the sorting. So what would the point of such a GUC be? To disable
GroupAgg when the input data is already sorted? Or a strange way to
disable Sort?
> Because HashAgg plans which used to run fine (because they weren't
> prevented
> from overflowing work_mem) might now run poorly after spilling to
> disk (because
> of overflowing work_mem).
It's probably worth a mention in the release notes, but I wouldn't word
it too strongly. Typically the performance difference is not a lot if
the workload still fits in system memory.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-04-09 19:26 Robert Haas <robertmhaas@gmail.com>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
1 sibling, 1 reply; 91+ messages in thread
From: Robert Haas @ 2020-04-09 19:26 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Jeff Davis <pgsql@j-davis.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Thu, Apr 9, 2020 at 7:49 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> It it really any different from our enable_* GUCs? Even if you do e.g.
> enable_sort=off, we may still do a sort. Same for enable_groupagg etc.
I think it's actually pretty different. All of the other enable_* GUCs
disable an entire type of plan node, except for cases where that would
otherwise result in planning failure. This just disables a portion of
the planning logic for a certain kind of node, without actually
disabling the whole node type. I'm not sure that's a bad idea, but it
definitely seems to be inconsistent with what we've done in the past.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-04-09 20:02 Jeff Davis <pgsql@j-davis.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-04-09 20:02 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Thu, 2020-04-09 at 15:26 -0400, Robert Haas wrote:
> I think it's actually pretty different. All of the other enable_*
> GUCs
> disable an entire type of plan node, except for cases where that
> would
> otherwise result in planning failure. This just disables a portion of
> the planning logic for a certain kind of node, without actually
> disabling the whole node type. I'm not sure that's a bad idea, but it
> definitely seems to be inconsistent with what we've done in the past.
The patch adds two GUCs. Both are slightly weird, to be honest, but let
me explain the reasoning. I am open to other suggestions.
1. enable_hashagg_disk (default true):
This is essentially there just to get some of the old behavior back, to
give people an escape hatch if they see bad plans while we are tweaking
the costing. The old behavior was weird, so this GUC is also weird.
Perhaps we can make this a compatibility GUC that we eventually drop? I
don't necessarily think this GUC would make sense, say, 5 versions from
now. I'm just trying to be conservative because I know that, even if
the plans are faster for 90% of people, the other 10% will be unhappy
and want a way to work around it.
2. enable_groupingsets_hash_disk (default false):
This is about how we choose which grouping sets to hash and which to
sort when generating mixed mode paths.
Even before this patch, there are quite a few paths that could be
generated. It tries to estimate the size of each grouping set's hash
table, and then see how many it can fit in work_mem (knapsack), while
also taking advantage of any path keys, etc.
With Disk-based Hash Aggregation, in principle we can generate paths
representing any combination of hashing and sorting for the grouping
sets. But that would be overkill (and grow to a huge number of paths if
we have more than a handful of grouping sets). So I think the existing
planner logic for grouping sets is fine for now. We might come up with
a better approach later.
But that created a testing problem, because if the planner estimates
correctly, no hashed grouping sets will spill, and the spilling code
won't be exercised. This GUC makes the planner disregard which grouping
sets' hash tables will fit, making it much easier to exercise the
spilling code. Is there a better way I should be testing this code
path?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-10 01:20 Melanie Plageman <melanieplageman@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 2 replies; 91+ messages in thread
From: Melanie Plageman @ 2020-06-10 01:20 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Thu, Apr 9, 2020 at 1:02 PM Jeff Davis <pgsql@j-davis.com> wrote:
> 2. enable_groupingsets_hash_disk (default false):
>
> This is about how we choose which grouping sets to hash and which to
> sort when generating mixed mode paths.
>
> Even before this patch, there are quite a few paths that could be
> generated. It tries to estimate the size of each grouping set's hash
> table, and then see how many it can fit in work_mem (knapsack), while
> also taking advantage of any path keys, etc.
>
> With Disk-based Hash Aggregation, in principle we can generate paths
> representing any combination of hashing and sorting for the grouping
> sets. But that would be overkill (and grow to a huge number of paths if
> we have more than a handful of grouping sets). So I think the existing
> planner logic for grouping sets is fine for now. We might come up with
> a better approach later.
>
> But that created a testing problem, because if the planner estimates
> correctly, no hashed grouping sets will spill, and the spilling code
> won't be exercised. This GUC makes the planner disregard which grouping
> sets' hash tables will fit, making it much easier to exercise the
> spilling code. Is there a better way I should be testing this code
> path?
>
>
So, I was catching up on email and noticed the last email in this
thread.
I think I am not fully understanding what enable_groupingsets_hash_disk
does. Is it only for testing?
Using the tests you added to src/test/regress/sql/groupingsets.sql, I
did get a plan that looks like hashagg is spilling to disk (goes through
hashagg_spill_tuple() code path and has number of batches reported in
Explain) in a MixedAgg plan for a grouping sets query even with
enable_groupingsets_hash_disk set to false. You don't have the exact
query I tried (below) in the test suite, but it is basically what is
already there, so I must be missing something.
set enable_hashagg_disk = true;
SET enable_groupingsets_hash_disk = false;
SET work_mem='64kB';
set enable_hashagg = true;
set jit_above_cost = 0;
drop table if exists gs_hash_1;
create table gs_hash_1 as
select g1000, g100, g10, sum(g::numeric), count(*), max(g::text) from
(select g%1000 as g1000, g%100 as g100, g%10 as g10, g
from generate_series(0,199999) g) s
group by cube (g1000,g100,g10);
explain (analyze, costs off, timing off)
select g1000, g100, g10
from gs_hash_1 group by cube (g1000,g100,g10);
QUERY PLAN
--------------------------------------------------------------
MixedAggregate (actual rows=9648 loops=1)
Hash Key: g10
Hash Key: g10, g1000
Hash Key: g100
Hash Key: g100, g10
Group Key: g1000, g100, g10
Group Key: g1000, g100
Group Key: g1000
Group Key: ()
Peak Memory Usage: 233 kB
Disk Usage: 1600 kB
HashAgg Batches: 2333
-> Sort (actual rows=4211 loops=1)
Sort Key: g1000, g100, g10
Sort Method: external merge Disk: 384kB
-> Seq Scan on gs_hash_1 (actual rows=4211 loops=1)
Anyway, when I throw in the stats trick that is used in join_hash.sql:
alter table gs_hash_1 set (autovacuum_enabled = 'false');
update pg_class set reltuples = 10 where relname = 'gs_hash_1';
I get a MixedAgg plan that doesn't have any Sort below and uses much
more disk.
QUERY PLAN
----------------------------------------------------------
MixedAggregate (actual rows=4211 loops=1)
Hash Key: g1000, g100, g10
Hash Key: g1000, g100
Hash Key: g1000
Hash Key: g100, g10
Hash Key: g100
Hash Key: g10, g1000
Hash Key: g10
Group Key: ()
Peak Memory Usage: 405 kB
Disk Usage: 59712 kB
HashAgg Batches: 4209
-> Seq Scan on gs_hash_1 (actual rows=200000 loops=1)
I'm not sure if this is more what you were looking for--or maybe I am
misunderstanding the guc.
--
Melanie Plageman
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-10 02:15 Justin Pryzby <pryzby@telsasoft.com>
parent: Melanie Plageman <melanieplageman@gmail.com>
1 sibling, 2 replies; 91+ messages in thread
From: Justin Pryzby @ 2020-06-10 02:15 UTC (permalink / raw)
To: Melanie Plageman <melanieplageman@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Tue, Jun 09, 2020 at 06:20:13PM -0700, Melanie Plageman wrote:
> On Thu, Apr 9, 2020 at 1:02 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> > 2. enable_groupingsets_hash_disk (default false):
> >
> > This is about how we choose which grouping sets to hash and which to
> > sort when generating mixed mode paths.
> >
> > Even before this patch, there are quite a few paths that could be
> > generated. It tries to estimate the size of each grouping set's hash
> > table, and then see how many it can fit in work_mem (knapsack), while
> > also taking advantage of any path keys, etc.
> >
> > With Disk-based Hash Aggregation, in principle we can generate paths
> > representing any combination of hashing and sorting for the grouping
> > sets. But that would be overkill (and grow to a huge number of paths if
> > we have more than a handful of grouping sets). So I think the existing
> > planner logic for grouping sets is fine for now. We might come up with
> > a better approach later.
> >
> > But that created a testing problem, because if the planner estimates
> > correctly, no hashed grouping sets will spill, and the spilling code
> > won't be exercised. This GUC makes the planner disregard which grouping
> > sets' hash tables will fit, making it much easier to exercise the
> > spilling code. Is there a better way I should be testing this code
> > path?
>
> So, I was catching up on email and noticed the last email in this
> thread.
>
> I think I am not fully understanding what enable_groupingsets_hash_disk
> does. Is it only for testing?
If so, it should be in category: "Developer Options".
> Using the tests you added to src/test/regress/sql/groupingsets.sql, I
> did get a plan that looks like hashagg is spilling to disk (goes through
> hashagg_spill_tuple() code path and has number of batches reported in
> Explain) in a MixedAgg plan for a grouping sets query even with
> enable_groupingsets_hash_disk set to false.
> I'm not sure if this is more what you were looking for--or maybe I am
> misunderstanding the guc.
The behavior of the GUC is inconsistent with the other GUCs, which is
confusing. See also Robert's comments in this thread.
https://www.postgresql.org/message-id/20200407223900.GT2228%40telsasoft.com
The old (pre-13) behavior was:
- work_mem is the amount of RAM to which each query node tries to constrain
itself, and the planner will reject a plan if it's expected to exceed that.
...But a chosen plan might exceed work_mem anyway.
The new behavior in v13 seems to be:
- HashAgg now respects work_mem, but instead enable*hash_disk are
opportunisitic. A node which is *expected* to spill to disk will be
rejected.
...But at execution time, a node which exceeds work_mem will be spilled.
If someone sees a plan which spills to disk and wants to improve performance by
avoid spilling, they might SET enable_hashagg_disk=off, which might do what
they want (if the plan is rejected at plan time), or it might not, which I
think will be a surprise every time.
If someone agrees, I suggest to add this as an Opened Item.
Maybe some combination of these would be an improvement:
- change documentation to emphasize behavior;
- change EXPLAIN ouput to make it obvious this isn't misbehaving;
- rename the GUC to not start with enable_* (work_mem_exceed?)
- rename the GUC *values* to something other than on/off. On/Planner?
- change the GUC to behave like it sounds like it should, which means "off"
would allow the pre-13 behavior of exceeding work_mem.
- Maybe make it ternary, like:
exceed_work_mem: {spill_disk, planner_reject, allow}
--
Justin
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-10 16:40 Melanie Plageman <melanieplageman@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 91+ messages in thread
From: Melanie Plageman @ 2020-06-10 16:40 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; Pg Hackers <pgsql-hackers@postgresql.org>
On Tue, Jun 9, 2020 at 7:15 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Tue, Jun 09, 2020 at 06:20:13PM -0700, Melanie Plageman wrote:
> > On Thu, Apr 9, 2020 at 1:02 PM Jeff Davis <pgsql@j-davis.com> wrote:
> >
> > > 2. enable_groupingsets_hash_disk (default false):
> > >
> > > This is about how we choose which grouping sets to hash and which to
> > > sort when generating mixed mode paths.
> > >
> > > Even before this patch, there are quite a few paths that could be
> > > generated. It tries to estimate the size of each grouping set's hash
> > > table, and then see how many it can fit in work_mem (knapsack), while
> > > also taking advantage of any path keys, etc.
> > >
> > > With Disk-based Hash Aggregation, in principle we can generate paths
> > > representing any combination of hashing and sorting for the grouping
> > > sets. But that would be overkill (and grow to a huge number of paths if
> > > we have more than a handful of grouping sets). So I think the existing
> > > planner logic for grouping sets is fine for now. We might come up with
> > > a better approach later.
> > >
> > > But that created a testing problem, because if the planner estimates
> > > correctly, no hashed grouping sets will spill, and the spilling code
> > > won't be exercised. This GUC makes the planner disregard which grouping
> > > sets' hash tables will fit, making it much easier to exercise the
> > > spilling code. Is there a better way I should be testing this code
> > > path?
> >
> > So, I was catching up on email and noticed the last email in this
> > thread.
> >
> > I think I am not fully understanding what enable_groupingsets_hash_disk
> > does. Is it only for testing?
>
> If so, it should be in category: "Developer Options".
>
> > Using the tests you added to src/test/regress/sql/groupingsets.sql, I
> > did get a plan that looks like hashagg is spilling to disk (goes through
> > hashagg_spill_tuple() code path and has number of batches reported in
> > Explain) in a MixedAgg plan for a grouping sets query even with
> > enable_groupingsets_hash_disk set to false.
>
> > I'm not sure if this is more what you were looking for--or maybe I am
> > misunderstanding the guc.
>
> The behavior of the GUC is inconsistent with the other GUCs, which is
> confusing. See also Robert's comments in this thread.
> https://www.postgresql.org/message-id/20200407223900.GT2228%40telsasoft.com
>
> The old (pre-13) behavior was:
> - work_mem is the amount of RAM to which each query node tries to
> constrain
> itself, and the planner will reject a plan if it's expected to exceed
> that.
> ...But a chosen plan might exceed work_mem anyway.
>
> The new behavior in v13 seems to be:
> - HashAgg now respects work_mem, but instead enable*hash_disk are
> opportunisitic. A node which is *expected* to spill to disk will be
> rejected.
> ...But at execution time, a node which exceeds work_mem will be spilled.
>
> If someone sees a plan which spills to disk and wants to improve
> performance by
> avoid spilling, they might SET enable_hashagg_disk=off, which might do what
> they want (if the plan is rejected at plan time), or it might not, which I
> think will be a surprise every time.
>
>
But I thought that the enable_groupingsets_hash_disk GUC allows us to
test the following scenario:
The following is true:
- planner thinks grouping sets' hashtables table would fit in memory
(spilling is *not* expected)
- user is okay with spilling
- some grouping keys happen to be sortable and some hashable
The following happens:
- Planner generates some HashAgg grouping sets paths
- A MixedAgg plan is created
- During execution of the MixedAgg plan, one or more grouping sets'
hashtables would exceed work_mem, so the executor spills those tuples
to disk instead of exceeding work_mem
Especially given the code and comment:
/*
* If we have sortable columns to work with (gd->rollups is non-empty)
* and enable_groupingsets_hash_disk is disabled, don't generate
* hash-based paths that will exceed work_mem.
*/
if (!enable_groupingsets_hash_disk &&
hashsize > work_mem * 1024L && gd->rollups)
return; /* nope, won't fit */
If this is the scenario that the GUC is designed to test, it seems like
you could exercise it without the enable_groupingsets_hash_disk GUC by
lying about the stats, no?
--
Melanie Plageman
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-10 17:39 Jeff Davis <pgsql@j-davis.com>
parent: Melanie Plageman <melanieplageman@gmail.com>
1 sibling, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-10 17:39 UTC (permalink / raw)
To: Melanie Plageman <melanieplageman@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Tue, 2020-06-09 at 18:20 -0700, Melanie Plageman wrote:
> So, I was catching up on email and noticed the last email in this
> thread.
>
> I think I am not fully understanding what
> enable_groupingsets_hash_disk
> does. Is it only for testing?
It's mostly for testing. I could imagine cases where it would be useful
to force groupingsets to use the disk, but I mainly wanted the setting
there for testing the grouping sets hash disk code path.
> Using the tests you added to src/test/regress/sql/groupingsets.sql, I
> did get a plan that looks like hashagg is spilling to disk (goes
> through
I had something that worked as a test for a while, but then when I
tweaked the costing, it started using the Sort path (therefore not
testing my grouping sets hash disk code at all) and a bug crept in. So
I thought it would be best to have a more forceful knob.
Perhaps I should just get rid of that GUC and use the stats trick?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-10 18:39 Jeff Davis <pgsql@j-davis.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 2 replies; 91+ messages in thread
From: Jeff Davis @ 2020-06-10 18:39 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Tue, 2020-06-09 at 21:15 -0500, Justin Pryzby wrote:
> The behavior of the GUC is inconsistent with the other GUCs, which is
> confusing. See also Robert's comments in this thread.
>
https://www.postgresql.org/message-id/20200407223900.GT2228%40telsasoft.com
enable_* GUCs are planner GUCs, so it would be confusing to me if they
affected execution-time behavior.
I think the point of confusion is that it's not enabling/disabling an
entire execution node; it only "disables" HashAgg if it thinks it will
spill. I agree that is a difference with the other GUCs, and could
cause confusion.
Stepping back, I was trying to solve two problems with these GUCs:
1. Testing the spilling of hashed grouping sets: I'm inclined to just
get rid of enable_groupingsets_hash_disk and use Melanie's stats-
hacking approach instead.
2. Trying to provide an escape hatch for someone who experiences a
performance regression and wants something like the old behavior back.
There are two aspects of the old behavior that a user could potentially
want back:
a. Don't choose HashAgg if it's expected to have more groups than fit
into a work_mem-sized hashtable.
b. If executing HashAgg, and the hash table exceeds work_mem, just
keep going.
The behavior in v13 master is, by default, analagous to Sort or
anything else that adapts at runtime to spill. If we had spillable
HashAgg the whole time, we wouldn't be worried about #2 at all. But,
out of conservatism, I am trying to accommodate users who want an
escape hatch, at least for a release or two until users feel more
comfortable with disk-based HashAgg.
Setting enable_hash_disk=false implements 2(a). This name apparently
causes confusion, but it's hard to come up with a better one because
the v12 behavior has nuance that's hard to express succinctly. I don't
think the names you suggested quite fit, but the idea to use a more
interesting GUC value might help express the behavior. Perhaps making
enable_hashagg a ternary "enable_hashagg=on|off|avoid_disk"? The word
"reject" is too definite for the planner, which is working with
imperfect information.
In master, there is no explicit way to get 2(b), but you can just set
work_mem higher in a lot of cases. If enough people want 2(b), I can
add it easily. Perhaps hashagg_overflow=on|off, which would control
execution time behavior?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-10 18:52 Jeff Davis <pgsql@j-davis.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 0 replies; 91+ messages in thread
From: Jeff Davis @ 2020-06-10 18:52 UTC (permalink / raw)
To: pgsql-hackers@postgresql.org
On Tue, 2020-04-07 at 11:20 -0700, Jeff Davis wrote:
> Now that we have Disk-based Hash Aggregation, there are a lot more
> situations where the planner can choose HashAgg. The
> enable_hashagg_disk GUC, if set to true, chooses HashAgg based on
> costing. If false, it only generates a HashAgg path if it thinks it
> will fit in work_mem, similar to the old behavior (though it wlil now
> spill to disk if the planner was wrong about it fitting in work_mem).
> The current default is true.
>
> I expect this to be a win in a lot of cases, obviously. But as with
> any
> planner change, it will be wrong sometimes. We may want to be
> conservative and set the default to false depending on the experience
> during beta. I'm inclined to leave it as true for now though, because
> that will give us better information upon which to base any decision.
A compromise may be to multiply the disk costs for HashAgg by, e.g. a
1.5 - 2X penalty. That would make the plan changes less abrupt, and may
mitigate some of the concerns about I/O patterns that Tomas raised
here:
https://www.postgresql.org/message-id/20200519151202.u2p2gpiawoaznsv2@development
The issues were improved a lot, but it will take us a while to really
tune the IO behavior as well as Sort.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-11 00:48 Melanie Plageman <melanieplageman@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 0 replies; 91+ messages in thread
From: Melanie Plageman @ 2020-06-11 00:48 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Wed, Jun 10, 2020 at 10:39 AM Jeff Davis <pgsql@j-davis.com> wrote:
> On Tue, 2020-06-09 at 18:20 -0700, Melanie Plageman wrote:
> > So, I was catching up on email and noticed the last email in this
> > thread.
> >
> > I think I am not fully understanding what
> > enable_groupingsets_hash_disk
> > does. Is it only for testing?
>
> It's mostly for testing. I could imagine cases where it would be useful
> to force groupingsets to use the disk, but I mainly wanted the setting
> there for testing the grouping sets hash disk code path.
>
> > Using the tests you added to src/test/regress/sql/groupingsets.sql, I
> > did get a plan that looks like hashagg is spilling to disk (goes
> > through
>
> I had something that worked as a test for a while, but then when I
> tweaked the costing, it started using the Sort path (therefore not
> testing my grouping sets hash disk code at all) and a bug crept in. So
> I thought it would be best to have a more forceful knob.
>
> Perhaps I should just get rid of that GUC and use the stats trick?
>
>
I like the idea of doing the stats trick. For extra security, you could
throw in that other trick that is used in groupingsets.sql and make some
of the grouping columns unhashable and some unsortable so you know that
you will not pick only the Sort Path and do just a GroupAgg.
This slight modification of my previous example will probably yield
consistent results:
set enable_hashagg_disk = true;
SET enable_groupingsets_hash_disk = false;
SET work_mem='64kB';
SET enable_hashagg = true;
drop table if exists gs_hash_1;
create table gs_hash_1 as
select g%1000 as g1000, g%100 as g100, g%10 as g10, g,
g::text::xid as g_unsortable, g::bit(4) as g_unhashable
from generate_series(0,199999) g;
analyze gs_hash_1;
alter table gs_hash_1 set (autovacuum_enabled = 'false');
update pg_class set reltuples = 10 where relname = 'gs_hash_1';
explain (analyze, costs off, timing off)
select g1000, g100, g10
from gs_hash_1
group by grouping sets ((g1000,g100), (g10, g_unhashable), (g100,
g_unsortable));
QUERY PLAN
----------------------------------------------------------------
MixedAggregate (actual rows=201080 loops=1)
Hash Key: g100, g_unsortable
Group Key: g1000, g100
Sort Key: g10, g_unhashable
Group Key: g10, g_unhashable
Peak Memory Usage: 109 kB
Disk Usage: 13504 kB
HashAgg Batches: 10111
-> Sort (actual rows=200000 loops=1)
Sort Key: g1000, g100
Sort Method: external merge Disk: 9856kB
-> Seq Scan on gs_hash_1 (actual rows=200000 loops=1)
While we are on the topic of the tests, I was wondering if you had
considered making a user defined type that had a lot of padding so that
the tests could use fewer rows. I did this for adaptive hashjoin and it
helped me with iteration time.
I don't know if that would still be the kind of test you are looking for
since a user probably wouldn't have a couple hundred really fat
untoasted tuples, but, I just thought I would check if that would be
useful.
--
Melanie Plageman
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-11 20:22 Jeff Davis <pgsql@j-davis.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 2 replies; 91+ messages in thread
From: Jeff Davis @ 2020-06-11 20:22 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Wed, 2020-06-10 at 11:39 -0700, Jeff Davis wrote:
> 1. Testing the spilling of hashed grouping sets: I'm inclined to just
> get rid of enable_groupingsets_hash_disk and use Melanie's stats-
> hacking approach instead.
Fixed in 92c58fd9.
> think the names you suggested quite fit, but the idea to use a more
> interesting GUC value might help express the behavior. Perhaps making
> enable_hashagg a ternary "enable_hashagg=on|off|avoid_disk"? The word
> "reject" is too definite for the planner, which is working with
> imperfect information.
I renamed enable_hashagg_disk to hashagg_avoid_disk_plan, which I think
satisfies the concerns raised here. Also in 92c58fd9.
There is still the original topic of this thread, which is whether we
need to change the default value of this GUC, or penalize disk-based
HashAgg in some way, to be more conservative about plan changes in v13.
I think we can wait a little longer to make a decision there.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-12 02:45 Justin Pryzby <pryzby@telsasoft.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 0 replies; 91+ messages in thread
From: Justin Pryzby @ 2020-06-12 02:45 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Melanie Plageman <melanieplageman@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Thu, Jun 11, 2020 at 01:22:57PM -0700, Jeff Davis wrote:
> On Wed, 2020-06-10 at 11:39 -0700, Jeff Davis wrote:
> > 1. Testing the spilling of hashed grouping sets: I'm inclined to just
> > get rid of enable_groupingsets_hash_disk and use Melanie's stats-
> > hacking approach instead.
>
> Fixed in 92c58fd9.
>
> > think the names you suggested quite fit, but the idea to use a more
> > interesting GUC value might help express the behavior. Perhaps making
> > enable_hashagg a ternary "enable_hashagg=on|off|avoid_disk"? The word
> > "reject" is too definite for the planner, which is working with
> > imperfect information.
>
> I renamed enable_hashagg_disk to hashagg_avoid_disk_plan, which I think
> satisfies the concerns raised here. Also in 92c58fd9.
Thanks for considering :)
I saw you updated the Open Items page, but put the items into "Older Bugs /
Fixed".
I moved them underneath "Resolved" since they're all new in v13.
https://wiki.postgresql.org/index.php?title=PostgreSQL_13_Open_Items&diff=34995&oldid=34994
--
Justin
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-12 03:35 Greg Stark <stark@mit.edu>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 91+ messages in thread
From: Greg Stark @ 2020-06-12 03:35 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>; Jeff Davis <pgsql@j-davis.com>; PostgreSQL-development <pgsql-hackers@postgresql.org>
On Thu, 9 Apr 2020 at 13:24, Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Thu, Apr 09, 2020 at 01:48:55PM +0200, Tomas Vondra wrote:
>
> > It it really any different from our enable_* GUCs? Even if you do e.g.
> > enable_sort=off, we may still do a sort. Same for enable_groupagg etc.
>
> Those show that the GUC was disabled by showing disable_cost. That's
> what's
> different about this one.
>
Fwiw in the past this was seen not so much as a positive thing but a bug to
be fixed. We've talked about carrying a boolean "disabled plan" flag which
would be treated as a large cost penalty but not actually be added to the
cost in the plan.
The problems with the disable_cost in the cost are (at least):
1) It causes the resulting costs to be useless for comparing the plan costs
with other plans.
2) It can cause other planning decisions to be distorted in strange
non-linear ways.
--
greg
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-20 22:04 Justin Pryzby <pryzby@telsasoft.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 0 replies; 91+ messages in thread
From: Justin Pryzby @ 2020-06-20 22:04 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Melanie Plageman <melanieplageman@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Thu, Jun 11, 2020 at 01:22:57PM -0700, Jeff Davis wrote:
> > think the names you suggested quite fit, but the idea to use a more
> > interesting GUC value might help express the behavior. Perhaps making
> > enable_hashagg a ternary "enable_hashagg=on|off|avoid_disk"? The word
> > "reject" is too definite for the planner, which is working with
> > imperfect information.
>
> I renamed enable_hashagg_disk to hashagg_avoid_disk_plan, which I think
> satisfies the concerns raised here. Also in 92c58fd9.
I think this should be re-arranged to be in alphabetical order
https://www.postgresql.org/docs/devel/runtime-config-query.html
--
Justin
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-22 14:52 Robert Haas <robertmhaas@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 2 replies; 91+ messages in thread
From: Robert Haas @ 2020-06-22 14:52 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Wed, Jun 10, 2020 at 2:39 PM Jeff Davis <pgsql@j-davis.com> wrote:
> The behavior in v13 master is, by default, analagous to Sort or
> anything else that adapts at runtime to spill. If we had spillable
> HashAgg the whole time, we wouldn't be worried about #2 at all. But,
> out of conservatism, I am trying to accommodate users who want an
> escape hatch, at least for a release or two until users feel more
> comfortable with disk-based HashAgg.
>
> Setting enable_hash_disk=false implements 2(a). This name apparently
> causes confusion, but it's hard to come up with a better one because
> the v12 behavior has nuance that's hard to express succinctly. I don't
> think the names you suggested quite fit, but the idea to use a more
> interesting GUC value might help express the behavior. Perhaps making
> enable_hashagg a ternary "enable_hashagg=on|off|avoid_disk"? The word
> "reject" is too definite for the planner, which is working with
> imperfect information.
>
> In master, there is no explicit way to get 2(b), but you can just set
> work_mem higher in a lot of cases. If enough people want 2(b), I can
> add it easily. Perhaps hashagg_overflow=on|off, which would control
> execution time behavior?
Planner GUCs are a pretty blunt instrument for solving problems that
users may have with planner features. There's no guarantee that the
experience a user has with one query will be the same as the
experience they have with another query, or even that you couldn't
have a single query which contains two different nodes where the
optimal behavior is different for one than it is for the other. In the
first case, changing the value of the GUC on a per-query basis is
pretty painful; in the second case, even that is not good enough. So,
as Tom has said before, the only really good choice in a case like
this is for the planner to figure out the right things automatically;
anything that boils down to a user-provided hint pretty well sucks.
So I feel like the really important thing here is to fix the cases
that don't come out well with default settings. If we can't do that,
then the feature is half-baked and maybe should not have been
committed in the first place. If we can, then we don't really need the
GUC, let alone multiple GUCs. I understand that some of the reason you
added these was out of paranoia, and I get that: it's hard to be sure
that any feature of this complexity isn't going to have some rough
patches, especially given how defective work_mem is as a model in
general. Still, we don't want to end up with 50 planner GUCs enabling
and disabling individual bits of various planner nodes, or at least I
don't think we do, so I'm very skeptical of the idea that we need 2
just for this feature. That doesn't feel scalable. I think the right
number is 0 or 1, and if it's 1, very few people should be changing
the default. If anything else is the case, then IMHO the feature isn't
ready to ship.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-22 15:06 Justin Pryzby <pryzby@telsasoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 91+ messages in thread
From: Justin Pryzby @ 2020-06-22 15:06 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Mon, Jun 22, 2020 at 10:52:37AM -0400, Robert Haas wrote:
> On Wed, Jun 10, 2020 at 2:39 PM Jeff Davis <pgsql@j-davis.com> wrote:
> > The behavior in v13 master is, by default, analagous to Sort or
> > anything else that adapts at runtime to spill. If we had spillable
> > HashAgg the whole time, we wouldn't be worried about #2 at all. But,
> > out of conservatism, I am trying to accommodate users who want an
> > escape hatch, at least for a release or two until users feel more
> > comfortable with disk-based HashAgg.
> >
> > Setting enable_hash_disk=false implements 2(a). This name apparently
> > causes confusion, but it's hard to come up with a better one because
> > the v12 behavior has nuance that's hard to express succinctly. I don't
> > think the names you suggested quite fit, but the idea to use a more
> > interesting GUC value might help express the behavior. Perhaps making
> > enable_hashagg a ternary "enable_hashagg=on|off|avoid_disk"? The word
> > "reject" is too definite for the planner, which is working with
> > imperfect information.
> >
> > In master, there is no explicit way to get 2(b), but you can just set
> > work_mem higher in a lot of cases. If enough people want 2(b), I can
> > add it easily. Perhaps hashagg_overflow=on|off, which would control
> > execution time behavior?
>
> don't think we do, so I'm very skeptical of the idea that we need 2
> just for this feature. That doesn't feel scalable. I think the right
> number is 0 or 1, and if it's 1, very few people should be changing
> the default. If anything else is the case, then IMHO the feature isn't
> ready to ship.
This was addressed in 92c58fd94801dd5c81ee20e26c5bb71ad64552a8
https://wiki.postgresql.org/index.php?title=PostgreSQL_13_Open_Items&diff=34994&oldid=34993
--
Justin
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-22 15:17 Robert Haas <robertmhaas@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 91+ messages in thread
From: Robert Haas @ 2020-06-22 15:17 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, Jun 22, 2020 at 11:06 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> This was addressed in 92c58fd94801dd5c81ee20e26c5bb71ad64552a8
> https://wiki.postgresql.org/index.php?title=PostgreSQL_13_Open_Items&diff=34994&oldid=34993
I mean, that's fine, but I am trying to make a more general point
about priorities. Getting the GUCs right is a lot less important than
getting the feature right.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-22 17:30 Jeff Davis <pgsql@j-davis.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-22 17:30 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, 2020-06-22 at 10:52 -0400, Robert Haas wrote:
> So I feel like the really important thing here is to fix the cases
> that don't come out well with default settings.
...with the caveat that perfection is not something to expect from our
planner.
> If we can't do that,
> then the feature is half-baked and maybe should not have been
> committed in the first place.
HashAgg started out half-baked at the dawn of time, and stayed that way
through version 12. Disk-based HashAgg was designed to fix it.
Other major planner features generally offer a way to turn them off
(e.g. parallelism, JIT), and we don't call those half-baked.
I agree that the single GUC added in v13 (hashagg_avoid_disk_plan) is
weird because it's half of a disable switch. But it's not weird because
of my changes in v13; it's weird because the planner behavior in v12
was weird. I hope not many people need to set it, and I hope we can
remove it soon.
If you think we will never be able to remove the GUC, then we should
think a little harder about whether we really need it. I am open to
that discussion, but I don't think the presence of this GUC implies
that disk-based hashagg is half-baked.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-22 17:44 Jeff Davis <pgsql@j-davis.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 0 replies; 91+ messages in thread
From: Jeff Davis @ 2020-06-22 17:44 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; +Cc: Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, 2020-06-22 at 11:17 -0400, Robert Haas wrote:
> I mean, that's fine, but I am trying to make a more general point
> about priorities. Getting the GUCs right is a lot less important than
> getting the feature right.
What about the feature you are worried that we're getting wrong?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-22 19:28 Robert Haas <robertmhaas@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 91+ messages in thread
From: Robert Haas @ 2020-06-22 19:28 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, Jun 22, 2020 at 1:30 PM Jeff Davis <pgsql@j-davis.com> wrote:
> On Mon, 2020-06-22 at 10:52 -0400, Robert Haas wrote:
> > So I feel like the really important thing here is to fix the cases
> > that don't come out well with default settings.
>
> ...with the caveat that perfection is not something to expect from our
> planner.
+1.
> > If we can't do that,
> > then the feature is half-baked and maybe should not have been
> > committed in the first place.
>
> HashAgg started out half-baked at the dawn of time, and stayed that way
> through version 12. Disk-based HashAgg was designed to fix it.
>
> Other major planner features generally offer a way to turn them off
> (e.g. parallelism, JIT), and we don't call those half-baked.
Sure, and I'm not calling this half-baked either, but there is a
difference. JIT and parallelism are discrete features to a far greater
extent than this is. I think we can explain to people the pros and
cons of those things and ask them to make an intelligent choice about
whether they want them. You can say things like "well, JIT is liable
to make your queries run faster once they get going, but it adds to
the startup time and creates a dependency on LLVM" and the user can
decide whether they want that or not. At least to me, something like
this isn't so easy to consider as a separate feature. As you say:
> I agree that the single GUC added in v13 (hashagg_avoid_disk_plan) is
> weird because it's half of a disable switch. But it's not weird because
> of my changes in v13; it's weird because the planner behavior in v12
> was weird. I hope not many people need to set it, and I hope we can
> remove it soon.
The weirdness is the problem here, at least for me. Generally, I don't
like GUCs of the form give_me_the_old_strange_behavior=true, because
either they tend to be either unnecessary (because nobody wants the
old strange behavior) or hard to eliminate (because the new behavior
is also strange and is not categorically better).
> If you think we will never be able to remove the GUC, then we should
> think a little harder about whether we really need it. I am open to
> that discussion, but I don't think the presence of this GUC implies
> that disk-based hashagg is half-baked.
I don't think it necessarily implies that either. I do however have
some concerns about people using the GUC as a crutch. I am slightly
worried that this is going to have hard-to-fix problems and that we'll
be stuck with the GUC for that reason. Now if that is the case, is
removing the GUC any better? Maybe not. These decisions are hard, and
I am not trying to pretend like I have all the answers.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-22 20:23 Jeff Davis <pgsql@j-davis.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-22 20:23 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, 2020-06-22 at 15:28 -0400, Robert Haas wrote:
> The weirdness is the problem here, at least for me. Generally, I
> don't
> like GUCs of the form give_me_the_old_strange_behavior=true
I agree with all of that in general.
> I don't think it necessarily implies that either. I do however have
> some concerns about people using the GUC as a crutch.
Another way of looking at it is that the weird behavior is already
there in v12, so there are already users relying on this weird behavior
as a crutch for some other planner mistake. The question is whether we
want to:
(a) take the weird behavior away now as a consequence of implementing
disk-based HashAgg; or
(b) support the weird behavior forever; or
(c) introduce a GUC now to help transition away from the weird behavior
The danger with (c) is that it gives users more time to become more
reliant on the weird behavior; and worse, a GUC could be seen as an
endorsement of the weird behavior rather than a path to eliminating it.
So we could intend to do (c) and end up with (b). We can mitigate this
with documentation warnings, perhaps.
> I am slightly
> worried that this is going to have hard-to-fix problems and that
> we'll
> be stuck with the GUC for that reason.
Without the GUC, it's basically a normal cost-based decision, with all
of the good and bad that comes with that.
> Now if that is the case, is
> removing the GUC any better? Maybe not. These decisions are hard, and
> I am not trying to pretend like I have all the answers.
I agree that there is no easy answer.
My philosophy here is: if a user does experience a plan regression due
to my change, would it be reasonable to tell them that we don't have
any escape hatch or transition period at all? That would be a tough
sell for such a common plan type.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 02:11 David Rowley <dgrowleyml@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 3 replies; 91+ messages in thread
From: David Rowley @ 2020-06-24 02:11 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Robert Haas <robertmhaas@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>
On Tue, 23 Jun 2020 at 08:24, Jeff Davis <pgsql@j-davis.com> wrote:
> Another way of looking at it is that the weird behavior is already
> there in v12, so there are already users relying on this weird behavior
> as a crutch for some other planner mistake. The question is whether we
> want to:
>
> (a) take the weird behavior away now as a consequence of implementing
> disk-based HashAgg; or
> (b) support the weird behavior forever; or
> (c) introduce a GUC now to help transition away from the weird behavior
>
> The danger with (c) is that it gives users more time to become more
> reliant on the weird behavior; and worse, a GUC could be seen as an
> endorsement of the weird behavior rather than a path to eliminating it.
> So we could intend to do (c) and end up with (b). We can mitigate this
> with documentation warnings, perhaps.
So, I have a few thoughts on this subject. I understand both problem
cases have been mentioned before on this thread, but just to reiterate
the two problem cases that we really would rather people didn't hit.
They are:
1. Statistics underestimation can cause hashagg to be selected. The
executor will spill to disk in PG13. Users may find performance
suffers as previously the query may have just overshot work_mem
without causing any OOM issues. Their I/O performance might be
terrible.
2. We might now choose to hash aggregate where pre PG13, we didn't
choose that because the hash table was estimated to be bigger than
work_mem. Hash agg might not be the best plan for the job.
For #1. We know users are forced to run smaller work_mems than they
might like as they need to allow for that random moment where all
backends happen to be doing that 5-way hash join all at the same time.
It seems reasonable that someone might want the old behaviour. They
may well be sitting on a timebomb that's about to OOM, but it would be
sad if someone's upgrade to PG13 was blocked on this, especially if
it's just due to some query that runs once per month but needs to
perform quickly.
For #2. This seems like a very legitimate requirement to me. If a
user is unhappy that PG13 now hashaggs where before it sorted and
group aggregated, but they're unhappy, not because there's some issue
with hashagg spilling, but because that causes the node above the agg
to becomes a Hash Join rather than a Merge Join and that's bad for
some existing reason. Our planner doing the wrong thing based on
either; lack of, inaccurate or out-of-date statistics is not Jeff's
fault. Having the ability to switch off a certain planner feature is
just following along with what we do today for many other node types.
As for GUCs to try to help the group of users who, *I'm certain*, will
have problems with PG13's plan choice. I think the overloaded
enable_hashagg option is a really nice compromise. We don't really
have any other executor node type that has multiple GUCs controlling
its behaviour, so I believe it would be nice to keep it that way.
How about:
enable_hashagg = "on" -- enables hashagg allowing it to freely spill
to disk as it pleases.
enable_hashagg = "trynospill" -- Planner will only choose hash_agg if
it thinks it won't spill (pre PG13 planner behaviour)
enable_hashagg = "neverspill" -- executor will *never* spill to disk
and can still OOM (NOT RECOMMENDED, but does give pre PG13 planner and
executor behaviour)
enable_hashagg = "off" -- planner does not consider hash agg, ever.
Same as what PG12 did for this setting.
Now, it's a bit weird to have "neverspill" as this is controlling
what's done in the executor from a planner GUC. Likely we can just
work around that by having a new "allowhashspill" bool field in the
"Agg" struct that's set by the planner, say during createplan that
controls if nodeAgg.c is allowed to spill or not. That'll also allow
PREPAREd plans to continue to do what they had planned to do already.
The thing I like about doing it this way is that:
a) it does not add any new GUCs
b) it semi hides the weird values that we really wish nobody would
ever have to set in a GUC that people have become used it just
allowing the values "on" and "off".
The thing I don't quite like about this idea is:
a) I wish the planner was perfect and we didn't need to do this.
b) It's a bit weird to overload a GUC that has a very booleanish name
to not be bool.
However, I also think it's pretty lightweight to support this. I
imagine a dozen lines of docs and likely about half a dozen lines per
GUC option in the planner.
And in the future, when our planner is perfect*, we can easily just
remove the enum values from the GUC that we no longer want to support.
David
* Yes I know that will never happen.
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 03:14 Justin Pryzby <pryzby@telsasoft.com>
parent: David Rowley <dgrowleyml@gmail.com>
2 siblings, 0 replies; 91+ messages in thread
From: Justin Pryzby @ 2020-06-24 03:14 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Wed, Jun 24, 2020 at 02:11:57PM +1200, David Rowley wrote:
> On Tue, 23 Jun 2020 at 08:24, Jeff Davis <pgsql@j-davis.com> wrote:
> > Another way of looking at it is that the weird behavior is already
> > there in v12, so there are already users relying on this weird behavior
> > as a crutch for some other planner mistake. The question is whether we
> > want to:
Yea - "behavior change" is a scenario for which it's hard to anticipate well
all the range of consequences.
> How about:
>
> enable_hashagg = "on" -- enables hashagg allowing it to freely spill
> to disk as it pleases.
> enable_hashagg = "trynospill" -- Planner will only choose hash_agg if
> it thinks it won't spill (pre PG13 planner behaviour)
> enable_hashagg = "neverspill" -- executor will *never* spill to disk
> and can still OOM (NOT RECOMMENDED, but does give pre PG13 planner and
> executor behaviour)
> enable_hashagg = "off" -- planner does not consider hash agg, ever.
> Same as what PG12 did for this setting.
+1
I like that this allows the new behavior as an *option* one *can* use rather
than a "behavior change" which is imposed on users and which users then *have*
to accomodate in postgres.
--
Justin
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 09:06 Bruce Momjian <bruce@momjian.us>
parent: David Rowley <dgrowleyml@gmail.com>
2 siblings, 2 replies; 91+ messages in thread
From: Bruce Momjian @ 2020-06-24 09:06 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@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>
On Wed, Jun 24, 2020 at 02:11:57PM +1200, David Rowley wrote:
> On Tue, 23 Jun 2020 at 08:24, Jeff Davis <pgsql@j-davis.com> wrote:
> > Another way of looking at it is that the weird behavior is already
> > there in v12, so there are already users relying on this weird behavior
> > as a crutch for some other planner mistake. The question is whether we
> > want to:
> >
> > (a) take the weird behavior away now as a consequence of implementing
> > disk-based HashAgg; or
> > (b) support the weird behavior forever; or
> > (c) introduce a GUC now to help transition away from the weird behavior
> >
> > The danger with (c) is that it gives users more time to become more
> > reliant on the weird behavior; and worse, a GUC could be seen as an
> > endorsement of the weird behavior rather than a path to eliminating it.
> > So we could intend to do (c) and end up with (b). We can mitigate this
> > with documentation warnings, perhaps.
>
> So, I have a few thoughts on this subject. I understand both problem
> cases have been mentioned before on this thread, but just to reiterate
> the two problem cases that we really would rather people didn't hit.
I appreciated this summary since I wasn't fully following the issues.
> As for GUCs to try to help the group of users who, *I'm certain*, will
> have problems with PG13's plan choice. I think the overloaded
> enable_hashagg option is a really nice compromise. We don't really
> have any other executor node type that has multiple GUCs controlling
> its behaviour, so I believe it would be nice to keep it that way.
So, in trying to anticipate how users will be affected by an API change,
I try to look at similar cases where we already have this behavior, and
how users react to this. Looking at the available join methods, I think
we have one. We currently support:
* nested loop with sequential scan
* nested loop with index scan
* hash join
* merge join
It would seem merge join has almost the same complexities as the new
hash join code, since it can spill to disk doing sorts for merge joins,
and adjusting work_mem is the only way to control that spill to disk. I
don't remember anyone complaining about spills to disk during merge
join, so I am unclear why we would need a such control for hash join.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 12:24 David Rowley <dgrowleyml@gmail.com>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 1 reply; 91+ messages in thread
From: David Rowley @ 2020-06-24 12:24 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@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>
On Wed, 24 Jun 2020 at 21:06, Bruce Momjian <bruce@momjian.us> wrote:
> I
> don't remember anyone complaining about spills to disk during merge
> join, so I am unclear why we would need a such control for hash join.
Hash aggregate, you mean? The reason is that upgrading to PG13 can
cause a performance regression for an underestimated ndistinct on the
GROUP BY clause and cause hash aggregate to spill to disk where it
previously did everything in RAM. Sure, that behaviour was never
what we wanted to happen, Jeff has fixed that now, but the fact
remains that this does happen in the real world quite often and people
often get away with it, likey because work_mem is generally set to
some very conservative value. Of course, there's also a bunch of
people that have been bitten by OOM due to this too. The "neverspill"
wouldn't be for those people. Certainly, it's possible that we just
tell these people to increase work_mem for this query, that way they
can set it to something reasonable and still get spilling if it's
really needed to save them from OOM, but the problem there is that
it's not very easy to go and set work_mem for a certain query.
FWIW, I wish that I wasn't suggesting we do this, but I am because it
seems simple enough to implement and it removes a pretty big roadblock
that might exist for a small subset of people wishing to upgrade to
PG13. It seems lightweight enough to maintain, at least until we
invent some better management of how many executor nodes we can have
allocating work_mem at once.
The suggestion I made was just based on asking myself the following
set of questions:
Since Hash Aggregate has been able to overflow work_mem since day 1,
and now that we've completely changed that fact in PG13, is that
likely to upset anyone? If so, should we go to the trouble of giving
those people a way of getting the old behaviour back? If we do want to
help those people, what's the best way to make those options available
to them in a way that we can remove the special options with the least
pain in some future version of PostgreSQL?
I'd certainly be interested in hearing how other people would answer
those question.
David
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 12:38 Justin Pryzby <pryzby@telsasoft.com>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 2 replies; 91+ messages in thread
From: Justin Pryzby @ 2020-06-24 12:38 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Wed, Jun 24, 2020 at 05:06:28AM -0400, Bruce Momjian wrote:
> On Wed, Jun 24, 2020 at 02:11:57PM +1200, David Rowley wrote:
> > On Tue, 23 Jun 2020 at 08:24, Jeff Davis <pgsql@j-davis.com> wrote:
> > > Another way of looking at it is that the weird behavior is already
> > > there in v12, so there are already users relying on this weird behavior
> > > as a crutch for some other planner mistake. The question is whether we
> > > want to:
> > >
> > > (a) take the weird behavior away now as a consequence of implementing
> > > disk-based HashAgg; or
> > > (b) support the weird behavior forever; or
> > > (c) introduce a GUC now to help transition away from the weird behavior
> > >
> > > The danger with (c) is that it gives users more time to become more
> > > reliant on the weird behavior; and worse, a GUC could be seen as an
> > > endorsement of the weird behavior rather than a path to eliminating it.
> > > So we could intend to do (c) and end up with (b). We can mitigate this
> > > with documentation warnings, perhaps.
> >
> > So, I have a few thoughts on this subject. I understand both problem
> > cases have been mentioned before on this thread, but just to reiterate
> > the two problem cases that we really would rather people didn't hit.
>
> I appreciated this summary since I wasn't fully following the issues.
>
> > As for GUCs to try to help the group of users who, *I'm certain*, will
> > have problems with PG13's plan choice. I think the overloaded
> > enable_hashagg option is a really nice compromise. We don't really
> > have any other executor node type that has multiple GUCs controlling
> > its behaviour, so I believe it would be nice to keep it that way.
...
> It would seem merge join has almost the same complexities as the new
> hash join code, since it can spill to disk doing sorts for merge joins,
> and adjusting work_mem is the only way to control that spill to disk. I
> don't remember anyone complaining about spills to disk during merge
> join, so I am unclear why we would need a such control for hash join.
It loooks like merge join was new in 8.3. I don't think that's a good analogy,
since the old behavior was still available with enable_mergejoin=off.
I think a better analogy would be if we now changed sort nodes beneath merge
join to use at most 0.5*work_mem, with no way of going back to using
1.0*work_mem.
--
Justin
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 17:08 Bruce Momjian <bruce@momjian.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 91+ messages in thread
From: Bruce Momjian @ 2020-06-24 17:08 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Wed, Jun 24, 2020 at 07:38:43AM -0500, Justin Pryzby wrote:
> On Wed, Jun 24, 2020 at 05:06:28AM -0400, Bruce Momjian wrote:
> > It would seem merge join has almost the same complexities as the new
> > hash join code, since it can spill to disk doing sorts for merge joins,
> > and adjusting work_mem is the only way to control that spill to disk. I
> > don't remember anyone complaining about spills to disk during merge
> > join, so I am unclear why we would need a such control for hash join.
>
> It loooks like merge join was new in 8.3. I don't think that's a good analogy,
> since the old behavior was still available with enable_mergejoin=off.
Uh, we don't gurantee backward compatibility in the optimizer. You can
turn off hashagg if you want. That doesn't get you to PG 13 behavior,
but we don't gurantee that.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 17:12 Bruce Momjian <bruce@momjian.us>
parent: David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-24 17:12 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@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>
On Thu, Jun 25, 2020 at 12:24:29AM +1200, David Rowley wrote:
> On Wed, 24 Jun 2020 at 21:06, Bruce Momjian <bruce@momjian.us> wrote:
> > I
> > don't remember anyone complaining about spills to disk during merge
> > join, so I am unclear why we would need a such control for hash join.
>
> Hash aggregate, you mean? The reason is that upgrading to PG13 can
Yes, sorry.
> cause a performance regression for an underestimated ndistinct on the
> GROUP BY clause and cause hash aggregate to spill to disk where it
> previously did everything in RAM. Sure, that behaviour was never
> what we wanted to happen, Jeff has fixed that now, but the fact
> remains that this does happen in the real world quite often and people
> often get away with it, likey because work_mem is generally set to
> some very conservative value. Of course, there's also a bunch of
> people that have been bitten by OOM due to this too. The "neverspill"
> wouldn't be for those people. Certainly, it's possible that we just
> tell these people to increase work_mem for this query, that way they
> can set it to something reasonable and still get spilling if it's
> really needed to save them from OOM, but the problem there is that
> it's not very easy to go and set work_mem for a certain query.
Well, my point is that merge join works that way, and no one has needed
a knob to avoid mergejoin if it is going to spill to disk. If they are
adjusting work_mem to prevent spill of merge join, they can do the same
for hash agg. We just need to document this in the release notes.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 17:29 Tom Lane <tgl@sss.pgh.pa.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 2 replies; 91+ messages in thread
From: Tom Lane @ 2020-06-24 17:29 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Bruce Momjian <bruce@momjian.us>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Wed, Jun 24, 2020 at 05:06:28AM -0400, Bruce Momjian wrote:
>> It would seem merge join has almost the same complexities as the new
>> hash join code, since it can spill to disk doing sorts for merge joins,
>> and adjusting work_mem is the only way to control that spill to disk. I
>> don't remember anyone complaining about spills to disk during merge
>> join, so I am unclear why we would need a such control for hash join.
> It loooks like merge join was new in 8.3. I don't think that's a good analogy,
> since the old behavior was still available with enable_mergejoin=off.
Uh, what? A look into our git history shows immediately that
nodeMergejoin.c has been there since the initial code import in 1996.
I tend to agree with Bruce that it's not very obvious that we need
another GUC knob here ... especially not one as ugly as this.
I'm especially against the "neverspill" option, because that makes a
single GUC that affects both the planner and executor independently.
If we feel we need something to let people have the v12 behavior
back, let's have
(1) enable_hashagg on/off --- controls planner, same as it ever was
(2) enable_hashagg_spill on/off --- controls executor by disabling spill
But I'm not really convinced that we need (2).
regards, tom lane
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 18:32 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 91+ messages in thread
From: Tomas Vondra @ 2020-06-24 18:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Bruce Momjian <bruce@momjian.us>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org
On Wed, Jun 24, 2020 at 01:29:56PM -0400, Tom Lane wrote:
>Justin Pryzby <pryzby@telsasoft.com> writes:
>> On Wed, Jun 24, 2020 at 05:06:28AM -0400, Bruce Momjian wrote:
>>> It would seem merge join has almost the same complexities as the new
>>> hash join code, since it can spill to disk doing sorts for merge joins,
>>> and adjusting work_mem is the only way to control that spill to disk. I
>>> don't remember anyone complaining about spills to disk during merge
>>> join, so I am unclear why we would need a such control for hash join.
>
>> It loooks like merge join was new in 8.3. I don't think that's a good analogy,
>> since the old behavior was still available with enable_mergejoin=off.
>
>Uh, what? A look into our git history shows immediately that
>nodeMergejoin.c has been there since the initial code import in 1996.
>
>I tend to agree with Bruce that it's not very obvious that we need
>another GUC knob here ... especially not one as ugly as this.
>I'm especially against the "neverspill" option, because that makes a
>single GUC that affects both the planner and executor independently.
>
>If we feel we need something to let people have the v12 behavior
>back, let's have
>(1) enable_hashagg on/off --- controls planner, same as it ever was
>(2) enable_hashagg_spill on/off --- controls executor by disabling spill
>
What if a user specifies
enable_hashagg = on
enable_hashagg_spill = off
and the estimates say the hashagg would need to spill to disk. Should
that disable the query (in which case the second GUC affects both
executor and planner) or run it (in which case we knowingly ignore
work_mem, which seems wrong).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 18:40 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 1 reply; 91+ messages in thread
From: Tom Lane @ 2020-06-24 18:40 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Bruce Momjian <bruce@momjian.us>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On Wed, Jun 24, 2020 at 01:29:56PM -0400, Tom Lane wrote:
>> If we feel we need something to let people have the v12 behavior
>> back, let's have
>> (1) enable_hashagg on/off --- controls planner, same as it ever was
>> (2) enable_hashagg_spill on/off --- controls executor by disabling spill
> What if a user specifies
> enable_hashagg = on
> enable_hashagg_spill = off
It would probably be reasonable for the planner to behave as it did
pre-v13, that is not choose hashagg if it estimates that work_mem
would be exceeded. (So, okay, that means enable_hashagg_spill
affects both planner and executor ... but ISTM it's just one
behavior not two.)
regards, tom lane
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 19:14 Andres Freund <andres@anarazel.de>
parent: David Rowley <dgrowleyml@gmail.com>
2 siblings, 2 replies; 91+ messages in thread
From: Andres Freund @ 2020-06-24 19:14 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@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>
Hi,
On 2020-06-24 14:11:57 +1200, David Rowley wrote:
> 1. Statistics underestimation can cause hashagg to be selected. The
> executor will spill to disk in PG13. Users may find performance
> suffers as previously the query may have just overshot work_mem
> without causing any OOM issues. Their I/O performance might be
> terrible.
> 2. We might now choose to hash aggregate where pre PG13, we didn't
> choose that because the hash table was estimated to be bigger than
> work_mem. Hash agg might not be the best plan for the job.
> For #1. We know users are forced to run smaller work_mems than they
> might like as they need to allow for that random moment where all
> backends happen to be doing that 5-way hash join all at the same time.
> It seems reasonable that someone might want the old behaviour. They
> may well be sitting on a timebomb that's about to OOM, but it would be
> sad if someone's upgrade to PG13 was blocked on this, especially if
> it's just due to some query that runs once per month but needs to
> perform quickly.
I'm quite concerned about this one. I think this isn't just going to hit
when the planner mis-estimates ndistinct, but also when transition
values use a bit more space. We'll now start spilling in cases the
< v13 planner did everything right.
That's great for cases where we'd otherwise OOM, but for a lot of other
cases where there actually is more than sufficient RAM to overrun
work_mem by a single-digit factor, it can cause a pretty massive
increase of IO over < v13.
FWIW, my gut feeling is that we'll end up have to separate the
"execution time" spilling from using plain work mem, because it'll
trigger spilling too often. E.g. if the plan isn't expected to spill,
only spill at 10 x work_mem or something like that. Or we'll need
better management of temp file data when there's plenty memory
available.
> For #2. This seems like a very legitimate requirement to me. If a
> user is unhappy that PG13 now hashaggs where before it sorted and
> group aggregated, but they're unhappy, not because there's some issue
> with hashagg spilling, but because that causes the node above the agg
> to becomes a Hash Join rather than a Merge Join and that's bad for
> some existing reason. Our planner doing the wrong thing based on
> either; lack of, inaccurate or out-of-date statistics is not Jeff's
> fault. Having the ability to switch off a certain planner feature is
> just following along with what we do today for many other node types.
This one concerns me a bit less, fwiw. There's a lot more "pressure" in
the planner to choose hash agg or sorted agg, compared to e.g. a bunch
of aggregate states taking up a bit more space (can't estimate that at
all for ma.
> As for GUCs to try to help the group of users who, *I'm certain*, will
> have problems with PG13's plan choice. I think the overloaded
> enable_hashagg option is a really nice compromise. We don't really
> have any other executor node type that has multiple GUCs controlling
> its behaviour, so I believe it would be nice to keep it that way.
>
> How about:
>
> enable_hashagg = "on" -- enables hashagg allowing it to freely spill
> to disk as it pleases.
> enable_hashagg = "trynospill" -- Planner will only choose hash_agg if
> it thinks it won't spill (pre PG13 planner behaviour)
> enable_hashagg = "neverspill" -- executor will *never* spill to disk
> and can still OOM (NOT RECOMMENDED, but does give pre PG13 planner and
> executor behaviour)
> enable_hashagg = "off" -- planner does not consider hash agg, ever.
> Same as what PG12 did for this setting.
>
> Now, it's a bit weird to have "neverspill" as this is controlling
> what's done in the executor from a planner GUC. Likely we can just
> work around that by having a new "allowhashspill" bool field in the
> "Agg" struct that's set by the planner, say during createplan that
> controls if nodeAgg.c is allowed to spill or not. That'll also allow
> PREPAREd plans to continue to do what they had planned to do already.
>
> The thing I like about doing it this way is that:
>
> a) it does not add any new GUCs
> b) it semi hides the weird values that we really wish nobody would
> ever have to set in a GUC that people have become used it just
> allowing the values "on" and "off".
>
> The thing I don't quite like about this idea is:
> a) I wish the planner was perfect and we didn't need to do this.
> b) It's a bit weird to overload a GUC that has a very booleanish name
> to not be bool.
>
> However, I also think it's pretty lightweight to support this. I
> imagine a dozen lines of docs and likely about half a dozen lines per
> GUC option in the planner.
That'd work for me, but I honestly don't particularly care about the
specific naming, as long as we provide users an escape hatch from the
increased amount of IO.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 19:19 Andres Freund <andres@anarazel.de>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 91+ messages in thread
From: Andres Freund @ 2020-06-24 19:19 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@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>
Hi,
On 2020-06-24 13:12:03 -0400, Bruce Momjian wrote:
> Well, my point is that merge join works that way, and no one has needed
> a knob to avoid mergejoin if it is going to spill to disk. If they are
> adjusting work_mem to prevent spill of merge join, they can do the same
> for hash agg. We just need to document this in the release notes.
I don't think this is comparable. For starters, the IO indirectly
triggered by mergejoin actually leads to plenty people just straight out
disabling it. For lots of workloads there's never a valid reason to use
a mergejoin (and often the planner will never choose one). Secondly, the
planner has better information about estimating the memory usage for the
to-be-sorted data than it has about the size of the transition
values. And lastly, there's a difference between a long existing cause
for bad IO behaviour and one that's suddenly kicks in after a major
version upgrade, to which there's no escape hatch (it's rarely realistic
to disable hash aggs, in contrast to merge joins).
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 19:28 Robert Haas <robertmhaas@gmail.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 3 replies; 91+ messages in thread
From: Robert Haas @ 2020-06-24 19:28 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.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>
On Wed, Jun 24, 2020 at 3:14 PM Andres Freund <andres@anarazel.de> wrote:
> FWIW, my gut feeling is that we'll end up have to separate the
> "execution time" spilling from using plain work mem, because it'll
> trigger spilling too often. E.g. if the plan isn't expected to spill,
> only spill at 10 x work_mem or something like that. Or we'll need
> better management of temp file data when there's plenty memory
> available.
So, I don't think we can wire in a constant like 10x. That's really
unprincipled and I think it's a bad idea. What we could do, though, is
replace the existing Boolean-valued GUC with a new GUC that controls
the size at which the aggregate spills. The default could be -1,
meaning work_mem, but a user could configure a larger value if desired
(presumably, we would just treat a value smaller than work_mem as
work_mem, and document the same).
I think that's actually pretty appealing. Separating the memory we
plan to use from the memory we're willing to use before spilling seems
like a good idea in general, and I think we should probably also do it
in other places - like sorts.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 19:31 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 91+ messages in thread
From: Andres Freund @ 2020-06-24 19:31 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>; Justin Pryzby <pryzby@telsasoft.com>; Bruce Momjian <bruce@momjian.us>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org
Hi,
On 2020-06-24 14:40:50 -0400, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> > On Wed, Jun 24, 2020 at 01:29:56PM -0400, Tom Lane wrote:
> >> If we feel we need something to let people have the v12 behavior
> >> back, let's have
> >> (1) enable_hashagg on/off --- controls planner, same as it ever was
> >> (2) enable_hashagg_spill on/off --- controls executor by disabling spill
>
> > What if a user specifies
> > enable_hashagg = on
> > enable_hashagg_spill = off
>
> It would probably be reasonable for the planner to behave as it did
> pre-v13, that is not choose hashagg if it estimates that work_mem
> would be exceeded. (So, okay, that means enable_hashagg_spill
> affects both planner and executor ... but ISTM it's just one
> behavior not two.)
There's two different reasons for spilling in the executor right now:
1) The planner estimated that we'd need to spill, and that turns out to
be true. There seems no reason to not spill in that case (as long as
it's enabled/chosen in the planner).
2) The planner didn't think we'd need to spill, but we end up using more
than work_mem memory.
nodeAgg.c already treats those separately:
void
hash_agg_set_limits(double hashentrysize, uint64 input_groups, int used_bits,
Size *mem_limit, uint64 *ngroups_limit,
int *num_partitions)
{
int npartitions;
Size partition_mem;
/* if not expected to spill, use all of work_mem */
if (input_groups * hashentrysize < work_mem * 1024L)
{
if (num_partitions != NULL)
*num_partitions = 0;
*mem_limit = work_mem * 1024L;
*ngroups_limit = *mem_limit / hashentrysize;
return;
}
We can't sensibly disable spilling when chosen at plan time, because
that'd lead to *more* OOMS than in v12.
ISTM that we should have one option that controls whether 1) is done,
and one that controls whether 2) is done. Even if the option for 2 is
off, we still should spill when the option for 1) chooses a spilling
plan. I don't think it makes sense for one of those options to
influence the other implicitly.
So maybe enable_hashagg_spilling_plan for 1) and
hashagg_spill_on_exhaust for 2).
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 19:36 Andres Freund <andres@anarazel.de>
parent: Robert Haas <robertmhaas@gmail.com>
2 siblings, 1 reply; 91+ messages in thread
From: Andres Freund @ 2020-06-24 19:36 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.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>
Hi,
On 2020-06-24 15:28:47 -0400, Robert Haas wrote:
> On Wed, Jun 24, 2020 at 3:14 PM Andres Freund <andres@anarazel.de> wrote:
> > FWIW, my gut feeling is that we'll end up have to separate the
> > "execution time" spilling from using plain work mem, because it'll
> > trigger spilling too often. E.g. if the plan isn't expected to spill,
> > only spill at 10 x work_mem or something like that. Or we'll need
> > better management of temp file data when there's plenty memory
> > available.
>
> So, I don't think we can wire in a constant like 10x. That's really
> unprincipled and I think it's a bad idea. What we could do, though, is
> replace the existing Boolean-valued GUC with a new GUC that controls
> the size at which the aggregate spills. The default could be -1,
> meaning work_mem, but a user could configure a larger value if desired
> (presumably, we would just treat a value smaller than work_mem as
> work_mem, and document the same).
To be clear, I wasn't actually thinking of hard-coding 10x, but having a
config option that specifies a factor of work_mem. A factor seems better
because it'll work reasonably for different values of work_mem, whereas
a concrete size wouldn't.
> I think that's actually pretty appealing. Separating the memory we
> plan to use from the memory we're willing to use before spilling seems
> like a good idea in general, and I think we should probably also do it
> in other places - like sorts.
Indeed. And then perhaps we could eventually add some reporting /
monitoring infrastructure for the cases where plan time and execution
time memory estimate/usage widely differs.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 21:02 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 91+ messages in thread
From: Tomas Vondra @ 2020-06-24 21:02 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Robert Haas <robertmhaas@gmail.com>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Wed, Jun 24, 2020 at 12:36:24PM -0700, Andres Freund wrote:
>Hi,
>
>On 2020-06-24 15:28:47 -0400, Robert Haas wrote:
>> On Wed, Jun 24, 2020 at 3:14 PM Andres Freund <andres@anarazel.de> wrote:
>> > FWIW, my gut feeling is that we'll end up have to separate the
>> > "execution time" spilling from using plain work mem, because it'll
>> > trigger spilling too often. E.g. if the plan isn't expected to spill,
>> > only spill at 10 x work_mem or something like that. Or we'll need
>> > better management of temp file data when there's plenty memory
>> > available.
>>
>> So, I don't think we can wire in a constant like 10x. That's really
>> unprincipled and I think it's a bad idea. What we could do, though, is
>> replace the existing Boolean-valued GUC with a new GUC that controls
>> the size at which the aggregate spills. The default could be -1,
>> meaning work_mem, but a user could configure a larger value if desired
>> (presumably, we would just treat a value smaller than work_mem as
>> work_mem, and document the same).
>
>To be clear, I wasn't actually thinking of hard-coding 10x, but having a
>config option that specifies a factor of work_mem. A factor seems better
>because it'll work reasonably for different values of work_mem, whereas
>a concrete size wouldn't.
>
I'm not quite convinced we need/should introduce a new memory limit.
It's true keping it equal to work_mem by default makes this less of an
issue, but it's still another moving part the users will need to learn
how to use.
But if we do introduce a new limit, I very much think it should be a
plain limit, not a factor. That just makes it even more complicated, and
we don't have any such limit yet.
>
>> I think that's actually pretty appealing. Separating the memory we
>> plan to use from the memory we're willing to use before spilling seems
>> like a good idea in general, and I think we should probably also do it
>> in other places - like sorts.
>
>Indeed. And then perhaps we could eventually add some reporting /
>monitoring infrastructure for the cases where plan time and execution
>time memory estimate/usage widely differs.
>
I wouldn't mind something like that in general - not just for hashagg,
but for various other nodes.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 23:15 Bruce Momjian <bruce@momjian.us>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 0 replies; 91+ messages in thread
From: Bruce Momjian @ 2020-06-24 23:15 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Andres Freund <andres@anarazel.de>; Robert Haas <robertmhaas@gmail.com>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Wed, Jun 24, 2020 at 11:02:10PM +0200, Tomas Vondra wrote:
> > Indeed. And then perhaps we could eventually add some reporting /
> > monitoring infrastructure for the cases where plan time and execution
> > time memory estimate/usage widely differs.
> >
>
> I wouldn't mind something like that in general - not just for hashagg,
> but for various other nodes.
Well, other than worrying about problems with pre-13 queries, how is
this different from any other spill to disk when we exceed work_mem,
like sorts for merge join.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 23:18 Bruce Momjian <bruce@momjian.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-24 23:18 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@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>
On Wed, Jun 24, 2020 at 12:19:00PM -0700, Andres Freund wrote:
> Hi,
>
> On 2020-06-24 13:12:03 -0400, Bruce Momjian wrote:
> > Well, my point is that merge join works that way, and no one has needed
> > a knob to avoid mergejoin if it is going to spill to disk. If they are
> > adjusting work_mem to prevent spill of merge join, they can do the same
> > for hash agg. We just need to document this in the release notes.
>
> I don't think this is comparable. For starters, the IO indirectly
> triggered by mergejoin actually leads to plenty people just straight out
> disabling it. For lots of workloads there's never a valid reason to use
> a mergejoin (and often the planner will never choose one). Secondly, the
> planner has better information about estimating the memory usage for the
> to-be-sorted data than it has about the size of the transition
> values. And lastly, there's a difference between a long existing cause
> for bad IO behaviour and one that's suddenly kicks in after a major
> version upgrade, to which there's no escape hatch (it's rarely realistic
> to disable hash aggs, in contrast to merge joins).
Well, this sounds like an issue of degree, rather than kind. It sure
sounds like "ignore work_mem for this join type, but not the other".
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-24 23:38 Bruce Momjian <bruce@momjian.us>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-24 23:38 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Robert Haas <robertmhaas@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>
On Wed, Jun 24, 2020 at 07:18:10PM -0400, Bruce Momjian wrote:
> On Wed, Jun 24, 2020 at 12:19:00PM -0700, Andres Freund wrote:
> > Hi,
> >
> > On 2020-06-24 13:12:03 -0400, Bruce Momjian wrote:
> > > Well, my point is that merge join works that way, and no one has needed
> > > a knob to avoid mergejoin if it is going to spill to disk. If they are
> > > adjusting work_mem to prevent spill of merge join, they can do the same
> > > for hash agg. We just need to document this in the release notes.
> >
> > I don't think this is comparable. For starters, the IO indirectly
> > triggered by mergejoin actually leads to plenty people just straight out
> > disabling it. For lots of workloads there's never a valid reason to use
> > a mergejoin (and often the planner will never choose one). Secondly, the
> > planner has better information about estimating the memory usage for the
> > to-be-sorted data than it has about the size of the transition
> > values. And lastly, there's a difference between a long existing cause
> > for bad IO behaviour and one that's suddenly kicks in after a major
> > version upgrade, to which there's no escape hatch (it's rarely realistic
> > to disable hash aggs, in contrast to merge joins).
>
> Well, this sounds like an issue of degree, rather than kind. It sure
> sounds like "ignore work_mem for this join type, but not the other".
I think my main point is that work_mem was not being honored for
hash-agg before, but now that PG 13 can do it, we are again allowing
work_mem not to apply in certain cases. I am wondering if our hard
limit for work_mem is the issue, and we should make that more flexible
for all uses.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 15:46 Robert Haas <robertmhaas@gmail.com>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 2 replies; 91+ messages in thread
From: Robert Haas @ 2020-06-25 15:46 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.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>
On Wed, Jun 24, 2020 at 7:38 PM Bruce Momjian <bruce@momjian.us> wrote:
> I think my main point is that work_mem was not being honored for
> hash-agg before, but now that PG 13 can do it, we are again allowing
> work_mem not to apply in certain cases. I am wondering if our hard
> limit for work_mem is the issue, and we should make that more flexible
> for all uses.
I mean, that's pretty much what we're talking about here, isn't it? It
seems like in your previous two replies you were opposed to separating
the plan-type limit from the execution-time limit, but that idea is
precisely a way of being more flexible (and extending it to other plan
nodes is a way of making it more flexible for more use cases).
As I think you know, if you have a system where the workload varies a
lot, you may sometimes be using 0 copies of work_mem and at other
times 1000 or more copies, so the value has to be chosen
conservatively as a percentage of system memory, else you start
swapping or the OOM killer gets involved. On the other hand, some plan
nodes get a lot less efficient when the amount of memory available
falls below some threshold, so you can't just set this to a tiny value
and forget about it. Because the first problem is so bad, most people
set the value relatively conservatively and just live with the
performance consequences. But this also means that they have memory
left over most of the time, so the idea of letting a node burst above
its work_mem allocation when something unexpected happens isn't crazy:
as long as only a few nodes do that here and there, rather than, say,
all the nodes doing it all at the same time, it's actually fine. If we
had a smarter system that could dole out more work_mem to nodes that
would really benefit from it and less to nodes where it isn't likely
to make much difference, that would be similar in spirit but even
better.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 16:14 Jeff Davis <pgsql@j-davis.com>
parent: Robert Haas <robertmhaas@gmail.com>
2 siblings, 0 replies; 91+ messages in thread
From: Jeff Davis @ 2020-06-25 16:14 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; +Cc: 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>
On Wed, 2020-06-24 at 15:28 -0400, Robert Haas wrote:
> On Wed, Jun 24, 2020 at 3:14 PM Andres Freund <andres@anarazel.de>
> wrote:
> > FWIW, my gut feeling is that we'll end up have to separate the
> > "execution time" spilling from using plain work mem, because it'll
> > trigger spilling too often. E.g. if the plan isn't expected to
> > spill,
> > only spill at 10 x work_mem or something like that. Or we'll need
> > better management of temp file data when there's plenty memory
> > available.
...
> I think that's actually pretty appealing. Separating the memory we
> plan to use from the memory we're willing to use before spilling
> seems
> like a good idea in general, and I think we should probably also do
> it
> in other places - like sorts.
I'm trying to make sense of this. Let's say there are two GUCs:
planner_work_mem=16MB and executor_work_mem=32MB.
And let's say a query comes along and generates a HashAgg path, and the
planner (correctly) thinks if you put all the groups in memory at once,
it would be 24MB. Then the planner, using planner_work_mem, would think
spilling was necessary, and generate a cost that involves spilling.
Then it's going to generate a Sort+Group path, as well. And perhaps it
estimates that sorting all of the tuples in memory would also take
24MB, so it generates a cost that involves spilling to disk.
But it has to choose one of them. We've penalized plans at risk of
spilling to disk, but what's the point? The planner needs to choose one
of them, and both are at risk of spilling to disk.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 16:24 Jeff Davis <pgsql@j-davis.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-25 16:24 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; +Cc: Robert Haas <robertmhaas@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>
On Wed, 2020-06-24 at 12:14 -0700, Andres Freund wrote:
> E.g. if the plan isn't expected to spill,
> only spill at 10 x work_mem or something like that.
Let's say you have work_mem=32MB and a query that's expected to use
16MB of memory. In reality, it uses 64MB of memory. So you are saying
this query would get to use all 64MB of memory, right?
But then you run ANALYZE. Now the query is (correctly) expected to use
64MB of memory. Are you saying this query, executed again with better
stats, would only get to use 32MB of memory, and therefore run slower?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 16:37 Andres Freund <andres@anarazel.de>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 91+ messages in thread
From: Andres Freund @ 2020-06-25 16:37 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@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>
On 2020-06-25 09:24:52 -0700, Jeff Davis wrote:
> On Wed, 2020-06-24 at 12:14 -0700, Andres Freund wrote:
> > E.g. if the plan isn't expected to spill,
> > only spill at 10 x work_mem or something like that.
>
> Let's say you have work_mem=32MB and a query that's expected to use
> 16MB of memory. In reality, it uses 64MB of memory. So you are saying
> this query would get to use all 64MB of memory, right?
>
> But then you run ANALYZE. Now the query is (correctly) expected to use
> 64MB of memory. Are you saying this query, executed again with better
> stats, would only get to use 32MB of memory, and therefore run slower?
Yes. I think that's ok, because it was taken into account from a costing
perspective int he second case.
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 16:42 Jeff Davis <pgsql@j-davis.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-25 16:42 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>; Justin Pryzby <pryzby@telsasoft.com>; Bruce Momjian <bruce@momjian.us>; David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org
On Wed, 2020-06-24 at 12:31 -0700, Andres Freund wrote:
> nodeAgg.c already treats those separately:
>
> void
> hash_agg_set_limits(double hashentrysize, uint64 input_groups, int
> used_bits,
> Size *mem_limit, uint64
> *ngroups_limit,
> int *num_partitions)
> {
> int npartitions;
> Size partition_mem;
>
> /* if not expected to spill, use all of work_mem */
> if (input_groups * hashentrysize < work_mem * 1024L)
> {
> if (num_partitions != NULL)
> *num_partitions = 0;
> *mem_limit = work_mem * 1024L;
> *ngroups_limit = *mem_limit / hashentrysize;
> return;
> }
The reason this code exists is to decide how much of work_mem to set
aside for spilling (each spill partition needs an IO buffer).
The alternative would be to fix the number of partitions before
processing a batch, which didn't seem ideal. Or, we could just ignore
the memory required for IO buffers, like HashJoin.
Granted, this is an example where an underestimate can give an
advantage, but I don't think we want to extend the concept into other
areas.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 17:15 Jeff Davis <pgsql@j-davis.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-25 17:15 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Bruce Momjian <bruce@momjian.us>; +Cc: Andres Freund <andres@anarazel.de>; 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>
On Thu, 2020-06-25 at 11:46 -0400, Robert Haas wrote:
> Because the first problem is so bad, most people
> set the value relatively conservatively and just live with the
> performance consequences. But this also means that they have memory
> left over most of the time, so the idea of letting a node burst above
> its work_mem allocation when something unexpected happens isn't
> crazy:
> as long as only a few nodes do that here and there, rather than, say,
> all the nodes doing it all at the same time, it's actually fine.
Unexpected things (meaning underestimates) are not independent. All the
queries are based on the same stats, so if you have a lot of similar
queries, they will all get the same underestimate at once, and all be
surprised when they need to spill at once, and then all decide they are
entitled to ignore work_mem at once.
> If we
> had a smarter system that could dole out more work_mem to nodes that
> would really benefit from it and less to nodes where it isn't likely
> to make much difference, that would be similar in spirit but even
> better.
That sounds more useful and probably not too hard to implement in a
crude form. Just have a shared counter in memory representing GB. If a
node is about to spill, it could try to decrement the counter by N, and
if it succeeds, it gets to exceed work_mem by N more GB.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 17:17 Bruce Momjian <bruce@momjian.us>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 2 replies; 91+ messages in thread
From: Bruce Momjian @ 2020-06-25 17:17 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.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>
On Thu, Jun 25, 2020 at 11:46:54AM -0400, Robert Haas wrote:
> On Wed, Jun 24, 2020 at 7:38 PM Bruce Momjian <bruce@momjian.us> wrote:
> > I think my main point is that work_mem was not being honored for
> > hash-agg before, but now that PG 13 can do it, we are again allowing
> > work_mem not to apply in certain cases. I am wondering if our hard
> > limit for work_mem is the issue, and we should make that more flexible
> > for all uses.
>
> I mean, that's pretty much what we're talking about here, isn't it? It
> seems like in your previous two replies you were opposed to separating
> the plan-type limit from the execution-time limit, but that idea is
> precisely a way of being more flexible (and extending it to other plan
> nodes is a way of making it more flexible for more use cases).
I think it is was Tom who was complaining about plan vs. execution time
control.
> As I think you know, if you have a system where the workload varies a
> lot, you may sometimes be using 0 copies of work_mem and at other
> times 1000 or more copies, so the value has to be chosen
> conservatively as a percentage of system memory, else you start
> swapping or the OOM killer gets involved. On the other hand, some plan
> nodes get a lot less efficient when the amount of memory available
> falls below some threshold, so you can't just set this to a tiny value
> and forget about it. Because the first problem is so bad, most people
> set the value relatively conservatively and just live with the
> performance consequences. But this also means that they have memory
> left over most of the time, so the idea of letting a node burst above
> its work_mem allocation when something unexpected happens isn't crazy:
> as long as only a few nodes do that here and there, rather than, say,
> all the nodes doing it all at the same time, it's actually fine. If we
> had a smarter system that could dole out more work_mem to nodes that
> would really benefit from it and less to nodes where it isn't likely
> to make much difference, that would be similar in spirit but even
> better.
I think the issue is that in PG 13 work_mem controls sorts and hashes
with a new hard limit for hash aggregation:
https://www.postgresql.org/docs/12/runtime-config-resource.html#RUNTIME-CONFIG-RESOURCE-MEMORY
Sort operations are used for ORDER BY, DISTINCT, and merge joins. Hash
tables are used in hash joins, hash-based aggregation, and hash-based
processing of IN subqueries.
In pre-PG 13, we "controlled" it by avoiding hash-based aggregation if
was expected it to exceed work_mem, but if we assumed it would be less
than work_mem and it was more, we exceeded work_mem allocation for that
node. In PG 13, we "limit" memory to work_mem and spill to disk if we
exceed it.
We should really have always documented that hash agg could exceed
work_mem for misestimation, and if we add a hash_agg work_mem
misestimation bypass setting we should document this setting in work_mem
as well.
But then the question is why do we allow this bypass only for hash agg?
Should work_mem have a settings for ORDER BY, merge join, hash join, and
hash agg, e.g.:
work_mem = 'order_by=10MB, hash_join=20MB, hash_agg=100MB'
Yeah, crazy syntax, but you get the idea. I understand some nodes are
more sensitive to disk spill than others, so shouldn't we be controlling
this at the work_mem level, rather than for a specific node type like
hash agg? We could allow for misestimation over allocation of hash agg
work_mem by splitting up the hash agg values:
work_mem = 'order_by=10MB, hash_join=20MB, hash_agg=100MB hash_agg_max=200MB'
but _avoiding_ hash agg if it is estimated to exceed work mem and spill
to disk is not something to logically control at the work mem level,
which leads so something like David Rowley suggested, but with different
names:
enable_hashagg = on | soft | avoid | off
where 'on' and 'off' are the current PG 13 behavior, 'soft' means to
treat work_mem as a soft limit and allow it to exceed work mem for
misestimation, and 'avoid' means to avoid hash agg if it is estimated to
exceed work mem. Both 'soft' and 'avoid' don't spill to disk.
David's original terms of "trynospill" and "neverspill" were focused on
spilling, not on its interaction with work_mem, and I found that
confusing.
Frankly, if it took me this long to get my head around this, I am
unclear how many people will understand this tuning feature enough to
actually use it.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 17:44 Jeff Davis <pgsql@j-davis.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-25 17:44 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@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>
On Thu, 2020-06-25 at 09:37 -0700, Andres Freund wrote:
> > Let's say you have work_mem=32MB and a query that's expected to use
> > 16MB of memory. In reality, it uses 64MB of memory. So you are
> > saying
> > this query would get to use all 64MB of memory, right?
> >
> > But then you run ANALYZE. Now the query is (correctly) expected to
> > use
> > 64MB of memory. Are you saying this query, executed again with
> > better
> > stats, would only get to use 32MB of memory, and therefore run
> > slower?
>
> Yes. I think that's ok, because it was taken into account from a
> costing
> perspective int he second case.
What do you mean by "taken into account"?
There are only two possible paths: HashAgg and Sort+Group, and we need
to pick one. If the planner expects one to spill, it is likely to
expect the other to spill. If one spills in the executor, then the
other is likely to spill, too. (I'm ignoring the case with a lot of
tuples and few groups because that doesn't seem relevant.)
Imagine that there was only one path available to choose. Would you
suggest the same thing, that unexpected spills can exceed work_mem but
expected spills can't?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 17:47 Robert Haas <robertmhaas@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 0 replies; 91+ messages in thread
From: Robert Haas @ 2020-06-25 17:47 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Bruce Momjian <bruce@momjian.us>; Andres Freund <andres@anarazel.de>; 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>
On Thu, Jun 25, 2020 at 1:15 PM Jeff Davis <pgsql@j-davis.com> wrote:
> Unexpected things (meaning underestimates) are not independent. All the
> queries are based on the same stats, so if you have a lot of similar
> queries, they will all get the same underestimate at once, and all be
> surprised when they need to spill at once, and then all decide they are
> entitled to ignore work_mem at once.
Yeah, that's a risk. But what is proposed is a configuration setting,
so people can adjust it depending on what they think is likely to
happen in their environment.
> That sounds more useful and probably not too hard to implement in a
> crude form. Just have a shared counter in memory representing GB. If a
> node is about to spill, it could try to decrement the counter by N, and
> if it succeeds, it gets to exceed work_mem by N more GB.
That's a neat idea, although GB seems too coarse.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 18:02 Jeff Davis <pgsql@j-davis.com>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-25 18:02 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; 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>
On Thu, 2020-06-25 at 13:17 -0400, Bruce Momjian wrote:
> Frankly, if it took me this long to get my head around this, I am
> unclear how many people will understand this tuning feature enough to
> actually use it.
The way I think about it is that v13 HashAgg is much more consistent
with the way we do everything else: the planner costs it (including any
spilling that is expected), and the executor executes it (including any
spilling that is required to obey work_mem).
In earlier versions, HashAgg was weird. If we add GUCs to get that
weird behavior back, then the GUCs will necessarily be weird; and
therefore hard to document.
I would feel more comfortable with some kind of GUC escape hatch (or
two). GROUP BY is just too common, and I don't think we can ignore the
potential for users experiencing a regression of some type (even if, in
principle, the v13 version is better).
If we have the GUCs there, then at least if someone comes to the
mailing list with a problem, we can offer them a temporary solution,
and have time to try to avoid the problem in a future release (tweaking
estimates, cost model, defaults, etc.).
One idea is to have undocumented GUCs. That way we don't have to
support them forever, and we are more likely to hear problem reports.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 18:10 Jeff Davis <pgsql@j-davis.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-25 18:10 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; +Cc: Bruce Momjian <bruce@momjian.us>; David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Wed, 2020-06-24 at 13:29 -0400, Tom Lane wrote:
> If we feel we need something to let people have the v12 behavior
> back, let's have
> (1) enable_hashagg on/off --- controls planner, same as it ever was
> (2) enable_hashagg_spill on/off --- controls executor by disabling
> spill
>
> But I'm not really convinced that we need (2).
If we're not going to have a planner GUC, one alternative is to just
penalize the disk costs of HashAgg for a release or two. It would only
affect the cost of HashAgg paths that are expected to spill, which
weren't even generated in previous releases.
In other words, multiply the disk costs by enough that the planner will
usually not choose HashAgg if expected to spill unless the average
group size is quite large (i.e. there are a lot more tuples than
groups, but still enough groups to spill).
As we learn more and optimize more, we can reduce or eliminate the
penalty in a future release. I'm not sure exactly what the penalty
would be, though.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 18:16 Andres Freund <andres@anarazel.de>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 91+ messages in thread
From: Andres Freund @ 2020-06-25 18:16 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@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>
Hi,
On 2020-06-25 10:44:42 -0700, Jeff Davis wrote:
> There are only two possible paths: HashAgg and Sort+Group, and we need
> to pick one. If the planner expects one to spill, it is likely to
> expect the other to spill. If one spills in the executor, then the
> other is likely to spill, too. (I'm ignoring the case with a lot of
> tuples and few groups because that doesn't seem relevant.)
There's also ordered index scan + Group. Which will often be vastly
better than Sort+Group, but still slower than HashAgg.
> Imagine that there was only one path available to choose. Would you
> suggest the same thing, that unexpected spills can exceed work_mem but
> expected spills can't?
I'm not saying what I propose is perfect, but I've yet to hear a better
proposal. Given that there *are* different ways to implement
aggregation, and that we use expected costs to choose, I think the
assumed costs are relevant.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 18:25 Bruce Momjian <bruce@momjian.us>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-25 18:25 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; 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>
On Thu, Jun 25, 2020 at 11:02:30AM -0700, Jeff Davis wrote:
> If we have the GUCs there, then at least if someone comes to the
> mailing list with a problem, we can offer them a temporary solution,
> and have time to try to avoid the problem in a future release (tweaking
> estimates, cost model, defaults, etc.).
>
> One idea is to have undocumented GUCs. That way we don't have to
> support them forever, and we are more likely to hear problem reports.
Uh, our track record of adding GUCs just in case is not good, and
removing them is even harder. Undocumented sounds interesting but then
how do we even document when we remove it? I don't think we want to go
there. Oracle has done that, and I don't think the user experience is
good.
Maybe we should just continue though beta, add an incompatibility item
to the PG 13 release notes, and see what feedback we get. We know
increasing work_mem gets us the exceed work_mem behavior, but that
affects other nodes too, and I can't think of a way to avoid if spill is
predicted except to disable hash agg for that query.
I am still trying to get my head around why the spill is going to be so
much work to adjust for hash agg than our other spillable nodes. What
are people doing for those cases already? Do we have an real-world
queries that are a problem in PG 13 for this?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 19:24 Bruce Momjian <bruce@momjian.us>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-25 19:24 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Thu, Jun 25, 2020 at 11:10:58AM -0700, Jeff Davis wrote:
> On Wed, 2020-06-24 at 13:29 -0400, Tom Lane wrote:
> > If we feel we need something to let people have the v12 behavior
> > back, let's have
> > (1) enable_hashagg on/off --- controls planner, same as it ever was
> > (2) enable_hashagg_spill on/off --- controls executor by disabling
> > spill
> >
> > But I'm not really convinced that we need (2).
>
> If we're not going to have a planner GUC, one alternative is to just
> penalize the disk costs of HashAgg for a release or two. It would only
> affect the cost of HashAgg paths that are expected to spill, which
> weren't even generated in previous releases.
>
> In other words, multiply the disk costs by enough that the planner will
> usually not choose HashAgg if expected to spill unless the average
> group size is quite large (i.e. there are a lot more tuples than
> groups, but still enough groups to spill).
Well, the big question is whether this costing is actually more accurate
than what we have now. What I am hearing is that spilling hash agg is
expensive, so whatever we can do to reflect the actual costs seems like
a win. If it can be done, it certainly seems better than a cost setting
few people will use.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 19:56 Bruce Momjian <bruce@momjian.us>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-25 19:56 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Thu, Jun 25, 2020 at 03:24:42PM -0400, Bruce Momjian wrote:
> On Thu, Jun 25, 2020 at 11:10:58AM -0700, Jeff Davis wrote:
> > On Wed, 2020-06-24 at 13:29 -0400, Tom Lane wrote:
> > > If we feel we need something to let people have the v12 behavior
> > > back, let's have
> > > (1) enable_hashagg on/off --- controls planner, same as it ever was
> > > (2) enable_hashagg_spill on/off --- controls executor by disabling
> > > spill
> > >
> > > But I'm not really convinced that we need (2).
> >
> > If we're not going to have a planner GUC, one alternative is to just
> > penalize the disk costs of HashAgg for a release or two. It would only
> > affect the cost of HashAgg paths that are expected to spill, which
> > weren't even generated in previous releases.
> >
> > In other words, multiply the disk costs by enough that the planner will
> > usually not choose HashAgg if expected to spill unless the average
> > group size is quite large (i.e. there are a lot more tuples than
> > groups, but still enough groups to spill).
>
> Well, the big question is whether this costing is actually more accurate
> than what we have now. What I am hearing is that spilling hash agg is
> expensive, so whatever we can do to reflect the actual costs seems like
> a win. If it can be done, it certainly seems better than a cost setting
> few people will use.
It is my understanding that spill of sorts is mostly read sequentially,
while hash reads are random. Is that right? Is that not being costed
properly?
That doesn't fix the misestimation case, but increasing work mem does
allow pre-PG 13 behavior there.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 20:36 Andres Freund <andres@anarazel.de>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 2 replies; 91+ messages in thread
From: Andres Freund @ 2020-06-25 20:36 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Jeff Davis <pgsql@j-davis.com>; 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>
Hi,
On 2020-06-25 14:25:12 -0400, Bruce Momjian wrote:
> I am still trying to get my head around why the spill is going to be so
> much work to adjust for hash agg than our other spillable nodes.
Aggregates are the classical case used to process large amounts of
data. For larger amounts of data sorted input (be it via explicit sort
or ordered index scan) isn't an attractive option. IOW hash-agg is the
common case. There's also fewer stats for halfway accurately estimating
the number of groups and the size of the transition state - a sort /
hash join doesn't have an equivalent to the variably sized transition
value.
> What are people doing for those cases already? Do we have an
> real-world queries that are a problem in PG 13 for this?
I don't know about real world, but it's pretty easy to come up with
examples.
query:
SELECT a, array_agg(b) FROM (SELECT generate_series(1, 10000)) a(a), (SELECT generate_series(1, 10000)) b(b) GROUP BY a HAVING array_length(array_agg(b), 1) = 0;
work_mem = 4MB
12 18470.012 ms
HEAD 44635.210 ms
HEAD causes ~2.8GB of file IO, 12 doesn't cause any. If you're IO
bandwidth constrained, this could be quite bad.
Obviously this is contrived, and a pretty extreme case. But if you
imagine this happening on a system where disk IO isn't super fast
(e.g. just about any cloud provider).
An even more extreme version of the above is this:
query: SELECT a, array_agg(b) FROM (SELECT generate_series(1, 50000)) a(a), (SELECT generate_series(1, 10000)) b(b) GROUP BY a HAVING array_length(array_agg(b), 1) = 0;
work_mem = 16MB
12 81598.965 ms
HEAD 210772.360 ms
temporary tablespace on magnetic disk (raid 0 of two 7.2k server
spinning disks)
12 81136.530 ms
HEAD 225182.560 ms
The disks are busy in some periods, but still keep up. If I however make
the transition state a bit bigger:
query: SELECT a, array_agg(b), count(c), max(d),max(e) FROM (SELECT generate_series(1, 10000)) a(a), (SELECT generate_series(1, 5000)::text, repeat(random()::text, 10), repeat(random()::text, 10), repeat(random()::text, 10)) b(b,c,d,e) GROUP BY a HAVING array_length(array_agg(b), 1) = 0;
12 28164.865 ms
fast ssd:
HEAD 92520.680 ms
magnetic:
HEAD 183968.538 ms
(no reads, there's plenty enough memory. Just writes because the age /
amount thresholds for dirty data are reached)
In the magnetic case we're IO bottlenecked nearly the whole time.
Just to be clear: I think this is a completely over-the-top example. But
I do think it shows the problem to some degree at least.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 21:28 Jeff Davis <pgsql@j-davis.com>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 91+ messages in thread
From: Jeff Davis @ 2020-06-25 21:28 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@postgresql.org
On Thu, 2020-06-25 at 15:56 -0400, Bruce Momjian wrote:
> It is my understanding that spill of sorts is mostly read
> sequentially,
> while hash reads are random. Is that right? Is that not being
> costed
> properly?
I don't think there's a major problem with the cost model, but it could
probably use some tweaking.
Hash writes are random. The hash reads should be mostly sequential (for
large partitions it will be 128-block extents, or 1MB). The cost model
assumes 50% sequential and 50% random.
Sorts are written sequentially and read randomly, but there's
prefetching to keep the reads from being too random. The cost model
assumes 75% sequential and 25% random.
Overall, the IO pattern is better for Sort, but not dramatically so.
Tomas Vondra did some nice analysis here:
https://www.postgresql.org/message-id/20200525021045.dilgcsmgiu4l5jpa@development
That resulted in getting the prealloc and projection patches in.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 22:44 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 91+ messages in thread
From: Alvaro Herrera @ 2020-06-25 22:44 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Bruce Momjian <bruce@momjian.us>; Jeff Davis <pgsql@j-davis.com>; 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>
On 2020-Jun-25, Andres Freund wrote:
> > What are people doing for those cases already? Do we have an
> > real-world queries that are a problem in PG 13 for this?
>
> I don't know about real world, but it's pretty easy to come up with
> examples.
>
> query:
> SELECT a, array_agg(b) FROM (SELECT generate_series(1, 10000)) a(a), (SELECT generate_series(1, 10000)) b(b) GROUP BY a HAVING array_length(array_agg(b), 1) = 0;
>
> work_mem = 4MB
>
> 12 18470.012 ms
> HEAD 44635.210 ms
>
> HEAD causes ~2.8GB of file IO, 12 doesn't cause any. If you're IO
> bandwidth constrained, this could be quite bad.
... however, you can pretty much get the previous performance back by
increasing work_mem. I just tried your example here, and I get 32
seconds of runtime for work_mem 4MB, and 13.5 seconds for work_mem 1GB
(this one spills about 800 MB); if I increase that again to 1.7GB I get
no spilling and 9 seconds of runtime. (For comparison, 12 takes 15.7
seconds regardless of work_mem).
My point here is that maybe we don't need to offer a GUC to explicitly
turn spilling off; it seems sufficient to let users change work_mem so
that spilling will naturally not occur. Why do we need more?
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 22:48 Andres Freund <andres@anarazel.de>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 91+ messages in thread
From: Andres Freund @ 2020-06-25 22:48 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Bruce Momjian <bruce@momjian.us>; Jeff Davis <pgsql@j-davis.com>; 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>
Hi,
On June 25, 2020 3:44:22 PM PDT, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>On 2020-Jun-25, Andres Freund wrote:
>
>> > What are people doing for those cases already? Do we have an
>> > real-world queries that are a problem in PG 13 for this?
>>
>> I don't know about real world, but it's pretty easy to come up with
>> examples.
>>
>> query:
>> SELECT a, array_agg(b) FROM (SELECT generate_series(1, 10000)) a(a),
>(SELECT generate_series(1, 10000)) b(b) GROUP BY a HAVING
>array_length(array_agg(b), 1) = 0;
>>
>> work_mem = 4MB
>>
>> 12 18470.012 ms
>> HEAD 44635.210 ms
>>
>> HEAD causes ~2.8GB of file IO, 12 doesn't cause any. If you're IO
>> bandwidth constrained, this could be quite bad.
>
>... however, you can pretty much get the previous performance back by
>increasing work_mem. I just tried your example here, and I get 32
>seconds of runtime for work_mem 4MB, and 13.5 seconds for work_mem 1GB
>(this one spills about 800 MB); if I increase that again to 1.7GB I get
>no spilling and 9 seconds of runtime. (For comparison, 12 takes 15.7
>seconds regardless of work_mem).
>
>My point here is that maybe we don't need to offer a GUC to explicitly
>turn spilling off; it seems sufficient to let users change work_mem so
>that spilling will naturally not occur. Why do we need more?
That's not really a useful escape hatch, because I'll often lead to other nodes using more memory.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 22:58 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 91+ messages in thread
From: Alvaro Herrera @ 2020-06-25 22:58 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Bruce Momjian <bruce@momjian.us>; Jeff Davis <pgsql@j-davis.com>; 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>
On 2020-Jun-25, Andres Freund wrote:
> >My point here is that maybe we don't need to offer a GUC to explicitly
> >turn spilling off; it seems sufficient to let users change work_mem so
> >that spilling will naturally not occur. Why do we need more?
>
> That's not really a useful escape hatch, because I'll often lead to
> other nodes using more memory.
Ah -- other nodes in the same query -- you're right, that's not good.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 23:11 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 0 replies; 91+ messages in thread
From: Tomas Vondra @ 2020-06-25 23:11 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Bruce Momjian <bruce@momjian.us>; Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org
On Thu, Jun 25, 2020 at 02:28:02PM -0700, Jeff Davis wrote:
>On Thu, 2020-06-25 at 15:56 -0400, Bruce Momjian wrote:
>> It is my understanding that spill of sorts is mostly read
>> sequentially,
>> while hash reads are random. Is that right? Is that not being
>> costed
>> properly?
>
>I don't think there's a major problem with the cost model, but it could
>probably use some tweaking.
>
>Hash writes are random. The hash reads should be mostly sequential (for
>large partitions it will be 128-block extents, or 1MB). The cost model
>assumes 50% sequential and 50% random.
>
The important bit here is that while the logical writes are random,
those are effectively combined in page cache and the physical writes are
pretty sequential. So I think the cost model is fairly reasonable.
Note: Judging by iosnoop stats shared in the thread linked by Jeff.
>Sorts are written sequentially and read randomly, but there's
>prefetching to keep the reads from being too random. The cost model
>assumes 75% sequential and 25% random.
>
>Overall, the IO pattern is better for Sort, but not dramatically so.
>Tomas Vondra did some nice analysis here:
>
>
>https://www.postgresql.org/message-id/20200525021045.dilgcsmgiu4l5jpa@development
>
>That resulted in getting the prealloc and projection patches in.
>
>Regards,
> Jeff Davis
>
>
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 23:18 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 91+ messages in thread
From: Tomas Vondra @ 2020-06-25 23:18 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Jeff Davis <pgsql@j-davis.com>; David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Thu, Jun 25, 2020 at 11:16:23AM -0700, Andres Freund wrote:
>Hi,
>
>On 2020-06-25 10:44:42 -0700, Jeff Davis wrote:
>> There are only two possible paths: HashAgg and Sort+Group, and we need
>> to pick one. If the planner expects one to spill, it is likely to
>> expect the other to spill. If one spills in the executor, then the
>> other is likely to spill, too. (I'm ignoring the case with a lot of
>> tuples and few groups because that doesn't seem relevant.)
>
>There's also ordered index scan + Group. Which will often be vastly
>better than Sort+Group, but still slower than HashAgg.
>
>
>> Imagine that there was only one path available to choose. Would you
>> suggest the same thing, that unexpected spills can exceed work_mem but
>> expected spills can't?
>
>I'm not saying what I propose is perfect, but I've yet to hear a better
>proposal. Given that there *are* different ways to implement
>aggregation, and that we use expected costs to choose, I think the
>assumed costs are relevant.
>
I share Jeff's opinion that this is quite counter-intuitive and we'll
have a hard time explaining it to users.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 23:23 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 0 replies; 91+ messages in thread
From: Tomas Vondra @ 2020-06-25 23:23 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Andres Freund <andres@anarazel.de>; Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; Bruce Momjian <bruce@momjian.us>; David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org
On Thu, Jun 25, 2020 at 09:42:33AM -0700, Jeff Davis wrote:
>On Wed, 2020-06-24 at 12:31 -0700, Andres Freund wrote:
>> nodeAgg.c already treats those separately:
>>
>> void
>> hash_agg_set_limits(double hashentrysize, uint64 input_groups, int
>> used_bits,
>> Size *mem_limit, uint64
>> *ngroups_limit,
>> int *num_partitions)
>> {
>> int npartitions;
>> Size partition_mem;
>>
>> /* if not expected to spill, use all of work_mem */
>> if (input_groups * hashentrysize < work_mem * 1024L)
>> {
>> if (num_partitions != NULL)
>> *num_partitions = 0;
>> *mem_limit = work_mem * 1024L;
>> *ngroups_limit = *mem_limit / hashentrysize;
>> return;
>> }
>
>The reason this code exists is to decide how much of work_mem to set
>aside for spilling (each spill partition needs an IO buffer).
>
>The alternative would be to fix the number of partitions before
>processing a batch, which didn't seem ideal. Or, we could just ignore
>the memory required for IO buffers, like HashJoin.
>
I think the conclusion from the recent HashJoin discussions is that not
accounting for BufFiles is an issue, and we want to fix it. So repeating
that for HashAgg would be a mistake, IMHO.
>Granted, this is an example where an underestimate can give an
>advantage, but I don't think we want to extend the concept into other
>areas.
>
I agree.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-25 23:53 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 1 reply; 91+ messages in thread
From: Tomas Vondra @ 2020-06-25 23:53 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Thu, Jun 25, 2020 at 01:17:56PM -0400, Bruce Momjian wrote:
>On Thu, Jun 25, 2020 at 11:46:54AM -0400, Robert Haas wrote:
>> On Wed, Jun 24, 2020 at 7:38 PM Bruce Momjian <bruce@momjian.us> wrote:
>> > I think my main point is that work_mem was not being honored for
>> > hash-agg before, but now that PG 13 can do it, we are again allowing
>> > work_mem not to apply in certain cases. I am wondering if our hard
>> > limit for work_mem is the issue, and we should make that more flexible
>> > for all uses.
>>
>> I mean, that's pretty much what we're talking about here, isn't it? It
>> seems like in your previous two replies you were opposed to separating
>> the plan-type limit from the execution-time limit, but that idea is
>> precisely a way of being more flexible (and extending it to other plan
>> nodes is a way of making it more flexible for more use cases).
>
>I think it is was Tom who was complaining about plan vs. execution time
>control.
>
>> As I think you know, if you have a system where the workload varies a
>> lot, you may sometimes be using 0 copies of work_mem and at other
>> times 1000 or more copies, so the value has to be chosen
>> conservatively as a percentage of system memory, else you start
>> swapping or the OOM killer gets involved. On the other hand, some plan
>> nodes get a lot less efficient when the amount of memory available
>> falls below some threshold, so you can't just set this to a tiny value
>> and forget about it. Because the first problem is so bad, most people
>> set the value relatively conservatively and just live with the
>> performance consequences. But this also means that they have memory
>> left over most of the time, so the idea of letting a node burst above
>> its work_mem allocation when something unexpected happens isn't crazy:
>> as long as only a few nodes do that here and there, rather than, say,
>> all the nodes doing it all at the same time, it's actually fine. If we
>> had a smarter system that could dole out more work_mem to nodes that
>> would really benefit from it and less to nodes where it isn't likely
>> to make much difference, that would be similar in spirit but even
>> better.
>
>I think the issue is that in PG 13 work_mem controls sorts and hashes
>with a new hard limit for hash aggregation:
>
> https://www.postgresql.org/docs/12/runtime-config-resource.html#RUNTIME-CONFIG-RESOURCE-MEMORY
>
> Sort operations are used for ORDER BY, DISTINCT, and merge joins. Hash
> tables are used in hash joins, hash-based aggregation, and hash-based
> processing of IN subqueries.
>
>In pre-PG 13, we "controlled" it by avoiding hash-based aggregation if
>was expected it to exceed work_mem, but if we assumed it would be less
>than work_mem and it was more, we exceeded work_mem allocation for that
>node. In PG 13, we "limit" memory to work_mem and spill to disk if we
>exceed it.
>
>We should really have always documented that hash agg could exceed
>work_mem for misestimation, and if we add a hash_agg work_mem
>misestimation bypass setting we should document this setting in work_mem
>as well.
>
I don't think that would change anything, really. For the users the
consequences would be still exactly the same, and they wouldn't even be
in position to check if they are affected.
So just documenting that hashagg does not respect work_mem at runtime
would be nice, but it would not make any difference for v13, just like
documenting a bug is not really the same thing as fixing it.
>But then the question is why do we allow this bypass only for hash agg?
>Should work_mem have a settings for ORDER BY, merge join, hash join, and
>hash agg, e.g.:
>
> work_mem = 'order_by=10MB, hash_join=20MB, hash_agg=100MB'
>
>Yeah, crazy syntax, but you get the idea. I understand some nodes are
>more sensitive to disk spill than others, so shouldn't we be controlling
>this at the work_mem level, rather than for a specific node type like
>hash agg? We could allow for misestimation over allocation of hash agg
>work_mem by splitting up the hash agg values:
>
> work_mem = 'order_by=10MB, hash_join=20MB, hash_agg=100MB hash_agg_max=200MB'
>
>but _avoiding_ hash agg if it is estimated to exceed work mem and spill
>to disk is not something to logically control at the work mem level,
>which leads so something like David Rowley suggested, but with different
>names:
>
> enable_hashagg = on | soft | avoid | off
>
>where 'on' and 'off' are the current PG 13 behavior, 'soft' means to
>treat work_mem as a soft limit and allow it to exceed work mem for
>misestimation, and 'avoid' means to avoid hash agg if it is estimated to
>exceed work mem. Both 'soft' and 'avoid' don't spill to disk.
>
>David's original terms of "trynospill" and "neverspill" were focused on
>spilling, not on its interaction with work_mem, and I found that
>confusing.
>
>Frankly, if it took me this long to get my head around this, I am
>unclear how many people will understand this tuning feature enough to
>actually use it.
>
Yeah. I agree with Andres we this may be a real issue, and that adding
some sort of "escape hatch" for v13 would be good. But I'm not convinced
adding a whole lot of new memory limits for every node that might spill
is the way to go. What exactly would be our tuning advice to users? Of
course, we could keep it set to work_mem by default, but we all know
engineers - we can't resist tuning a know when we get one.
I'm not saying it's not beneficial to use different limits for different
nodes. Some nodes are less sensitive to the size (e.g. sorting often
gets faster with smaller work_mem). But I think we should instead have a
per-session limit, and the planner should "distribute" the memory to
different nodes. It's a hard problem, of course.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-26 04:02 Bruce Momjian <bruce@momjian.us>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-26 04:02 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Fri, Jun 26, 2020 at 01:53:57AM +0200, Tomas Vondra wrote:
> I'm not saying it's not beneficial to use different limits for different
> nodes. Some nodes are less sensitive to the size (e.g. sorting often
> gets faster with smaller work_mem). But I think we should instead have a
> per-session limit, and the planner should "distribute" the memory to
> different nodes. It's a hard problem, of course.
Yeah, I am actually confused why we haven't developed a global memory
allocation strategy and continue to use per-session work_mem.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-26 14:44 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 91+ messages in thread
From: Tomas Vondra @ 2020-06-26 14:44 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Fri, Jun 26, 2020 at 12:02:10AM -0400, Bruce Momjian wrote:
>On Fri, Jun 26, 2020 at 01:53:57AM +0200, Tomas Vondra wrote:
>> I'm not saying it's not beneficial to use different limits for different
>> nodes. Some nodes are less sensitive to the size (e.g. sorting often
>> gets faster with smaller work_mem). But I think we should instead have a
>> per-session limit, and the planner should "distribute" the memory to
>> different nodes. It's a hard problem, of course.
>
>Yeah, I am actually confused why we haven't developed a global memory
>allocation strategy and continue to use per-session work_mem.
>
I think it's pretty hard problem, actually. One of the reasons is that
the costing of a node depends on the amount of memory available to the
node, but as we're building the plan bottom-up, we have no information
about the nodes above us. So we don't know if there are operations that
will need memory, how sensitive they are, etc.
And so far the per-node limit served us pretty well, I think. So I'm not
very confused we don't have the per-session limit yet, TBH.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-26 16:37 Bruce Momjian <bruce@momjian.us>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-26 16:37 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Fri, Jun 26, 2020 at 04:44:14PM +0200, Tomas Vondra wrote:
> On Fri, Jun 26, 2020 at 12:02:10AM -0400, Bruce Momjian wrote:
> > On Fri, Jun 26, 2020 at 01:53:57AM +0200, Tomas Vondra wrote:
> > > I'm not saying it's not beneficial to use different limits for different
> > > nodes. Some nodes are less sensitive to the size (e.g. sorting often
> > > gets faster with smaller work_mem). But I think we should instead have a
> > > per-session limit, and the planner should "distribute" the memory to
> > > different nodes. It's a hard problem, of course.
> >
> > Yeah, I am actually confused why we haven't developed a global memory
> > allocation strategy and continue to use per-session work_mem.
> >
>
> I think it's pretty hard problem, actually. One of the reasons is that
Yes, it is a hard problem, because it is balancing memory for shared
buffers, work_mem, and kernel buffers:
https://momjian.us/main/blogs/pgblog/2018.html#December_7_2018
I think the big problem is that the work_mem value is not one value but
a floating value that is different per query and session, and concurrent
session activity.
> the costing of a node depends on the amount of memory available to the
> node, but as we're building the plan bottom-up, we have no information
> about the nodes above us. So we don't know if there are operations that
> will need memory, how sensitive they are, etc.
>
> And so far the per-node limit served us pretty well, I think. So I'm not
> very confused we don't have the per-session limit yet, TBH.
I was thinking more of being able to allocate a single value to be
shared by all active sesions.
Also, doesn't this blog entry also show that spiling to disk for ORDER
BY is similarly slow compared to hash aggs?
https://momjian.us/main/blogs/pgblog/2012.html#February_2_2012
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-26 17:45 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 91+ messages in thread
From: Tomas Vondra @ 2020-06-26 17:45 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Fri, Jun 26, 2020 at 12:37:26PM -0400, Bruce Momjian wrote:
>On Fri, Jun 26, 2020 at 04:44:14PM +0200, Tomas Vondra wrote:
>> On Fri, Jun 26, 2020 at 12:02:10AM -0400, Bruce Momjian wrote:
>> > On Fri, Jun 26, 2020 at 01:53:57AM +0200, Tomas Vondra wrote:
>> > > I'm not saying it's not beneficial to use different limits for different
>> > > nodes. Some nodes are less sensitive to the size (e.g. sorting often
>> > > gets faster with smaller work_mem). But I think we should instead have a
>> > > per-session limit, and the planner should "distribute" the memory to
>> > > different nodes. It's a hard problem, of course.
>> >
>> > Yeah, I am actually confused why we haven't developed a global memory
>> > allocation strategy and continue to use per-session work_mem.
>> >
>>
>> I think it's pretty hard problem, actually. One of the reasons is that
>
>Yes, it is a hard problem, because it is balancing memory for shared
>buffers, work_mem, and kernel buffers:
>
> https://momjian.us/main/blogs/pgblog/2018.html#December_7_2018
>
>I think the big problem is that the work_mem value is not one value but
>a floating value that is different per query and session, and concurrent
>session activity.
>
>> the costing of a node depends on the amount of memory available to the
>> node, but as we're building the plan bottom-up, we have no information
>> about the nodes above us. So we don't know if there are operations that
>> will need memory, how sensitive they are, etc.
>>
>> And so far the per-node limit served us pretty well, I think. So I'm not
>> very confused we don't have the per-session limit yet, TBH.
>
>I was thinking more of being able to allocate a single value to be
>shared by all active sesions.
>
Not sure I understand. What "single value" do you mean?
Wasn't the idea was to replace work_mem with something like query_mem?
That'd be nice, but I think it's inherently circular - we don't know how
to distribute this to different nodes until we know which nodes will
need a buffer, but the buffer size is important for costing (so we need
it when constructing the paths).
Plus then there's the question whether all nodes should get the same
fraction, or less sensitive nodes should get smaller chunks, etc.
Ultimately this would be based on costing too, I think, but it makes it
soe much complex ...
>Also, doesn't this blog entry also show that spiling to disk for ORDER
>BY is similarly slow compared to hash aggs?
>
> https://momjian.us/main/blogs/pgblog/2012.html#February_2_2012
>
The post does not mention hashagg at all, so I'm not sure how could it
show that? But I think you're right the spilling itself is not that far
away, in most cases (thanks to the recent fixes made by Jeff).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-26 20:53 Peter Geoghegan <pg@bowt.ie>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 91+ messages in thread
From: Peter Geoghegan @ 2020-06-26 20:53 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Bruce Momjian <bruce@momjian.us>; Jeff Davis <pgsql@j-davis.com>; 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>
On Thu, Jun 25, 2020 at 1:36 PM Andres Freund <andres@anarazel.de> wrote:
> 12 28164.865 ms
>
> fast ssd:
> HEAD 92520.680 ms
>
> magnetic:
> HEAD 183968.538 ms
>
> (no reads, there's plenty enough memory. Just writes because the age /
> amount thresholds for dirty data are reached)
>
> In the magnetic case we're IO bottlenecked nearly the whole time.
I agree with almost everything you've said on this thread, but at the
same time I question the emphasis on I/O here. You've shown that
spinning rust is about twice as slow as a fast SSD here. Fair enough,
but to me the real story is that spilling is clearly a lot slower in
general, regardless of how fast the storage subsystem happens to be (I
wonder how fast it is with a ramdisk). To me, it makes more sense to
think of the problem here as the fact that v13 will *not* do
aggregation using the fast strategy (i.e. in-memory) -- as opposed to
the problem being that v13 does the aggregation using the slow
strategy (which is assumed to be slow because it involves I/O instead
of memory buffers).
I get approximately the same query runtimes with your "make the
transition state a bit bigger" test case. With "set enable_hashagg =
off", I get a group aggregate + sort. It spills to disk, even with
'work_mem = '15GB'" -- leaving 4 runs to merge at the end. That takes
63702.992 ms on v13. But if I reduce the amount of work_mem radically,
to only 16MB (a x960 decrease!), then the run time only increases by
~30% -- it's only 83123.926 ms. So we're talking about a ~200%
increase (for hash aggregate) versus a ~30% increase (for groupagg +
sort) on fast SSDs.
Changing the cost of I/O in the context of hashaggregate seems like it
misses the point. Jeff recently said "Overall, the IO pattern is
better for Sort, but not dramatically so". Whatever the IO pattern may
be, I think that it's pretty clear that the performance
characteristics of hash aggregation with limited memory are very
different to groupaggregate + sort, at least when only a fraction of
the optimal amount of memory we'd like is available. It's true that
hash aggregate was weird among plan nodes in v12, and is now in some
sense less weird among plan nodes. And yet we have a new problem now
-- so where does that leave that whole "weirdness" framing? ISTM that
the work_mem framework was and is the main problem. We seem to have
lost a crutch that ameliorated the problem before now, even though
that amelioration was kind of an accident. Or a thing that user apps
evolved to rely on.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-26 23:00 Bruce Momjian <bruce@momjian.us>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 2 replies; 91+ messages in thread
From: Bruce Momjian @ 2020-06-26 23:00 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Fri, Jun 26, 2020 at 07:45:13PM +0200, Tomas Vondra wrote:
> On Fri, Jun 26, 2020 at 12:37:26PM -0400, Bruce Momjian wrote:
> > I was thinking more of being able to allocate a single value to be
> > shared by all active sesions.
>
> Not sure I understand. What "single value" do you mean?
I was thinking of a full-cluster work_mem maximum allocation that could
be given to various backends that request it.
Imagine we set the cluster-wide total of work_mem to 1GB. If a session
asks for 100MB, if there are no other active sessions, it can grant the
entire 100MB. If there are other sessions running, and 500MB has
already been allocated, maybe it is only given an active per-node
work_mem of 50MB. As the amount of unallocated cluster-wide work_mem
gets smaller, requests are granted smaller actual allocations.
What we do now makes little sense, because we might have lots of free
memory, but we force nodes to spill to disk when they exceed a fixed
work_mem. I realize this is very imprecise, because you don't know what
future work_mem requests are coming, or how long until existing
allocations are freed, but it seems it would have to be better than what
we do now.
> Wasn't the idea was to replace work_mem with something like query_mem?
> That'd be nice, but I think it's inherently circular - we don't know how
> to distribute this to different nodes until we know which nodes will
> need a buffer, but the buffer size is important for costing (so we need
> it when constructing the paths).
>
> Plus then there's the question whether all nodes should get the same
> fraction, or less sensitive nodes should get smaller chunks, etc.
> Ultimately this would be based on costing too, I think, but it makes it
> soe much complex ...
Since work_mem affect the optimizer choices, I can imagine it getting
complex since nodes would have to ask the global work_mem allocator how
much memory it _might_ get, but then ask for final work_mem during
execution, and they might differ. Still, our spill costs are so high
for so many node types, that reducing spills seems like it would be a
win, even if it sometimes causes poorer plans.
> > Also, doesn't this blog entry also show that spiling to disk for ORDER
> > BY is similarly slow compared to hash aggs?
> >
> > https://momjian.us/main/blogs/pgblog/2012.html#February_2_2012
>
> The post does not mention hashagg at all, so I'm not sure how could it
> show that? But I think you're right the spilling itself is not that far
> away, in most cases (thanks to the recent fixes made by Jeff).
Yeah, I was just measuring ORDER BY spill, but it seems to be a similar
overhead to hashagg spill, which is being singled out in this discussion
as particularly expensive, and I am questioning that.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-26 23:41 Peter Geoghegan <pg@bowt.ie>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 0 replies; 91+ messages in thread
From: Peter Geoghegan @ 2020-06-26 23:41 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Fri, Jun 26, 2020 at 4:00 PM Bruce Momjian <bruce@momjian.us> wrote:
> Imagine we set the cluster-wide total of work_mem to 1GB. If a session
> asks for 100MB, if there are no other active sessions, it can grant the
> entire 100MB. If there are other sessions running, and 500MB has
> already been allocated, maybe it is only given an active per-node
> work_mem of 50MB. As the amount of unallocated cluster-wide work_mem
> gets smaller, requests are granted smaller actual allocations.
I think that that's the right approach long term. But right now the
DBA has no way to give hash-based nodes more memory, even though it's
clear that that's where it's truly needed in most cases, across almost
workloads. I think that that's the really glaring problem.
This is just the intrinsic nature of hash-based aggregation and hash
join vs sort-based aggregation and merge join (roughly speaking). It's
much more valuable to be able to do hash-based aggregation in one
pass, especially in cases where hashing already did particularly well
in Postgres v12.
> What we do now makes little sense, because we might have lots of free
> memory, but we force nodes to spill to disk when they exceed a fixed
> work_mem. I realize this is very imprecise, because you don't know what
> future work_mem requests are coming, or how long until existing
> allocations are freed, but it seems it would have to be better than what
> we do now.
Postgres 13 made hash aggregate respect work_mem. Perhaps it would
have made more sense to teach work_mem to respect hash aggregate,
though.
Hash aggregate cannot consume an unbounded amount of memory in v13,
since the old behavior was clearly unreasonable. Which is great. But
it may be even more unreasonable to force users to conservatively set
the limit on the size of the hash table in an artificial, generic way.
> Since work_mem affect the optimizer choices, I can imagine it getting
> complex since nodes would have to ask the global work_mem allocator how
> much memory it _might_ get, but then ask for final work_mem during
> execution, and they might differ. Still, our spill costs are so high
> for so many node types, that reducing spills seems like it would be a
> win, even if it sometimes causes poorer plans.
I don't think it's really about the spill costs, at least in one
important sense. If performing a hash aggregate in memory uses twice
as much memory as spilling (with either sorting or hashing), but the
operation completes in one third the time, you have actually saved
memory in the aggregate (no pun intended). Also, the query is 3x
faster, which is a nice bonus! I don't think that this kind of
scenario is rare.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-26 23:56 Bruce Momjian <bruce@momjian.us>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 0 replies; 91+ messages in thread
From: Bruce Momjian @ 2020-06-26 23:56 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Andres Freund <andres@anarazel.de>; Jeff Davis <pgsql@j-davis.com>; 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>
On Fri, Jun 26, 2020 at 01:53:05PM -0700, Peter Geoghegan wrote:
> On Thu, Jun 25, 2020 at 1:36 PM Andres Freund <andres@anarazel.de> wrote:
> > 12 28164.865 ms
> >
> > fast ssd:
> > HEAD 92520.680 ms
> >
> > magnetic:
> > HEAD 183968.538 ms
> >
> > (no reads, there's plenty enough memory. Just writes because the age /
> > amount thresholds for dirty data are reached)
> >
> > In the magnetic case we're IO bottlenecked nearly the whole time.
>
> I agree with almost everything you've said on this thread, but at the
> same time I question the emphasis on I/O here. You've shown that
> spinning rust is about twice as slow as a fast SSD here. Fair enough,
> but to me the real story is that spilling is clearly a lot slower in
> general, regardless of how fast the storage subsystem happens to be (I
> wonder how fast it is with a ramdisk). To me, it makes more sense to
This blog entry shows ORDER BY using ram disk, SSD, and magnetic:
https://momjian.us/main/blogs/pgblog/2012.html#February_2_2012
It is from 2012, but I can re-run the test if you want.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-26 23:58 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 2 replies; 91+ messages in thread
From: Tomas Vondra @ 2020-06-26 23:58 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Fri, Jun 26, 2020 at 07:00:20PM -0400, Bruce Momjian wrote:
>On Fri, Jun 26, 2020 at 07:45:13PM +0200, Tomas Vondra wrote:
>> On Fri, Jun 26, 2020 at 12:37:26PM -0400, Bruce Momjian wrote:
>> > I was thinking more of being able to allocate a single value to be
>> > shared by all active sesions.
>>
>> Not sure I understand. What "single value" do you mean?
>
>I was thinking of a full-cluster work_mem maximum allocation that could
>be given to various backends that request it.
>
>Imagine we set the cluster-wide total of work_mem to 1GB. If a session
>asks for 100MB, if there are no other active sessions, it can grant the
>entire 100MB. If there are other sessions running, and 500MB has
>already been allocated, maybe it is only given an active per-node
>work_mem of 50MB. As the amount of unallocated cluster-wide work_mem
>gets smaller, requests are granted smaller actual allocations.
>
>What we do now makes little sense, because we might have lots of free
>memory, but we force nodes to spill to disk when they exceed a fixed
>work_mem. I realize this is very imprecise, because you don't know what
>future work_mem requests are coming, or how long until existing
>allocations are freed, but it seems it would have to be better than what
>we do now.
>
>> Wasn't the idea was to replace work_mem with something like query_mem?
>> That'd be nice, but I think it's inherently circular - we don't know how
>> to distribute this to different nodes until we know which nodes will
>> need a buffer, but the buffer size is important for costing (so we need
>> it when constructing the paths).
>>
>> Plus then there's the question whether all nodes should get the same
>> fraction, or less sensitive nodes should get smaller chunks, etc.
>> Ultimately this would be based on costing too, I think, but it makes it
>> soe much complex ...
>
>Since work_mem affect the optimizer choices, I can imagine it getting
>complex since nodes would have to ask the global work_mem allocator how
>much memory it _might_ get, but then ask for final work_mem during
>execution, and they might differ. Still, our spill costs are so high
>for so many node types, that reducing spills seems like it would be a
>win, even if it sometimes causes poorer plans.
>
I may not understand what you mean by "poorer plans" here, but I find it
hard to accept that reducing spills is generally worth poorer plans.
I agree larger work_mem for hashagg (and thus less spilling) may mean
lower work_mem for so some other nodes that are less sensitive to this.
But I think this needs to be formulated as a cost-based decision,
although I don't know how to do that for the reasons I explained before
(bottom-up plan construction vs. distributing the memory budget).
FWIW some databases already do something like this - SQL Server has
something called "memory grant" which I think mostly does what you
described here.
>> > Also, doesn't this blog entry also show that spiling to disk for ORDER
>> > BY is similarly slow compared to hash aggs?
>> >
>> > https://momjian.us/main/blogs/pgblog/2012.html#February_2_2012
>>
>> The post does not mention hashagg at all, so I'm not sure how could it
>> show that? But I think you're right the spilling itself is not that far
>> away, in most cases (thanks to the recent fixes made by Jeff).
>
>Yeah, I was just measuring ORDER BY spill, but it seems to be a similar
>overhead to hashagg spill, which is being singled out in this discussion
>as particularly expensive, and I am questioning that.
>
The difference between sort and hashagg spills is that for sorts there
is no behavior change. Plans that did (not) spill in v12 will behave the
same way on v13, modulo some random perturbation. For hashagg that's not
the case - some queries that did not spill before will spill now.
So even if the hashagg spills are roughly equal to sort spills, both are
significantly more expensive than not spilling.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-27 00:05 Bruce Momjian <bruce@momjian.us>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
1 sibling, 0 replies; 91+ messages in thread
From: Bruce Momjian @ 2020-06-27 00:05 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Sat, Jun 27, 2020 at 01:58:50AM +0200, Tomas Vondra wrote:
> > Since work_mem affect the optimizer choices, I can imagine it getting
> > complex since nodes would have to ask the global work_mem allocator how
> > much memory it _might_ get, but then ask for final work_mem during
> > execution, and they might differ. Still, our spill costs are so high
> > for so many node types, that reducing spills seems like it would be a
> > win, even if it sometimes causes poorer plans.
> >
>
> I may not understand what you mean by "poorer plans" here, but I find it
> hard to accept that reducing spills is generally worth poorer plans.
We might cost a plan based on a work_mem that the global allocator
things it will give us when we are in the executor, but that might
change when we are in the executor. We could code is to an optimizer
request is always honored in the executor, but prepared plans would be a
problem, or perhaps already are if you prepare a plan and change
work_mem before EXECUTE.
> I agree larger work_mem for hashagg (and thus less spilling) may mean
> lower work_mem for so some other nodes that are less sensitive to this.
> But I think this needs to be formulated as a cost-based decision,
> although I don't know how to do that for the reasons I explained before
> (bottom-up plan construction vs. distributing the memory budget).
>
> FWIW some databases already do something like this - SQL Server has
> something called "memory grant" which I think mostly does what you
> described here.
Yep, something like that.
> > > > Also, doesn't this blog entry also show that spiling to disk for ORDER
> > > > BY is similarly slow compared to hash aggs?
> > > >
> > > > https://momjian.us/main/blogs/pgblog/2012.html#February_2_2012
> > >
> > > The post does not mention hashagg at all, so I'm not sure how could it
> > > show that? But I think you're right the spilling itself is not that far
> > > away, in most cases (thanks to the recent fixes made by Jeff).
> >
> > Yeah, I was just measuring ORDER BY spill, but it seems to be a similar
> > overhead to hashagg spill, which is being singled out in this discussion
> > as particularly expensive, and I am questioning that.
> >
>
> The difference between sort and hashagg spills is that for sorts there
> is no behavior change. Plans that did (not) spill in v12 will behave the
> same way on v13, modulo some random perturbation. For hashagg that's not
> the case - some queries that did not spill before will spill now.
Well, my point is that we already had ORDER BY problems, and if hash agg
now has them too in PG 13, I am fine with that. We don't guarantee no
problems in major versions. If we want to add a general knob that says,
"Hey allow this node to exceed work_mem by X%," I don't see the point
--- just increase work_mem, or have different work_mem settings for
different node types, as I outlined previously.
> So even if the hashagg spills are roughly equal to sort spills, both are
> significantly more expensive than not spilling.
Yes, but that means we need a more general fix and worrying about hash
agg is not addressing the core issue.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-27 00:24 Peter Geoghegan <pg@bowt.ie>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
1 sibling, 1 reply; 91+ messages in thread
From: Peter Geoghegan @ 2020-06-27 00:24 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Bruce Momjian <bruce@momjian.us>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Fri, Jun 26, 2020 at 4:59 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> I agree larger work_mem for hashagg (and thus less spilling) may mean
> lower work_mem for so some other nodes that are less sensitive to this.
> But I think this needs to be formulated as a cost-based decision,
> although I don't know how to do that for the reasons I explained before
> (bottom-up plan construction vs. distributing the memory budget).
Why do you think that it needs to be formulated as a cost-based
decision? That's probably true of a scheme that allocates memory very
intelligently, but what about an approach that's slightly better than
work_mem?
What problems do you foresee (if any) with adding a hash_mem GUC that
gets used for both planning and execution for hash aggregate and hash
join nodes, in about the same way as work_mem is now?
> FWIW some databases already do something like this - SQL Server has
> something called "memory grant" which I think mostly does what you
> described here.
Same is true of Oracle. But Oracle also has simple work_mem-like
settings for sorting and hashing. People don't really use them
anymore, but apparently it was once common for the DBA to explicitly
give over more memory to hashing -- much like the hash_mem setting I
asked about. IIRC the same is true of DB2.
> The difference between sort and hashagg spills is that for sorts there
> is no behavior change. Plans that did (not) spill in v12 will behave the
> same way on v13, modulo some random perturbation. For hashagg that's not
> the case - some queries that did not spill before will spill now.
>
> So even if the hashagg spills are roughly equal to sort spills, both are
> significantly more expensive than not spilling.
Just to make sure we're on the same page: both are significantly more
expensive than a hash aggregate not spilling *specifically*. OTOH, a
group aggregate may not be much slower when it spills compared to an
in-memory sort group aggregate. It may even be noticeably faster, due
to caching effects, as you mentioned at one point upthread.
This is the property that makes hash aggregate special, and justifies
giving it more memory than other nodes on a system-wide basis (the
same thing applies to hash join). This could even work as a multiplier
of work_mem.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-27 10:00 Amit Kapila <amit.kapila16@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
2 siblings, 1 reply; 91+ messages in thread
From: Amit Kapila @ 2020-06-27 10:00 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.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>
On Thu, Jun 25, 2020 at 12:59 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> So, I don't think we can wire in a constant like 10x. That's really
> unprincipled and I think it's a bad idea. What we could do, though, is
> replace the existing Boolean-valued GUC with a new GUC that controls
> the size at which the aggregate spills. The default could be -1,
> meaning work_mem, but a user could configure a larger value if desired
> (presumably, we would just treat a value smaller than work_mem as
> work_mem, and document the same).
>
> I think that's actually pretty appealing. Separating the memory we
> plan to use from the memory we're willing to use before spilling seems
> like a good idea in general, and I think we should probably also do it
> in other places - like sorts.
>
+1. I also think GUC on these lines could help not only the problem
being discussed here but in other cases as well. However, I think the
real question is do we want to design/implement it for PG13? It seems
to me at this stage we don't have a clear understanding of what
percentage of real-world cases will get impacted due to the new
behavior of hash aggregates. We want to provide some mechanism as a
safety net to avoid problems that users might face which is not a bad
idea but what if we wait and see the real impact of this? Is it too
bad to provide a GUC later in back-branch if we see users face such
problems quite often? I think the advantage of delaying it is that we
might see some real problems (like where hash aggregate is not a good
choice) which can be fixed via the costing model.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-27 10:41 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 91+ messages in thread
From: Tomas Vondra @ 2020-06-27 10:41 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Bruce Momjian <bruce@momjian.us>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Fri, Jun 26, 2020 at 05:24:36PM -0700, Peter Geoghegan wrote:
>On Fri, Jun 26, 2020 at 4:59 PM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>> I agree larger work_mem for hashagg (and thus less spilling) may mean
>> lower work_mem for so some other nodes that are less sensitive to this.
>> But I think this needs to be formulated as a cost-based decision,
>> although I don't know how to do that for the reasons I explained before
>> (bottom-up plan construction vs. distributing the memory budget).
>
>Why do you think that it needs to be formulated as a cost-based
>decision? That's probably true of a scheme that allocates memory very
>intelligently, but what about an approach that's slightly better than
>work_mem?
>
Well, there are multiple ideas discussed in this (sub)thread, one of
them being a per-query memory limit. That requires decisions how much
memory should different nodes get, which I think would need to be
cost-based.
>What problems do you foresee (if any) with adding a hash_mem GUC that
>gets used for both planning and execution for hash aggregate and hash
>join nodes, in about the same way as work_mem is now?
>
Of course, a simpler scheme like this would not require that. And maybe
introducing hash_mem is a good idea - I'm not particularly opposed to
that, actually. But I think we should not introduce separate memory
limits for each node type, which was also mentioned earlier.
The problem of course is that hash_mem does not really solve the issue
discussed at the beginning of this thread, i.e. regressions due to
underestimates and unexpected spilling at execution time.
The thread is getting a rather confusing mix of proposals how to fix
that for v13 and proposals how to improve our configuration of memory
limits :-(
>> FWIW some databases already do something like this - SQL Server has
>> something called "memory grant" which I think mostly does what you
>> described here.
>
>Same is true of Oracle. But Oracle also has simple work_mem-like
>settings for sorting and hashing. People don't really use them anymore,
>but apparently it was once common for the DBA to explicitly give over
>more memory to hashing -- much like the hash_mem setting I asked about.
>IIRC the same is true of DB2.
>
Interesting. What is not entirely clear to me how do these databases
decide how much should each node get during planning. With the separate
work_mem-like settings it's fairly obvious, but how do they do that with
the global limit (either per-instance or per-query)?
>> The difference between sort and hashagg spills is that for sorts
>> there is no behavior change. Plans that did (not) spill in v12 will
>> behave the same way on v13, modulo some random perturbation. For
>> hashagg that's not the case - some queries that did not spill before
>> will spill now.
>>
>> So even if the hashagg spills are roughly equal to sort spills, both
>> are significantly more expensive than not spilling.
>
>Just to make sure we're on the same page: both are significantly more
>expensive than a hash aggregate not spilling *specifically*. OTOH, a
>group aggregate may not be much slower when it spills compared to an
>in-memory sort group aggregate. It may even be noticeably faster, due
>to caching effects, as you mentioned at one point upthread.
>
>This is the property that makes hash aggregate special, and justifies
>giving it more memory than other nodes on a system-wide basis (the same
>thing applies to hash join). This could even work as a multiplier of
>work_mem.
>
Yes, I agree.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 00:40 Peter Geoghegan <pg@bowt.ie>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 91+ messages in thread
From: Peter Geoghegan @ 2020-06-29 00:40 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.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>
On Sat, Jun 27, 2020 at 3:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> I think the advantage of delaying it is that we
> might see some real problems (like where hash aggregate is not a good
> choice) which can be fixed via the costing model.
I think any problem that might come up with the costing is best
thought of as a distinct problem. This thread is mostly about the
problem of users getting fewer in-memory hash aggregates compared to a
previous release running the same application (though there has been
some discussion of the other problem, too [1], but it's thought to be
less serious).
The problem is that affected users were theoretically never entitled
to the performance they came to rely on, and yet there is good reason
to think that hash aggregate really should be entitled to more memory.
They won't care that they were theoretically never entitled to that
performance, though -- they *liked* the fact that hash agg could
cheat. And they'll dislike the fact that this cannot be corrected by
tuning work_mem, since that affects all node types that consume
work_mem, not just hash aggregate -- that could cause OOMs for them.
There are two or three similar ideas under discussion that might fix
the problem. They all seem to involve admitting that hash aggregate's
"cheating" might actually have been a good thing all along (even
though giving hash aggregate much much more memory than other nodes is
terrible), and giving hash aggregate license to "cheat openly". Note
that the problem isn't exactly a problem with the hash aggregate
spilling patch. You could think of the problem as a pre-existing issue
-- a failure to give more memory to hash aggregate, which really
should be entitled to more memory. Jeff's patch just made the issue
more obvious.
[1] https://postgr.es/m/20200624191433.5gnqgrxfmucexldm@alap3.anarazel.de
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 01:39 Peter Geoghegan <pg@bowt.ie>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 1 reply; 91+ messages in thread
From: Peter Geoghegan @ 2020-06-29 01:39 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Bruce Momjian <bruce@momjian.us>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Sat, Jun 27, 2020 at 3:41 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Well, there are multiple ideas discussed in this (sub)thread, one of
> them being a per-query memory limit. That requires decisions how much
> memory should different nodes get, which I think would need to be
> cost-based.
A design like that probably makes sense. But it's way out of scope for
Postgres 13, and not something that should be discussed further on
this thread IMV.
> Of course, a simpler scheme like this would not require that. And maybe
> introducing hash_mem is a good idea - I'm not particularly opposed to
> that, actually. But I think we should not introduce separate memory
> limits for each node type, which was also mentioned earlier.
I had imagined that hash_mem would apply to hash join and hash
aggregate only. A GUC that either represents a multiple of work_mem,
or an absolute work_mem-style KB value.
> The problem of course is that hash_mem does not really solve the issue
> discussed at the beginning of this thread, i.e. regressions due to
> underestimates and unexpected spilling at execution time.
Like Andres, I suspect that that's a smaller problem in practice. A
hash aggregate that spills often has performance characteristics
somewhat like a group aggregate + sort, anyway. I'm worried about
cases where an *in-memory* hash aggregate is naturally far far faster
than other strategies, and yet we can't use it -- despite the fact
that Postgres 12 could "safely" do so. (It probably doesn't matter
whether the slow plan that you get in Postgres 13 is a hash aggregate
that spills, or something else -- this is not really a costing
problem.)
Besides, hash_mem *can* solve that problem to some extent. Other cases
(cases where the estimates are so bad that hash_mem won't help) seem
like less of a concern to me. To some extent, that's the price you pay
to avoid the risk of an OOM.
> The thread is getting a rather confusing mix of proposals how to fix
> that for v13 and proposals how to improve our configuration of memory
> limits :-(
As I said to Amit in my last message, I think that all of the ideas
that are worth pursuing involve giving hash aggregate nodes license to
use more memory than other nodes. One variant involves doing so only
at execution time, while the hash_mem idea involves formalizing and
documenting that hash-based nodes are special -- and taking that into
account during both planning and execution.
> Interesting. What is not entirely clear to me how do these databases
> decide how much should each node get during planning. With the separate
> work_mem-like settings it's fairly obvious, but how do they do that with
> the global limit (either per-instance or per-query)?
I don't know, but that seems like a much more advanced way of
approaching the problem. It isn't in scope here.
Perhaps I'm not considering some unintended consequence of the planner
giving hash-based nodes extra memory "for free" in the common case
where hash_mem exceeds work_mem (by 2x, say). But my guess is that
that won't be a significant problem that biases the planner in some
obviously undesirable way.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 15:06 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 91+ messages in thread
From: Tomas Vondra @ 2020-06-29 15:06 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Bruce Momjian <bruce@momjian.us>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Sun, Jun 28, 2020 at 06:39:40PM -0700, Peter Geoghegan wrote:
>On Sat, Jun 27, 2020 at 3:41 AM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>> Well, there are multiple ideas discussed in this (sub)thread, one of
>> them being a per-query memory limit. That requires decisions how much
>> memory should different nodes get, which I think would need to be
>> cost-based.
>
>A design like that probably makes sense. But it's way out of scope for
>Postgres 13, and not something that should be discussed further on
>this thread IMV.
>
100% agree
>> Of course, a simpler scheme like this would not require that. And maybe
>> introducing hash_mem is a good idea - I'm not particularly opposed to
>> that, actually. But I think we should not introduce separate memory
>> limits for each node type, which was also mentioned earlier.
>
>I had imagined that hash_mem would apply to hash join and hash
>aggregate only. A GUC that either represents a multiple of work_mem,
>or an absolute work_mem-style KB value.
>
I'm not against having a hash_mem (and I'd vote to be it a simple
work_mem-style value, not a multiple). Maybe we should have it, the
argument to allow hashagg (and perhaps hashjoin) to use more memory than
some other nodes seems convincing to me.
I'm just not sure which of the problems mentioned in this thread it
actually addresses ...
>> The problem of course is that hash_mem does not really solve the issue
>> discussed at the beginning of this thread, i.e. regressions due to
>> underestimates and unexpected spilling at execution time.
>
>Like Andres, I suspect that that's a smaller problem in practice. A
>hash aggregate that spills often has performance characteristics
>somewhat like a group aggregate + sort, anyway. I'm worried about
>cases where an *in-memory* hash aggregate is naturally far far faster
>than other strategies, and yet we can't use it -- despite the fact
>that Postgres 12 could "safely" do so. (It probably doesn't matter
>whether the slow plan that you get in Postgres 13 is a hash aggregate
>that spills, or something else -- this is not really a costing
>problem.)
>
Not sure I follow. Which cases do you mean when you say that 12 could
safely do them, but 13 won't? I see the following two cases:
a) Planner in 12 and 13 disagree about whether the hash table will fit
into work_mem.
I don't quite see why this would be the case (given the same cardinality
estimates etc.), though. That is, if 12 says "will fit" I'd expect 13 to
end up with the same conclusion. But maybe 13 has higher per-tuple
overhead or something? I know we set aside some memory for BufFiles, but
not when we expect the whole hash table to fit into memory.
b) Planner believes the hash table will fit, due to underestimate.
On 12, we'd just let the hash table overflow, which may be a win when
there's enough RAM and the estimate is not "too wrong". But it may
easily end with a sad OOM.
On 13, we'll just start spilling. True - people tend to use conservative
work_mem values exactly because of cases like this (which is somewhat
futile as the underestimate may be arbitrarily wrong) and also because
they don't know how many work_mem instances the query will use.
So yeah, I understand why people may not want to increase work_mem too
much, and maybe hash_work would be a way to get the "no spill" behavior.
>Besides, hash_mem *can* solve that problem to some extent. Other cases
>(cases where the estimates are so bad that hash_mem won't help) seem
>like less of a concern to me. To some extent, that's the price you pay
>to avoid the risk of an OOM.
>
True, avoiding the risk of OOM has it's cost.
>> The thread is getting a rather confusing mix of proposals how to fix
>> that for v13 and proposals how to improve our configuration of memory
>> limits :-(
>
>As I said to Amit in my last message, I think that all of the ideas
>that are worth pursuing involve giving hash aggregate nodes license to
>use more memory than other nodes. One variant involves doing so only
>at execution time, while the hash_mem idea involves formalizing and
>documenting that hash-based nodes are special -- and taking that into
>account during both planning and execution.
>
Understood. I mostly agree with this.
>> Interesting. What is not entirely clear to me how do these databases
>> decide how much should each node get during planning. With the separate
>> work_mem-like settings it's fairly obvious, but how do they do that with
>> the global limit (either per-instance or per-query)?
>
>I don't know, but that seems like a much more advanced way of
>approaching the problem. It isn't in scope here.
>
+1
>Perhaps I'm not considering some unintended consequence of the planner
>giving hash-based nodes extra memory "for free" in the common case
>where hash_mem exceeds work_mem (by 2x, say). But my guess is that
>that won't be a significant problem that biases the planner in some
>obviously undesirable way.
>
My concern is how much more difficult would these proposals make the
reasoning about memory usage get. Maybe not much, not sure.
I certainly agree it may be beneficial to give more memory to hashagg at
the expense of other nodes.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 15:29 Bruce Momjian <bruce@momjian.us>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-29 15:29 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.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>
On Sun, Jun 28, 2020 at 05:40:16PM -0700, Peter Geoghegan wrote:
> I think any problem that might come up with the costing is best
> thought of as a distinct problem. This thread is mostly about the
> problem of users getting fewer in-memory hash aggregates compared to a
> previous release running the same application (though there has been
> some discussion of the other problem, too [1], but it's thought to be
> less serious).
>
> The problem is that affected users were theoretically never entitled
> to the performance they came to rely on, and yet there is good reason
> to think that hash aggregate really should be entitled to more memory.
> They won't care that they were theoretically never entitled to that
> performance, though -- they *liked* the fact that hash agg could
> cheat. And they'll dislike the fact that this cannot be corrected by
> tuning work_mem, since that affects all node types that consume
> work_mem, not just hash aggregate -- that could cause OOMs for them.
>
> There are two or three similar ideas under discussion that might fix
> the problem. They all seem to involve admitting that hash aggregate's
> "cheating" might actually have been a good thing all along (even
> though giving hash aggregate much much more memory than other nodes is
> terrible), and giving hash aggregate license to "cheat openly". Note
> that the problem isn't exactly a problem with the hash aggregate
> spilling patch. You could think of the problem as a pre-existing issue
> -- a failure to give more memory to hash aggregate, which really
> should be entitled to more memory. Jeff's patch just made the issue
> more obvious.
In thinking some more about this, I came out with two ideas. First, in
pre-PG 13, we didn't choose hash_agg if we thought it would spill, but
if we misestimated and it used more work_mem, we allowed it. The effect
of this is that if we were close, but it went over, we allowed it just
for hash_agg. Is this something we want to codify for all node types,
i.e., choose a non-spill node type if we need a lot more than work_mem,
but then let work_mem be a soft limit if we do choose it, e.g., allow
50% over work_mem in the executor for misestimation before spill? My
point is, do we want to use a lower work_mem for planning and a higher
one in the executor before spilling.
My second thought is from an earlier report that spilling is very
expensive, but smaller work_mem doesn't seem to hurt much. Would we
achieve better overall performance by giving a few nodes a lot of memory
(and not spill those), and other nodes very little, rather than having
them all be the same size, and all spill?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 17:20 Peter Geoghegan <pg@bowt.ie>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 2 replies; 91+ messages in thread
From: Peter Geoghegan @ 2020-06-29 17:20 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Bruce Momjian <bruce@momjian.us>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, Jun 29, 2020 at 8:07 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Not sure I follow. Which cases do you mean when you say that 12 could
> safely do them, but 13 won't? I see the following two cases:
> a) Planner in 12 and 13 disagree about whether the hash table will fit
> into work_mem.
>
> I don't quite see why this would be the case (given the same cardinality
> estimates etc.), though. That is, if 12 says "will fit" I'd expect 13 to
> end up with the same conclusion. But maybe 13 has higher per-tuple
> overhead or something? I know we set aside some memory for BufFiles, but
> not when we expect the whole hash table to fit into memory.
I have no reason to believe that the planner is any more or any less
likely to conclude that the hash table will fit in memory in v13 as
things stand (I don't know if the BufFile issue matters).
In general, grouping estimates probably aren't very good compared to
join estimates. I imagine that in either v12 or v13 the planner is
likely to incorrectly believe that it'll all fit in memory fairly
often. v12 was much too permissive about what could happen. But v13 is
too conservative.
> b) Planner believes the hash table will fit, due to underestimate.
>
> On 12, we'd just let the hash table overflow, which may be a win when
> there's enough RAM and the estimate is not "too wrong". But it may
> easily end with a sad OOM.
It might end up with an OOM on v12 due to an underestimate -- but
probably not! The fact that a hash aggregate is faster than a group
aggregate ameliorates the higher memory usage. You might actually use
less memory this way.
> On 13, we'll just start spilling. True - people tend to use conservative
> work_mem values exactly because of cases like this (which is somewhat
> futile as the underestimate may be arbitrarily wrong) and also because
> they don't know how many work_mem instances the query will use.
>
> So yeah, I understand why people may not want to increase work_mem too
> much, and maybe hash_work would be a way to get the "no spill" behavior.
Andres wanted to increase the amount of memory that could be used at
execution time, without changing planning. You could say that hash_mem
is a more ambitious proposal than that. It's changing the behavior
across the board -- though in a way that makes sense anyway. It has
the additional benefit of making it more likely that an in-memory hash
aggregate will be used. That isn't a problem that we're obligated to
solve now, so this may seem odd. But if the more ambitious plan is
actually easier to implement and support, why not pursue it?
hash_mem seems a lot easier to explain and reason about than having
different work_mem budgets during planning and execution, which is
clearly a kludge. hash_mem makes sense generally, and more or less
solves the problems raised on this thread.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 17:31 Bruce Momjian <bruce@momjian.us>
parent: Peter Geoghegan <pg@bowt.ie>
1 sibling, 1 reply; 91+ messages in thread
From: Bruce Momjian @ 2020-06-29 17:31 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, Jun 29, 2020 at 10:20:14AM -0700, Peter Geoghegan wrote:
> I have no reason to believe that the planner is any more or any less
> likely to conclude that the hash table will fit in memory in v13 as
> things stand (I don't know if the BufFile issue matters).
>
> In general, grouping estimates probably aren't very good compared to
> join estimates. I imagine that in either v12 or v13 the planner is
> likely to incorrectly believe that it'll all fit in memory fairly
> often. v12 was much too permissive about what could happen. But v13 is
> too conservative.
FYI, we have improved planner statistics estimates for years, which must
have affected node spill behavior on many node types (except hash_agg),
and don't remember any complaints about it.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 17:36 Peter Geoghegan <pg@bowt.ie>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 0 replies; 91+ messages in thread
From: Peter Geoghegan @ 2020-06-29 17:36 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.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>
On Mon, Jun 29, 2020 at 8:29 AM Bruce Momjian <bruce@momjian.us> wrote:
> Is this something we want to codify for all node types,
> i.e., choose a non-spill node type if we need a lot more than work_mem,
> but then let work_mem be a soft limit if we do choose it, e.g., allow
> 50% over work_mem in the executor for misestimation before spill? My
> point is, do we want to use a lower work_mem for planning and a higher
> one in the executor before spilling.
Andres said something about doing that with hash aggregate, which I
can see an argument for, but I don't think that it would make sense
with most other nodes. In particular, sorts still perform almost as
well with only a fraction of the "optimal" memory.
> My second thought is from an earlier report that spilling is very
> expensive, but smaller work_mem doesn't seem to hurt much.
It's not really about the spilling itself IMV. It's the inability to
do hash aggregation in a single pass.
You can think of hashing (say for hash join or hash aggregate) as a
strategy that consists of a logical division followed by a physical
combination. Sorting (or sort merge join, or group agg), in contrast,
consists of a physical division and logical combination. As a
consequence, it can be a huge win to do everything in memory in the
case of hash aggregate. Whereas sort-based aggregation can sometimes
be slightly faster with external sorts due to CPU caching effects, and
because an on-the-fly merge in tuplesort can output the first tuple
before the tuples are fully sorted.
> Would we
> achieve better overall performance by giving a few nodes a lot of memory
> (and not spill those), and other nodes very little, rather than having
> them all be the same size, and all spill?
If the nodes that we give more memory to use it for a hash table, then yes.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 21:22 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Peter Geoghegan <pg@bowt.ie>
1 sibling, 1 reply; 91+ messages in thread
From: Tomas Vondra @ 2020-06-29 21:22 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Bruce Momjian <bruce@momjian.us>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, Jun 29, 2020 at 10:20:14AM -0700, Peter Geoghegan wrote:
>On Mon, Jun 29, 2020 at 8:07 AM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>> Not sure I follow. Which cases do you mean when you say that 12 could
>> safely do them, but 13 won't? I see the following two cases:
>
>> a) Planner in 12 and 13 disagree about whether the hash table will fit
>> into work_mem.
>>
>> I don't quite see why this would be the case (given the same cardinality
>> estimates etc.), though. That is, if 12 says "will fit" I'd expect 13 to
>> end up with the same conclusion. But maybe 13 has higher per-tuple
>> overhead or something? I know we set aside some memory for BufFiles, but
>> not when we expect the whole hash table to fit into memory.
>
>I have no reason to believe that the planner is any more or any less
>likely to conclude that the hash table will fit in memory in v13 as
>things stand (I don't know if the BufFile issue matters).
>
>In general, grouping estimates probably aren't very good compared to
>join estimates. I imagine that in either v12 or v13 the planner is
>likely to incorrectly believe that it'll all fit in memory fairly
>often. v12 was much too permissive about what could happen. But v13 is
>too conservative.
>
Can you give and example of what you mean by being too permissive or too
conservative? Do you mean the possibility of unlimited memory usage in
v12, and strict enforcement in v13?
IMO enforcing the work_mem limit (in v13) is right in principle, but I
do understand the concerns about unexpected regressions compared to v12.
>> b) Planner believes the hash table will fit, due to underestimate.
>>
>> On 12, we'd just let the hash table overflow, which may be a win when
>> there's enough RAM and the estimate is not "too wrong". But it may
>> easily end with a sad OOM.
>
>It might end up with an OOM on v12 due to an underestimate -- but
>probably not! The fact that a hash aggregate is faster than a group
>aggregate ameliorates the higher memory usage. You might actually use
>less memory this way.
>
I don't understand what you mean by "less memory" when the whole issue
is significantly exceeding work_mem?
I don't think the OOM is the only negative performance here - using a
lot of memory also forces eviction of data from page cache (although
writing a lot of temporary files may have similar effect).
>> On 13, we'll just start spilling. True - people tend to use conservative
>> work_mem values exactly because of cases like this (which is somewhat
>> futile as the underestimate may be arbitrarily wrong) and also because
>> they don't know how many work_mem instances the query will use.
>>
>> So yeah, I understand why people may not want to increase work_mem too
>> much, and maybe hash_work would be a way to get the "no spill" behavior.
>
>Andres wanted to increase the amount of memory that could be used at
>execution time, without changing planning. You could say that hash_mem
>is a more ambitious proposal than that. It's changing the behavior
>across the board -- though in a way that makes sense anyway. It has
>the additional benefit of making it more likely that an in-memory hash
>aggregate will be used. That isn't a problem that we're obligated to
>solve now, so this may seem odd. But if the more ambitious plan is
>actually easier to implement and support, why not pursue it?
>
>hash_mem seems a lot easier to explain and reason about than having
>different work_mem budgets during planning and execution, which is
>clearly a kludge. hash_mem makes sense generally, and more or less
>solves the problems raised on this thread.
>
I agree with this, and I'm mostly OK with having hash_mem. In fact, from
the proposals in this thread I like it the most - as long as it's used
both during planning and execution. It's a pretty clear solution.
It's not a perfect solution in the sense that it does not reintroduce
the v12 behavior perfectly (i.e. we'll still spill after reaching
hash_mem) but that may be good enougn.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 21:33 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 0 replies; 91+ messages in thread
From: Tomas Vondra @ 2020-06-29 21:33 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Peter Geoghegan <pg@bowt.ie>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, Jun 29, 2020 at 01:31:40PM -0400, Bruce Momjian wrote:
>On Mon, Jun 29, 2020 at 10:20:14AM -0700, Peter Geoghegan wrote:
>> I have no reason to believe that the planner is any more or any less
>> likely to conclude that the hash table will fit in memory in v13 as
>> things stand (I don't know if the BufFile issue matters).
>>
>> In general, grouping estimates probably aren't very good compared to
>> join estimates. I imagine that in either v12 or v13 the planner is
>> likely to incorrectly believe that it'll all fit in memory fairly
>> often. v12 was much too permissive about what could happen. But v13 is
>> too conservative.
>
>FYI, we have improved planner statistics estimates for years, which must
>have affected node spill behavior on many node types (except hash_agg),
>and don't remember any complaints about it.
>
I think misestimates for GROUP BY are quite common and very hard to fix.
Firstly, our ndistinct estimator may give pretty bad results depending
e.g. on how is the table correlated.
I've been running some TPC-H benchmarks, and for partsupp.ps_partkey our
estimate was 4338776, when the actual value is 15000000, i.e. ~3.5x
higher. This was with statistics target increased to 1000. I can easily
imagine even worse estimates with lower values.
This ndistinct estimator is used even for extended statistics, so that
can't quite save us. Moreover, the grouping may be on top of a join, in
which case using ndistinct coefficients may not be possible :-(
So I think this is a quite real problem ...
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 91+ messages in thread
* Re: Default setting for enable_hashagg_disk
@ 2020-06-29 21:46 Peter Geoghegan <pg@bowt.ie>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 0 replies; 91+ messages in thread
From: Peter Geoghegan @ 2020-06-29 21:46 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Bruce Momjian <bruce@momjian.us>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Rowley <dgrowleyml@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Justin Pryzby <pryzby@telsasoft.com>; Melanie Plageman <melanieplageman@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Mon, Jun 29, 2020 at 2:22 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Can you give and example of what you mean by being too permissive or too
> conservative? Do you mean the possibility of unlimited memory usage in
> v12, and strict enforcement in v13?
Yes -- that's all I meant.
> IMO enforcing the work_mem limit (in v13) is right in principle, but I
> do understand the concerns about unexpected regressions compared to v12.
Yeah. Both of these two things are true at the same time.
> I don't understand what you mean by "less memory" when the whole issue
> is significantly exceeding work_mem?
I was just reiterating what I said a few times already: Not using an
in-memory hash aggregate when the amount of memory required is high
but not prohibitively high is penny wise, pound foolish. It's easy to
imagine this actually using up more memory when an entire workload is
considered. This observation does not apply to a system that only ever
has one active connection at a time, but who cares about that?
> I don't think the OOM is the only negative performance here - using a
> lot of memory also forces eviction of data from page cache (although
> writing a lot of temporary files may have similar effect).
True.
> I agree with this, and I'm mostly OK with having hash_mem. In fact, from
> the proposals in this thread I like it the most - as long as it's used
> both during planning and execution. It's a pretty clear solution.
Great.
It's not trivial to write the patch, since there are a few tricky
cases. For example, maybe there is some subtlety in nodeAgg.c with
AGG_MIXED cases. Is there somebody else that knows that code better
than I do that wants to have a go at writing a patch?
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 91+ messages in thread
end of thread, other threads:[~2020-06-29 21:46 UTC | newest]
Thread overview: 91+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-04-07 18:20 Default setting for enable_hashagg_disk Jeff Davis <pgsql@j-davis.com>
2020-04-07 22:39 ` Justin Pryzby <pryzby@telsasoft.com>
2020-04-09 11:48 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-04-09 17:24 ` Justin Pryzby <pryzby@telsasoft.com>
2020-04-09 18:25 ` Jeff Davis <pgsql@j-davis.com>
2020-06-12 03:35 ` Greg Stark <stark@mit.edu>
2020-04-09 19:26 ` Robert Haas <robertmhaas@gmail.com>
2020-04-09 20:02 ` Jeff Davis <pgsql@j-davis.com>
2020-06-10 01:20 ` Melanie Plageman <melanieplageman@gmail.com>
2020-06-10 02:15 ` Justin Pryzby <pryzby@telsasoft.com>
2020-06-10 16:40 ` Melanie Plageman <melanieplageman@gmail.com>
2020-06-10 18:39 ` Jeff Davis <pgsql@j-davis.com>
2020-06-11 20:22 ` Jeff Davis <pgsql@j-davis.com>
2020-06-12 02:45 ` Justin Pryzby <pryzby@telsasoft.com>
2020-06-20 22:04 ` Justin Pryzby <pryzby@telsasoft.com>
2020-06-22 14:52 ` Robert Haas <robertmhaas@gmail.com>
2020-06-22 15:06 ` Justin Pryzby <pryzby@telsasoft.com>
2020-06-22 15:17 ` Robert Haas <robertmhaas@gmail.com>
2020-06-22 17:44 ` Jeff Davis <pgsql@j-davis.com>
2020-06-22 17:30 ` Jeff Davis <pgsql@j-davis.com>
2020-06-22 19:28 ` Robert Haas <robertmhaas@gmail.com>
2020-06-22 20:23 ` Jeff Davis <pgsql@j-davis.com>
2020-06-24 02:11 ` David Rowley <dgrowleyml@gmail.com>
2020-06-24 03:14 ` Justin Pryzby <pryzby@telsasoft.com>
2020-06-24 09:06 ` Bruce Momjian <bruce@momjian.us>
2020-06-24 12:24 ` David Rowley <dgrowleyml@gmail.com>
2020-06-24 17:12 ` Bruce Momjian <bruce@momjian.us>
2020-06-24 19:19 ` Andres Freund <andres@anarazel.de>
2020-06-24 23:18 ` Bruce Momjian <bruce@momjian.us>
2020-06-24 23:38 ` Bruce Momjian <bruce@momjian.us>
2020-06-25 15:46 ` Robert Haas <robertmhaas@gmail.com>
2020-06-25 17:15 ` Jeff Davis <pgsql@j-davis.com>
2020-06-25 17:47 ` Robert Haas <robertmhaas@gmail.com>
2020-06-25 17:17 ` Bruce Momjian <bruce@momjian.us>
2020-06-25 18:02 ` Jeff Davis <pgsql@j-davis.com>
2020-06-25 18:25 ` Bruce Momjian <bruce@momjian.us>
2020-06-25 20:36 ` Andres Freund <andres@anarazel.de>
2020-06-25 22:44 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-06-25 22:48 ` Andres Freund <andres@anarazel.de>
2020-06-25 22:58 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-06-26 20:53 ` Peter Geoghegan <pg@bowt.ie>
2020-06-26 23:56 ` Bruce Momjian <bruce@momjian.us>
2020-06-25 23:53 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-26 04:02 ` Bruce Momjian <bruce@momjian.us>
2020-06-26 14:44 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-26 16:37 ` Bruce Momjian <bruce@momjian.us>
2020-06-26 17:45 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-26 23:00 ` Bruce Momjian <bruce@momjian.us>
2020-06-26 23:41 ` Peter Geoghegan <pg@bowt.ie>
2020-06-26 23:58 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-27 00:05 ` Bruce Momjian <bruce@momjian.us>
2020-06-27 00:24 ` Peter Geoghegan <pg@bowt.ie>
2020-06-27 10:41 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-29 01:39 ` Peter Geoghegan <pg@bowt.ie>
2020-06-29 15:06 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-29 17:20 ` Peter Geoghegan <pg@bowt.ie>
2020-06-29 17:31 ` Bruce Momjian <bruce@momjian.us>
2020-06-29 21:33 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-29 21:22 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-29 21:46 ` Peter Geoghegan <pg@bowt.ie>
2020-06-24 12:38 ` Justin Pryzby <pryzby@telsasoft.com>
2020-06-24 17:08 ` Bruce Momjian <bruce@momjian.us>
2020-06-24 17:29 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-24 18:32 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-24 18:40 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-24 19:31 ` Andres Freund <andres@anarazel.de>
2020-06-25 16:42 ` Jeff Davis <pgsql@j-davis.com>
2020-06-25 23:23 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-25 18:10 ` Jeff Davis <pgsql@j-davis.com>
2020-06-25 19:24 ` Bruce Momjian <bruce@momjian.us>
2020-06-25 19:56 ` Bruce Momjian <bruce@momjian.us>
2020-06-25 21:28 ` Jeff Davis <pgsql@j-davis.com>
2020-06-25 23:11 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-24 19:14 ` Andres Freund <andres@anarazel.de>
2020-06-24 19:28 ` Robert Haas <robertmhaas@gmail.com>
2020-06-24 19:36 ` Andres Freund <andres@anarazel.de>
2020-06-24 21:02 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-24 23:15 ` Bruce Momjian <bruce@momjian.us>
2020-06-25 16:14 ` Jeff Davis <pgsql@j-davis.com>
2020-06-27 10:00 ` Amit Kapila <amit.kapila16@gmail.com>
2020-06-29 00:40 ` Peter Geoghegan <pg@bowt.ie>
2020-06-29 15:29 ` Bruce Momjian <bruce@momjian.us>
2020-06-29 17:36 ` Peter Geoghegan <pg@bowt.ie>
2020-06-25 16:24 ` Jeff Davis <pgsql@j-davis.com>
2020-06-25 16:37 ` Andres Freund <andres@anarazel.de>
2020-06-25 17:44 ` Jeff Davis <pgsql@j-davis.com>
2020-06-25 18:16 ` Andres Freund <andres@anarazel.de>
2020-06-25 23:18 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-06-10 17:39 ` Jeff Davis <pgsql@j-davis.com>
2020-06-11 00:48 ` Melanie Plageman <melanieplageman@gmail.com>
2020-06-10 18:52 ` Jeff Davis <pgsql@j-davis.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox