Re: [HACKERS] Back-patch change in hashed DISTINCT estimation?
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
Re: [HACKERS] Back-patch change in hashed DISTINCT estimation?
* Andres Freund (and...@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 signature.asc Description: Digital signature
Re: [HACKERS] Back-patch change in hashed DISTINCT estimation?
Stephen Frost sfr...@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
Re: [HACKERS] Back-patch change in hashed DISTINCT estimation?
On Wed, Aug 21, 2013 at 4:05 AM, Andres Freund and...@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
[HACKERS] 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
Re: [HACKERS] Back-patch change in hashed DISTINCT estimation?
On Wed, Aug 21, 2013 at 2:54 AM, Tom Lane t...@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
