Re: partition routing layering in nodeModifyTable.c

Amit-san,

On Mon, Aug 5, 2019 at 1:31 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Sun, Aug 4, 2019 at 4:45 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> > On Sun, Aug 4, 2019 at 3:03 AM Andres Freund <andres(at)anarazel(dot)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