On 10/11/2020 13:12, Amit Langote wrote:
On Wed, Nov 4, 2020 at 11:32 AM Amit Langote <amitlangot...@gmail.com> wrote:
On Tue, Nov 3, 2020 at 9:05 PM Heikki Linnakangas <hlinn...@iki.fi> wrote:
A) We could change FDW API so that BeginDirectModify takes the same
arguments as BeginForeignModify(). That avoids the assumption that it's
a ForeignScan node, because BeginForeignModify() doesn't take
ForeignScanState as argument. That would be consistent, which is nice.
But I think we'd somehow still need to pass the ResultRelInfo to the
corresponding ForeignScan, and I'm not sure how.
Maybe ForeignScan doesn't need to contain any result relation info
then? ForeignScan.operation != CMD_SELECT is enough to tell it to
call IterateDirectModify() as today.
Hmm, I misspoke. We do still need ForeignScanState.resultRelInfo,
because the IterateDirectModify() API uses it to return the remotely
inserted/updated/deleted tuple for the RETURNING projection performed
by ExecModifyTable().
B) Look up the ResultRelInfo, and call BeginDirectModify(), on the first
call to ForeignNext().
C) Accept the Assertion. And add an elog() check in the planner for that
with a proper error message.
I'm leaning towards B), but maybe there's some better solution I didn't
think of? Perhaps changing the API would make sense in any case, it is a
bit weird as it is. Backwards-incompatible API changes are not nice, but
I don't think there are many FDWs out there that implement the
DirectModify functions. And those functions are pretty tightly coupled
with the executor and ModifyTable node details anyway, so I don't feel
like we can, or need to, guarantee that they stay unchanged across major
versions.
B is not too bad, but I tend to prefer doing A too.
On second thought, it seems A would amount to merely a cosmetic
adjustment of the API, nothing more. B seems to get the job done for
me and also doesn't unnecessarily break compatibility, so I've updated
0001 to implement B. Please give it a look.
Looks good at a quick glance. It is a small API break that
BeginDirectModify() is now called during execution, not at executor
startup, but I don't think that's going to break FDWs in practice. One
could argue, though, that if we're going to change the API, we should do
it more loudly. So changing the arguments might be a good thing.
The BeginDirectModify() and BeginForeignModify() interfaces are
inconsistent, but that's not this patch's fault. I wonder if we could
move the call to BeginForeignModify() also to ForeignNext(), though? And
BeginForeignScan() too, while we're at it.
Overall, this is probably fine as it is though. I'll review more
thorougly tomorrow.
- Heikki
