Thread: Back-patch change in hashed DISTINCT estimation?
In a thread over in pgsql-performance, Tomas Vondra pointed out that
choose_hashed_distinct was sometimes making different choices than
choose_hashed_grouping, so that queries like these:
select distinct x from ... ;select x from ... group by 1;
might get different plans even though they should be equivalent.
After some debugging it turns out that I omitted hash_agg_entry_size()
from the hash table size estimate in choose_hashed_distinct --- I'm pretty
sure I momentarily thought that this function must yield zero if there are
no aggregates, but that's wrong. So we need a patch like this:
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index bcc0d45..99284cb 100644
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** choose_hashed_distinct(PlannerInfo *root
*** 2848,2854 ****
--- 2848,2858 ---- * Don't do it if it doesn't look like the hashtable will fit into * work_mem. */
+
+ /* Estimate per-hash-entry space at tuple width... */ hashentrysize = MAXALIGN(path_width) +
MAXALIGN(sizeof(MinimalTupleData));
+ /* plus the per-hash-entry overhead */
+ hashentrysize += hash_agg_entry_size(0); if (hashentrysize * dNumDistinctRows > work_mem * 1024L)
returnfalse;
When grouping narrow data, like a float or a couple of ints, this
oversight makes for more than 2X error in the hash table size estimate.
What I'm wondering is whether to back-patch this or leave well enough
alone. The risk of back-patching is that it might destabilize plan
choices that people like. (In Tomas' original example, the underestimate
of the table size leads it to choose a plan that is in fact better.)
The risk of not back-patching is that the error could lead to
out-of-memory failures because the hash aggregation uses more memory
than the planner expected. (Tomas was rather fortunate in that his
case had an overestimate of dNumDistinctRows, so it didn't end up
blowing out memory ... but usually I think we underestimate that more
than overestimate it.)
A possible compromise is to back-patch into 9.3 (where the argument about
destabilizing plan choices doesn't have much force yet), but not further.
Thoughts?
regards, tom lane
On Wed, Aug 21, 2013 at 2:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
FWIW I recently investigated an out-of-memory issue in hash aggregation. That case was because of use of a large temp table which was not manually analysed and thus lead to a bad plan selection. But out of memory errors are very confusing to the users and I have seen them unnecessarily tinkering their memory settings to circumvent that issue. So +1 to fix the bug in back branches, even though I understand there could be some casualties on the border.
--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee
What I'm wondering is whether to back-patch this or leave well enough
alone. The risk of back-patching is that it might destabilize plan
choices that people like. (In Tomas' original example, the underestimate
of the table size leads it to choose a plan that is in fact better.)
The risk of not back-patching is that the error could lead to
out-of-memory failures because the hash aggregation uses more memory
than the planner expected.
Thanks,
Pavan
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee
On 2013-08-20 17:24:18 -0400, Tom Lane wrote: > In a thread over in pgsql-performance, Tomas Vondra pointed out that > choose_hashed_distinct was sometimes making different choices than > choose_hashed_grouping, so that queries like these: > > select distinct x from ... ; > select x from ... group by 1; > > might get different plans even though they should be equivalent. > After some debugging it turns out that I omitted hash_agg_entry_size() > from the hash table size estimate in choose_hashed_distinct --- I'm pretty > sure I momentarily thought that this function must yield zero if there are > no aggregates, but that's wrong. So we need a patch like this: > What I'm wondering is whether to back-patch this or leave well enough > alone. The risk of back-patching is that it might destabilize plan > choices that people like. [...] > A possible compromise is to back-patch into 9.3 (where the argument about > destabilizing plan choices doesn't have much force yet), but not further. +1 for 9.3 only. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
* Andres Freund (andres@2ndquadrant.com) wrote:
> On 2013-08-20 17:24:18 -0400, Tom Lane wrote:
> > In a thread over in pgsql-performance, Tomas Vondra pointed out that
> > choose_hashed_distinct was sometimes making different choices than
> > choose_hashed_grouping, so that queries like these:
> >
> > select distinct x from ... ;
> > select x from ... group by 1;
> >
> > might get different plans even though they should be equivalent.
> > After some debugging it turns out that I omitted hash_agg_entry_size()
> > from the hash table size estimate in choose_hashed_distinct --- I'm pretty
> > sure I momentarily thought that this function must yield zero if there are
> > no aggregates, but that's wrong. So we need a patch like this:
>
> > What I'm wondering is whether to back-patch this or leave well enough
> > alone. The risk of back-patching is that it might destabilize plan
> > choices that people like. [...]
>
> > A possible compromise is to back-patch into 9.3 (where the argument about
> > destabilizing plan choices doesn't have much force yet), but not further.
>
> +1 for 9.3 only.
Yeah, I've been thinking about this a bit also and agree that 9.3 is
fine but not farther back.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> wrote: > Yeah, I've been thinking about this a bit also and agree that 9.3 > is fine but not farther back. +1 to 9.3 but no farther back. I would be in favor of going farther back if there were not fairly obvious workarounds for the OOM problems that lack of back-patch could cause. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Aug 21, 2013 at 4:05 AM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-08-20 17:24:18 -0400, Tom Lane wrote: >> In a thread over in pgsql-performance, Tomas Vondra pointed out that >> choose_hashed_distinct was sometimes making different choices than >> choose_hashed_grouping, so that queries like these: >> >> select distinct x from ... ; >> select x from ... group by 1; >> >> might get different plans even though they should be equivalent. >> After some debugging it turns out that I omitted hash_agg_entry_size() >> from the hash table size estimate in choose_hashed_distinct --- I'm pretty >> sure I momentarily thought that this function must yield zero if there are >> no aggregates, but that's wrong. So we need a patch like this: > >> What I'm wondering is whether to back-patch this or leave well enough >> alone. The risk of back-patching is that it might destabilize plan >> choices that people like. [...] > >> A possible compromise is to back-patch into 9.3 (where the argument about >> destabilizing plan choices doesn't have much force yet), but not further. > > +1 for 9.3 only. I agree. work_mem is hard to tune with any great precision analytically. If it is carefully tuned, it was probably done empirically, so changing the behavior in back branches seems bad. Cheers, Jeff