On 10/11/2020 17:32, Heikki Linnakangas wrote:
On 10/11/2020 13:12, Amit Langote wrote:
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.
With these patches, BeginForeignModify() and BeginDirectModify() are
both called during execution, before the first
IterateForeignScan/IterateDirectModify call. The documentation for
BeginForeignModify() needs to be updated, it still claims that it's run
at executor startup, but that's not true after these patches. So that
needs to be updated.
I think that's a good thing, because it means that BeginForeignModify()
and BeginDirectModify() are called at the same stage, from the FDW's
point of view. Even though BeginDirectModify() is called from
ForeignNext(), and BeginForeignModify() from ExecModifyTable(), that
difference isn't visible to the FDW; both are after executor startup but
before the first Iterate call.
- Heikki
