Hi, On Sat, Aug 3, 2019 at 5:31 AM Andres Freund <and...@anarazel.de> wrote: > On 2019-08-03 05:20:35 +0900, Etsuro Fujita wrote: > > On Sat, Aug 3, 2019 at 3:01 AM Andres Freund <and...@anarazel.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
