Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> On 2015/05/11 8:50, Tom Lane wrote:
>> In particular, I find the addition of "void *fdw_state" to ExecRowMark
>> to be pretty questionable. That does not seem like a good place to keep
>> random state. (I realize that WHERE CURRENT OF keeps some state in
>> ExecRowMark, but that's a crock not something to emulate.) ISTM that in
>> most scenarios, the state that LockForeignRow/FetchForeignRow would like
>> to get at is probably the FDW state associated with the ForeignScanState
>> that the tuple came from. Which this API doesn't help with particularly.
>> I wonder if we should instead add a "ScanState*" field and expect the
>> core code to set that up (ExecOpenScanRelation could do it with minor
>> API changes...).
> Sorry, I don't understand clearly what you mean, but that (the idea of
> expecting the core to set it up) sounds inconsistent with your comment
> on the earlier version of the API "BeginForeignFetch" [1].
Well, the other way that it could work would be for the FDW's
BeginForeignScan routine to search for a relevant ExecRowMark entry
(which, per that previous discussion, it'd likely need to do anyway) and
then plug a back-link to the ForeignScanState into the ExecRowMark.
But I don't see any very good reason to make every FDW that's concerned
with this do that, rather than doing it once in the core code. I'm also
thinking that having a link to the source scan node there might be useful
someday for regular tables as well as FDWs.
>> Also, as I mentioned, I'd be a whole lot happier if we had a way to test
>> this...
> Attached is a postgres_fdw patch that I used for the testing. If you
> try it, edit postgresGetForeignRowMarkType as necessary. I have to
> confess that I did the testing only in the normal conditions by the patch.
Thanks, this is helpful. However, it pretty much proves my point about
wanting the back-link --- it seems like init_foreign_fetch_state, for
example, is uselessly repeating a lot of the setup already done for
the scan node itself.
regards, tom lane