Re: ModifyTable overheads in generic plans

On 03/11/2020 10:27, Amit Langote wrote:
> Please check the attached if that looks better.

Great, thanks! Yeah, I like that much better.

This makes me a bit unhappy:

> 
>         /* Also let FDWs init themselves for foreign-table result rels */
>         if (resultRelInfo->ri_FdwRoutine != NULL)
>         {
>             if (resultRelInfo->ri_usesFdwDirectModify)
>             {
>                 ForeignScanState *fscan = (ForeignScanState *) mtstate->mt_plans[i];
> 
>                 /*
>                  * For the FDW's convenience, set the ForeignScanState node's
>                  * ResultRelInfo to let the FDW know which result relation it
>                  * is going to work with.
>                  */
>                 Assert(IsA(fscan, ForeignScanState));
>                 fscan->resultRelInfo = resultRelInfo;
>                 resultRelInfo->ri_FdwRoutine->BeginDirectModify(fscan, eflags);
>             }
>             else if (resultRelInfo->ri_FdwRoutine->BeginForeignModify != NULL)
>             {
>                 List   *fdw_private = (List *) list_nth(node->fdwPrivLists, i);
> 
>                 resultRelInfo->ri_FdwRoutine->BeginForeignModify(mtstate,
>                                                                  resultRelInfo,
>                                                                  fdw_private,
>                                                                  i,
>                                                                  eflags);
>             }
>         }

If you remember, I was unhappy with a similar assertion in the earlier 
patches [1]. I'm not sure what to do instead though. A few options:

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.

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.

[1] 
https://www.postgresql.org/message-id/19c23dd9-89ce-75a3-9105-5fc05a46f94a%40iki.fi

- Heikki