Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> [ EvalPlanQual-v6.patch ]

I've started to study this in a little more detail, and I'm not terribly
happy with some of the API decisions in it.

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...).

I'm also a bit tempted to pass the TIDs to LockForeignRow and
FetchForeignRow as Datums not ItemPointers. We have the Datum format
available already at the call sites, so this is free as far as the core
code is concerned, and would only cost another line or so for the FDWs.
This is by no means sufficient to allow FDWs to use some other type than
"tid" for row identifiers; but it would be a down payment on that problem,
and at least would avoid nailing the rowids-are-tids assumption into yet
another global API.

Thoughts?

Also, as I mentioned, I'd be a whole lot happier if we had a way to test
this...

regards, tom lane