Re: partition routing layering in nodeModifyTable.c

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.fujita@gmail.com> wrote:
> 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.

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