Re: pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

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