Thread: Back-patch change in hashed DISTINCT estimation?

Back-patch change in hashed DISTINCT estimation?

From
Tom Lane
Date:
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



Re: Back-patch change in hashed DISTINCT estimation?

From
Pavan Deolasee
Date:
On Wed, Aug 21, 2013 at 2:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.  

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.

Thanks,
Pavan

--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

Re: Back-patch change in hashed DISTINCT estimation?

From
Andres Freund
Date:
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



Re: Back-patch change in hashed DISTINCT estimation?

From
Stephen Frost
Date:
* 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

Re: Back-patch change in hashed DISTINCT estimation?

From
Kevin Grittner
Date:
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



Re: Back-patch change in hashed DISTINCT estimation?

From
Jeff Janes
Date:
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