Thread
Commits
-
Correctly re-use hash tables in buildSubPlanHash().
- 9eb6b652a46d 11.8 landed
- 96d783ae55b9 12.3 landed
- 58c47ccfff20 13.0 landed
-
Reset, not recreate, execGrouping.c style hashtables.
- 356687bd825e 12.0 cited
-
subplan resets wrong hashtable
Justin Pryzby <pryzby@telsasoft.com> — 2020-02-10T03:25:47Z
I believe the 2nd hunk should reset node->hashnulls, rather than reset ->hashtable a 2nd time: @@ -505,7 +505,10 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext) if (nbuckets < 1) nbuckets = 1; - node->hashtable = BuildTupleHashTable(node->parent, + if (node->hashtable) + ResetTupleHashTable(node->hashtable); + else + node->hashtable = BuildTupleHashTableExt(node->parent, node->descRight, ncols, node->keyColIdx, ... @@ -527,7 +531,11 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext) if (nbuckets < 1) nbuckets = 1; } - node->hashnulls = BuildTupleHashTable(node->parent, + + if (node->hashnulls) + ResetTupleHashTable(node->hashtable); + else + node->hashnulls = BuildTupleHashTableExt(node->parent, node->descRight, ncols, node->keyColIdx, Added here: commit 356687bd825e5ca7230d43c1bffe7a59ad2e77bd Author: Andres Freund <andres@anarazel.de> Date: Sat Feb 9 00:35:57 2019 -0800 Reset, not recreate, execGrouping.c style hashtables. -
Re: subplan resets wrong hashtable
Andres Freund <andres@anarazel.de> — 2020-02-10T04:01:26Z
Hi, On 2020-02-09 21:25:47 -0600, Justin Pryzby wrote: > I believe the 2nd hunk should reset node->hashnulls, rather than reset > ->hashtable a 2nd time: > > @@ -505,7 +505,10 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext) > if (nbuckets < 1) > nbuckets = 1; > > - node->hashtable = BuildTupleHashTable(node->parent, > + if (node->hashtable) > + ResetTupleHashTable(node->hashtable); > + else > + node->hashtable = BuildTupleHashTableExt(node->parent, > node->descRight, > ncols, > node->keyColIdx, > ... > > @@ -527,7 +531,11 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext) > if (nbuckets < 1) > nbuckets = 1; > } > - node->hashnulls = BuildTupleHashTable(node->parent, > + > + if (node->hashnulls) > + ResetTupleHashTable(node->hashtable); > + else > + node->hashnulls = BuildTupleHashTableExt(node->parent, > node->descRight, > ncols, > node->keyColIdx, Ugh, that indeed looks wrong. Did you check whether it can actively cause wrong query results? If so, did you do theoretically, or got to a query returning wrong results? - Andres
-
Re: subplan resets wrong hashtable
Justin Pryzby <pryzby@telsasoft.com> — 2020-02-10T04:05:08Z
On Sun, Feb 09, 2020 at 08:01:26PM -0800, Andres Freund wrote: > Ugh, that indeed looks wrong. Did you check whether it can actively > cause wrong query results? If so, did you do theoretically, or got to a > query returning wrong results? No, I only noticed while reading code. I tried briefly to find a plan that looked like what I thought might be broken, but haven't found anything close. Justin
-
Re: subplan resets wrong hashtable
Justin Pryzby <pryzby@telsasoft.com> — 2020-02-10T04:53:08Z
On Sun, Feb 09, 2020 at 08:01:26PM -0800, Andres Freund wrote: > Ugh, that indeed looks wrong. Did you check whether it can actively > cause wrong query results? If so, did you do theoretically, or got to a > query returning wrong results? Actually .. I can "theoretically" prove that there's no wrong results from that patch...since in that file it has no effect, the tested variables being zeroed few lines earlier: @@ -499,51 +499,60 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext) * node->hashtable = NULL; * node->hashnulls = NULL; node->havehashrows = false; node->havenullrows = false; nbuckets = (long) Min(planstate->plan->plan_rows, (double) LONG_MAX); if (nbuckets < 1) nbuckets = 1; - node->hashtable = BuildTupleHashTable(node->parent, - node->descRight, - ncols, - node->keyColIdx, - node->tab_eq_funcoids, - node->tab_hash_funcs, - nbuckets, - 0, - node->hashtablecxt, - node->hashtempcxt, - false); *+ if (node->hashtable) + ResetTupleHashTable(node->hashtable); + else + node->hashtable = BuildTupleHashTableExt(node->parent, ... *+ if (node->hashnulls) + ResetTupleHashTable(node->hashtable); + else + node->hashnulls = BuildTupleHashTableExt(node->parent, + node->descRight, -
Re: subplan resets wrong hashtable
Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-10T22:08:31Z
Justin Pryzby <pryzby@telsasoft.com> writes: > On Sun, Feb 09, 2020 at 08:01:26PM -0800, Andres Freund wrote: >> Ugh, that indeed looks wrong. Did you check whether it can actively >> cause wrong query results? If so, did you do theoretically, or got to a >> query returning wrong results? > Actually .. I can "theoretically" prove that there's no wrong results from that > patch...since in that file it has no effect, the tested variables being zeroed > few lines earlier: Right. So the incorrect ResetTupleHashTable call is unreachable (and a look at the code coverage report confirms that). The whole thing obviously is a bit hasty and unreviewed, but it doesn't have a live bug AFAICS ... or at least, if there's a bug, it's a memory leakage issue across repeat executions, not a crash hazard. I'm not too clear on whether the context reset just above those pointer assignments will get rid of all traces of the old hash tables, but it sort of looks like it might not anymore. Anyway, not going to hold up the releases for a fix for this. We've lived with it for a year, so it can wait another quarter. regards, tom lane
-
Re: subplan resets wrong hashtable
Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-29T19:05:51Z
I wrote: > Right. So the incorrect ResetTupleHashTable call is unreachable > (and a look at the code coverage report confirms that). The whole > thing obviously is a bit hasty and unreviewed, but it doesn't have > a live bug AFAICS ... or at least, if there's a bug, it's a memory > leakage issue across repeat executions, not a crash hazard. For the archives' sake: this *is* a memory leak, and we dealt with it at 58c47ccfff20b8c125903482725c1dbfd30beade. regards, tom lane