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)
return false;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers