Re: partition routing layering in nodeModifyTable.c

On Tue, Jul 30, 2019 at 4:20 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Sat, Jul 20, 2019 at 1:52 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > The first one (0001) deals with reducing the core executor's reliance
> > > on es_result_relation_info to access the currently active result
> > > relation, in favor of receiving it from the caller as a function
> > > argument. So no piece of core code relies on it being correctly set
> > > anymore. It still needs to be set correctly for the third-party code
> > > such as FDWs.
> >
> > I'm inclined to just remove it. There's not much code out there relying
> > on it, as far as I can tell. Most FDWs don't support the direct modify
> > API, and that's afaict the case where we one needs to use
> > es_result_relation_info?
>
> Right, only the directly modify API uses it.
>
> > In fact, I searched through alllFDWs listed on https://wiki.postgresql.org/wiki/Foreign_data_wrappers
> > that are on github and in first few categories (up and including to
> > "file wrappers"), and there was only one reference to
> > es_result_relation_info, and that just in comments in a test:
> > https://github.com/pgspider/griddb_fdw/search?utf8=%E2%9C%93&q=es_result_relation_info&type=
> > which I think was just copied from our source code.
> >
> > 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 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. Patches that
were previously 0001, 0002, and 0003 are now 0002, 003, and 0004,
respectively. 0002 is now a patch to "remove"
es_result_relation_info.

Thanks,
Amit