Re: review: FDW API

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> On 18.02.2011 22:16, Tom Lane wrote:
>> Question after first look: what is the motivation for passing
>> estate->es_param_list_info to BeginScan? AFAICS, even if there is a
>> reason for that function to need that, it isn't receiving any info that
>> would be sufficient to let it know what's in there.

> The idea is that when the query is planned, the FDW can choose to push
> down a qual that contains a parameter marker, like "WHERE remotecol =
> $1". At execution time, it needs the value of the parameter to send it
> to the remote server. The PostgreSQL FDW does that, although I didn't
> test it so it might well be broken.

s/might well be/is/ --- there's no guarantee that parameters are valid
at executor setup time. The place that needs to be grabbing the
parameter value for that purpose is BeginScan.

>> What seems more
>> likely to be useful is to pass in the EState pointer, as for example
>> being able to look at es_query_cxt seems like a good idea.

> By "look at", you mean allocate stuff in it?

Right. I suppose you're going to comment that CurrentMemoryContext is
probably the same thing, but in general it's not going to pay to make
this API run with blinders on. My feeling is it'd be best to pass down
all the information the executor node has got --- probably we should
just pass the ForeignScanState node itself, and leave a void * in that
for FDW-private data, and be done with it. Otherwise we're going to be
adding missed stuff back to the API every time somebody notices that
their FDW can't do X because they don't have access to the necessary
information. That definitional instability will trump any ABI stability
that might be gained from not relying on executor node types. (And it's
not like changing ScanState in a released version is an entirely safe
thing to do even today --- there are lots of loadable modules that know
about that struct.)

regards, tom lane