Обсуждение: subplan resets wrong hashtable
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.
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
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
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,
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
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