Re: partition routing layering in nodeModifyTable.c

Hi,

On Sat, Aug 3, 2019 at 5:31 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2019-08-03 05:20:35 +0900, Etsuro Fujita wrote:
> > On Sat, Aug 3, 2019 at 3:01 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > On 2019-07-31 17:04:38 +0900, Amit Langote wrote:
> > > > I looked into trying to do the things I mentioned above and it seems
> > > > to me that revising BeginDirectModify()'s API to receive the
> > > > ResultRelInfo directly as Andres suggested might be the best way
> > > > forward. I've implemented that in the attached 0001.
> >
> > > Fujita-san, do you have any comments on the FDW API change? Or anybody
> > > else?
> > >
> > > I'm a bit woried about the move of BeginDirectModify() into
> > > nodeModifyTable.c - it just seems like an odd control flow to me. Not
> > > allowing any intermittent nodes between ForeignScan and ModifyTable also
> > > seems like an undesirable restriction for the future. I realize that we
> > > already do that for BeginForeignModify() (just btw, that already accepts
> > > resultRelInfo as a parameter, so being symmetrical for BeginDirectModify
> > > makes sense), but it still seems like the wrong direction to me.
> > >
> > > The need for that move, I assume, comes from needing knowing the correct
> > > ResultRelInfo, correct? I wonder if we shouldn't instead determine the
> > > at plan time (in setrefs.c), somewhat similar to how we determine
> > > ModifyTable.resultRelIndex. Doesn't look like that'd be too hard?
> >
> > I'd vote for that; I created a patch for that [1].
> >
> > [1] https://www.postgresql.org/message-id/CAPmGK15%3DoFHmWNND5vopfokSGfn6jMXVvnHa7K7P49F7k1hWPQ%40mail.gmail.com
>
> Oh, missed that. But that's not quite what I'm proposing.

Sorry, I misread your message. I think I was too tired.

> I don't like
> ExecFindResultRelInfo at all. What's the point of it? It's introduction
> is still an API break - I don't understand why that break is better than
> just passing the ResultRelInfo directly to BeginDirectModify()?

What API does that function break? The point of that function was to
keep the direct modify layering/API as-is, because 1) I too felt the
same way about the move of BeginDirectModify() to nodeModifyTable.c,
and 2) I was thinking that when rewriting direct modify with upper
planner pathification so that we can perform it without ModifyTable,
we could still use the existing layering/API as-is, leading to smaller
changes to the core for that.

> I want
> to again remark that BeginForeignModify() does get the ResultRelInfo -
> it should have been done the same when adding direct modify.

Might have been so.

> Even if you need the loop - which I don't think is right - it should
> live somewhere that individual FDWs don't have to care about.

I was thinking to use hash lookup in ExecFindResultRelInfo() when
es_result_relations is very long, but I think the setters.c approach
you mentioned above might be much better. Will consider that.

Best regards,
Etsuro Fujita