Hi,
On 03/21/2016 12:30 AM, David Rowley wrote:
On 21 March 2016 at 09:47, Tomas Vondra <tomas.von...@2ndquadrant.com> wrote:
...
I'm not sure changing the meaning of enable_hashagg like this is a
good idea. It worked as a hard switch before, while with this
change that would not be the case. Or more accurately - it would
not be the case for aggregates, but it would still work the old way
for other types of plans. Not sure that's a particularly good
idea.
Hmm, I don't see how it was a hard switch before. If we were unable
to sort by the group by clause then hashagg would magically be
enabled.
Sure, but that's not what I meant by the "hard switch" (sorry for the
inaccuracy). Let's assume we can actually so the sorted aggregate. In
that case the enable_hashagg=off entirely skipped the hashagg path,
irrespectively of the cost or whatever. With the new code we will add
the path, and it may actually win on the basis of cost (e.g. if there's
enable_sort=off at the same time).
But I'm not convinced this is actually wrong, I merely pointed out the
behavior is not exactly the same and may have unintended consequences.
The reason I did this was to simplify the logic in
create_grouping_paths(). What difference do you imagine that there
actually is here?
That the hashed path may win over the sorted path, as explained above.
The only thing I can think of is; we now generate a hashagg path
where we previously didn't. This has disable_cost added to the
startup_cost so is quite unlikely to win. Perhaps there is some
differences if someone did SET enable_sort = false; SET
enable_hashagg = false; I'm not sure if we should be worried there
though. Also maybe there's going to be a difference if the plan
costings were so high that disable_cost was drowned out by the other
costings.
Ah, I see you came to the same conclusion ...
Apart from that, It would actually be nice to be consistent with
this enable_* GUCs, as to my knowledge the others all just add
disable_cost to the startup_cost of the path.
Perhaps.
What about introducing a GUC to enable parallel aggregate, while
still allowing other types of parallel nodes (I guess that would be
handy for other types of parallel nodes - it's a bit blunt tool,
but tweaking max_parallel_degree is even blumter)? e.g.
enable_parallelagg?
Haribabu this in his version of the patch and I didn't really
understand the need for it, I assumed it was for testing only. We
don't have enable_parallelseqscan, and would we plan on adding GUCs
each time we enable a node for parallelism? I really don't think so,
we already have parallel hash join and nested loop join without GUCs
to disable them. I see no reason to add them there, and I also don't
here.
I'm not so sure about that. I certainly think we'll be debugging queries
in a not so distant future, wishing for such GUCs.
I do also have a question about parallel aggregate vs. work_mem.
Nowadays we mostly say to users a query may allocate a multiple of
work_mem, up to one per node in the plan. Apparently with parallel
aggregate we'll have to say "multiplied by number of workers", because
each aggregate worker may allocate up to hashaggtablesize. Is that
reasonable? Shouldn't we restrict the total size of hash tables in
all workers somehow?
I did think about this, but thought, either;
1) that a project wide decision should be made on how to handle this,
not just for parallel aggregate, but parallel hash join too, which as
I understand it, for now it builds an identical hashtable per worker.
>
2) the limit is per node, per connection, and parallel aggregates have
multiple connections, so we might not be breaking our definition of
how to define work_mem, since we're still limited by max_connections
anyway.
I do agree this is not just a question for parallel aggregate, and that
perhaps we're not breaking the definition. But I think in practice we'll
be hitting the memory limits much more often, because parallel queries
are very synchronized (running the same node on all parallel workers at
exactly the same time).
Not sure if we have to reflect that in the planner, but it will probably
impact what work_mem values are considered safe.
create_grouping_paths also contains this comment:
/*
* Generate HashAgg Path providing the estimated hash table size is not
* too big, although if no other Paths were generated above, then we'll
* begrudgingly generate one so that we actually have a Path to work
* with.
*/
I'm not sure this is a particularly clear comment, I think the old one was
way much informative despite being a single line:
/* Consider reasons to disable hashing, but only if we can sort instead */
hmm, I find it quite clear, but perhaps that's because I wrote the
code. I'm not really sure what you're not finding clear about it to be
honest. Tom's original comment was quite generic to allow for more
reasons, but I removed one of those reasons by simplifying the logic
around enable_hashagg, so I didn't think Tom's comment suited well
anymore.
I've rewritten the comment to become:
/*
* Providing that the estimated size of the hashtable does not exceed
* work_mem, we'll generate a HashAgg Path, although if we were unable
* to sort above, then we'd better generate a Path, so that we at least
* have one.
*/
How about that?
OK
BTW create_grouping_paths probably grew to a size when splitting it into
smaller pieces would be helpful?
I'd rather not. Amit mentioned this too [1]. See 4A. Robert has marked
it as ready for committer, so I really don't want to start hacking it
up too much at this stage unless Robert requests so.
OK
An updated patch is attached. This hopefully addresses your concerns
with the comment, and also the estimate_hashagg_tablesize() NULL
checking.
[1]
http://www.postgresql.org/message-id/CAKJS1f80=f-z1CUU7=QDmn0r=_yeu7pan2dz6rqsnupfefo...@mail.gmail.com
thanks
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
