Re: partition routing layering in nodeModifyTable.c

On Wed, Jul 31, 2019 at 5:05 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Tue, Jul 30, 2019 at 4:20 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Sat, Jul 20, 2019 at 1:52 AM Andres Freund <andres@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.  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.

> 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.

Sorry for speaking this late.

Best regards,
Etsuro Fujita