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
