Re: BUG #12694: crash if the number of result rows is lower than gin_fuzzy_search_limit

On 01/29/2015 08:59 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>> For master and 9.4, I'm thinking of applying the attached. It makes it
>> clear that startScan() is not used to re-start a scan with existing scan
>> keys, but is always called on a newly initialized scan keys.
>
> Looks reasonable to me, but should ginFreeScanKeys() null out the pointers
> after freeing them, to be sure we find any incorrect accesses? It might
> not be worth the trouble; but if you have any doubts at all about the
> order of operations this seems like a good safety feature.

Nah, I'm not worried about that. ginFreeScanKeys() frees the whole
'keys' array, so we'd have bigger problems if there was a
reference-after-free.

> Also, in the department of nitpicks, I'd do this:
>
> {
> IndexScanDesc scan = (IndexScanDesc) PG_GETARG_POINTER(0);
> + GinScanOpaque so = (GinScanOpaque) scan->opaque;
> TIDBitmap *tbm = (TIDBitmap *) PG_GETARG_POINTER(1);
> int64 ntids;
>
> more like this:
>
> {
> IndexScanDesc scan = (IndexScanDesc) PG_GETARG_POINTER(0);
> TIDBitmap *tbm = (TIDBitmap *) PG_GETARG_POINTER(1);
> + GinScanOpaque so = (GinScanOpaque) scan->opaque;
> int64 ntids;
>
> I think of the PG_GETARG calls as being an ugly stepchild of a proper
> function header declaration, and as such, they should come first unless
> there is an unavoidable reason not to.

Ok, committed with that fix.

- Heikki