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