Fujita-san, Thanks for the reply and sorry I didn't wait a bit more before posting the patch.
On Wed, Jul 31, 2019 at 9:04 PM Etsuro Fujita <etsuro.fuj...@gmail.com> wrote: > On Wed, Jul 31, 2019 at 5:05 PM Amit Langote <amitlangot...@gmail.com> wrote: > > On Tue, Jul 30, 2019 at 4:20 PM Amit Langote <amitlangot...@gmail.com> > > wrote: > > > On Sat, Jul 20, 2019 at 1:52 AM Andres Freund <and...@anarazel.de> wrote: > > > > IOW, we should just change the direct modify calls to get the relevant > > > > ResultRelationInfo or something in that vein (perhaps just the relevant > > > > RT index?). > > > > > > It seems easy to make one of the two functions that constitute the > > > direct modify API, IterateDirectModify(), access the result relation > > > from ForeignScanState by saving either the result relation RT index or > > > ResultRelInfo pointer itself into the ForeignScanState's FDW-private > > > area. For example, for postgres_fdw, one would simply add a new > > > member to PgFdwDirectModifyState struct. > > > > > > Doing that for the other function BeginDirectModify() seems a bit more > > > involved. We could add a new field to ForeignScan, say > > > resultRelation, that's set by either PlanDirectModify() (the FDW code) > > > or make_modifytable() (the core code) if the ForeignScan node contains > > > the command for direct modification. BeginDirectModify() can then use > > > that value instead of relying on es_result_relation_info being set. > > > > > > Thoughts? Fujita-san, do you have any opinion on whether that would > > > be a good idea? > > I'm still not sure that it's a good idea to remove > es_result_relation_info, but if I had to say then I think the latter > would probably be better. Could you please clarify what you meant by the "latter"? If it's the approach of adding a resultRelation Index field to ForeignScan node, I tried and had to give up, realizing that we don't maintain ResultRelInfos in an array that is indexable by RT indexes. It would've worked if es_result_relations had mirrored es_range_table, although that probably complicates how the individual ModifyTable nodes attach to that array. In any case, given this discussion, further hacking on a global variable like es_result_relations may be a course we might not want to pursue. > I'm planning to rework on direct > modification to base it on upper planner pathification so we can > perform direct modification without the ModifyTable node. (I'm not > sure we can really do this for inherited UPDATE/DELETE, though.) For > that rewrite, I'm thinking to call BeginDirectModify() from the > ForeignScan node (ie, ExecInitForeignScan()) as-is. The latter > approach would allow that without any changes and avoid changing that > API many times. That's the reason why I think the latter would > probably be better. Will the new planning approach you're thinking of get rid of needing any result relations at all (and so the ResultRelInfos in the executor)? Thanks, Amit