Thanks for the review. On Wed, Nov 11, 2020 at 5:55 PM Heikki Linnakangas <hlinn...@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
v9-0001-Set-ForeignScanState.resultRelInfo-lazily.patch
Description: Binary data
v9-0002-Initialize-result-relation-information-lazily.patch
Description: Binary data