Amit-san, On Mon, Aug 5, 2019 at 1:31 PM Amit Langote <amitlangot...@gmail.com> wrote: > On Sun, Aug 4, 2019 at 4:45 AM Etsuro Fujita <etsuro.fuj...@gmail.com> wrote: > > On Sun, Aug 4, 2019 at 3:03 AM Andres Freund <and...@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()?
I think so. > 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. I think so. > That information is the ordinal > position of a given result relation in PlannedStmt.resultRelations, > not the RT index as we were discussing. Yeah, that would be what Andres is proposing, which I think is much better than what I proposed using the RT index. Could you update your patch? Best regards, Etsuro Fujita
