Re: ModifyTable overheads in generic plans

On 10/11/2020 13:12, Amit Langote wrote:
> On Wed, Nov 4, 2020 at 11:32 AM Amit Langote <amitlangote09@gmail.com> wrote:
>> On Tue, Nov 3, 2020 at 9:05 PM Heikki Linnakangas <hlinnaka@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