Thanks for the review.
On Wed, Nov 11, 2020 at 5:55 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 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.
Good point, I've updated the patch to note that.
> 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.
Right.
--
Amit Langote
EDB: http://www.enterprisedb.com