On 01/29/2015 08:59 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas@vmware.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
