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

Alexander Korotkov <a.korotkov@postgrespro.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