On Wed, Nov 4, 2020 at 11:32 AM Amit Langote <amitlangot...@gmail.com> wrote:
> 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.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. -- Amit Langote EDB: http://www.enterprisedb.com
v8-0001-Set-ForeignScanState.resultRelInfo-lazily.patch
Description: Binary data
v8-0002-Initialize-result-relation-information-lazily.patch
Description: Binary data
