On Tue, Nov 3, 2020 at 9:05 PM Heikki Linnakangas <hlinn...@iki.fi> wrote:
> 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.
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.
> 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.
--
Amit Langote
EDB: http://www.enterprisedb.com
