Re: partition routing layering in nodeModifyTable.c

On Tue, Oct 13, 2020 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 13/10/2020 07:32, Amit Langote wrote:
> > On Tue, Oct 13, 2020 at 1:57 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >> It occurred to me that if we do that (initialize the array lazily),
> >> there's very little need for the PlannedStmt->resultRelations list
> >> anymore. It's only used in ExecRelationIsTargetRelation(), but if we
> >> assume that ExecRelationIsTargetRelation() is only called after InitPlan
> >> has initialized the result relation for the relation, it can easily
> >> check es_result_relations instead. I think that's a safe assumption.
> >> ExecRelationIsTargetRelation() is only used in FDWs, and I believe the
> >> FDWs initialization routine can only be called after ExecInitModifyTable
> >> has been called on the relation.
> >>
> >> The PlannedStmt->rootResultRelations field is even more useless.
> >
> > I am very much tempted to remove those fields from PlannedStmt,
> > although I am concerned that the following now assumes that *all*
> > result relations are initialized in the executor initialization phase:
> >
> > bool
> > ExecRelationIsTargetRelation(EState *estate, Index scanrelid)
> > {
> >      if (!estate->es_result_relations)
> >          return false;
> >
> >      return estate->es_result_relations[scanrelid - 1] != NULL;
> > }
> >
> > In the other thread [1], I am proposing that we initialize result
> > relations lazily, but the above will be a blocker to that.
>
> Ok, I'll leave it alone then. But I'll still merge resultRelations and
> rootResultRelations into one list. I don't see any point in keeping them
> separate.

Should be fine.  As you said in the commit message, it should probably
have been that way to begin with, but I don't recall why I didn't make
it so.

> I'm tempted to remove ExecRelationIsTargetRelation() altogether, but
> keeping the resultRelations list isn't really a big deal, so I'll leave
> that for another discussion.

Yeah, makes sense.

> > Anyway, other than my concern about ExecRelationIsTargetRelation()
> > mentioned above, I think the patch looks good.
>
> Ok, committed. I'll continue to look at the rest of the patches in this
> patch series now.

Thanks.

BTW, you mentioned the lazy ResultRelInfo optimization bit in the
commit message, so does that mean you intend to take a look at the
other thread [1] too?  Or should I post a rebased version of the lazy
ResultRelInfo initialization patch here in this thread?  That patch is
just a bunch of refactoring too.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

[1] https://commitfest.postgresql.org/30/2621/