Hi,
On Sun, Aug 4, 2019 at 4:45 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Sun, Aug 4, 2019 at 3:03 AM Andres Freund <andres@anarazel.de> wrote:
> > On 2019-08-03 13:48:01 -0400, Tom Lane wrote:
> > > If those are the choices, adding a parameter is clearly the preferable
> > > solution, because it makes the API breakage obvious at compile.
> >
> > Right. I think it's a *bit* less clear in this case because we'd also
> > remove the field that such FDWs with direct modify support would use
> > now (EState.es_result_relation_info).
> >
> > But I think it's also just plainly a better API to use the
> > parameter. Even if, in contrast to the BeginDirectModify at hand,
> > BeginForeignModify didn't already accept it. Requiring a function call to
> > gather information that just about every realistic implementation is
> > going to need doesn't make sense.
>
> Agreed.
So, is it correct to think that the consensus is to add a parameter to
BeginDirectModify()?
Also, avoid changing where BeginDirectModify() is called from, like my
patch did, only to have easy access to the ResultRelInfo to pass. We
can do that by by augmenting ForeignScan node to add the information
needed to fetch the ResultRelInfo efficiently from
ExecInitForeignScan() itself. That information is the ordinal
position of a given result relation in PlannedStmt.resultRelations,
not the RT index as we were discussing.
Thanks,
Amit