From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> |
Cc: | Erik Rijkers <er(at)xs4all(dot)nl>, Alexander Korotkov <akorotkov(at)postgresql(dot)org>, pgsql-committers <pgsql-committers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST |
Date: | 2019-09-19 21:13:26 |
Message-ID: | 14346.1568927606@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-committers |
Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> Both dromedary and tern, where segfault happened, are 32-bit. Bug
> seems related to USE_FLOAT8_BYVAL or something.
Yeah, I was just about to write back that there's an independent problem.
Look at the logic inside the loop in index_store_float8_orderby_distances:
scan->xs_orderbynulls[i] = distances[i].isnull;
if (scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = (Datum) 0;
if (orderByTypes[i] == FLOAT8OID)
{
#ifndef USE_FLOAT8_BYVAL
/* must free any old value to avoid memory leakage */
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
if (!scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = Float8GetDatum(distances[i].value);
}
The pfree is being done way too late, as you've already stomped on
both the isnull flag and the pointer value. I think the first
three lines quoted above need to be moved to after the #ifndef
stanza (and, hence, duplicated for the float4 case), like
#ifndef USE_FLOAT8_BYVAL
/* must free any old value to avoid memory leakage */
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
scan->xs_orderbynulls[i] = distances[i].isnull;
if (scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = (Datum) 0;
else
scan->xs_orderbyvals[i] = Float8GetDatum(distances[i].value);
Another issue here is that the short-circuit path above (for !distances)
leaks memory, in not-passbyval cases. I'd be inclined to get rid of the
short circuit and just handle the case within the main loop, so really
that'd be more like
if (distances)
{
scan->xs_orderbynulls[i] = distances[i].isnull;
if (scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = (Datum) 0;
else
scan->xs_orderbyvals[i] = Float8GetDatum(distances[i].value);
}
else
{
scan->xs_orderbynulls[i] = true;
scan->xs_orderbyvals[i] = (Datum) 0;
}
regards, tom lane
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Korotkov | 2019-09-19 22:20:58 | pgsql: Fix freeing old values in index_store_float8_orderby_distances() |
Previous Message | Alexander Korotkov | 2019-09-19 20:47:43 | Re: pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST |