Re: partition routing layering in nodeModifyTable.c

On Wed, Oct 7, 2020 at 9:07 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 07/10/2020 12:50, Amit Langote wrote:
> > On Tue, Oct 6, 2020 at 12:45 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >> It would be better to set it in make_modifytable(), just
> >> after calling PlanDirectModify().
> >
> > Actually, that's how it was done in earlier iterations but I think I
> > decided to move that into the FDW's functions due to some concern of
> > one of the other patches that depended on this patch.  Maybe it makes
> > sense to bring that back into make_modifytable() and worry about the
> > other patch later.
>
> On second thoughts, I take back my earlier comment. Setting it in
> make_modifytable() relies on the assumption that the subplan is a single
> ForeignScan node, on the target relation. The documentation for
> PlanDirectModify says:
>
> > To execute the direct modification on the remote server, this
> > function must rewrite the target subplan with a ForeignScan plan node
> > that executes the direct modification on the remote server.
>>
> So I guess that assumption is safe. But I'd like to have some wiggle
> room here. Wouldn't it be OK to have a Result node on top of the
> ForeignScan, for example? If it really must be a simple ForeignScan
> node, the PlanDirectModify API seems pretty strange.
>
> I'm not entirely sure what I would like to do with this now. I could
> live with either version, but I'm not totally happy with either. (I like
> your suggestion below)

Assuming you mean the idea of using RT index to access ResultRelInfos
in es_result_relations, we would still need to store the index in the
ForeignScan node, so the question of whether to do it in
make_modifytable() or in PlanDirectModify() must still be answered.

> Looking at this block in postgresBeginDirectModify:
>
> >       /*
> >        * Identify which user to do the remote access as.  This should match what
> >        * ExecCheckRTEPerms() does.
> >        */
> >       Assert(fsplan->resultRelIndex >= 0);
> >       dmstate->resultRelIndex = fsplan->resultRelIndex;
> >       rtindex = list_nth_int(resultRelations, fsplan->resultRelIndex);
> >       rte = exec_rt_fetch(rtindex, estate);
> >       userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
>
> That's a complicated way of finding out the target table's RTI. We
> should probably store the result RTI in the ForeignScan in the first place.
>
> >> Another idea is to merge "resultRelIndex" and a "range table index" into
> >> one value. Range table entries that are updated would have a
> >> ResultRelInfo, others would not. I'm not sure if that would end up being
> >> cleaner or messier than what we have now, but might be worth trying.
> >
> > I have thought about something like this before.  An idea I had is to
> > make es_result_relations array indexable by plain RT indexes, then we
> > don't need to maintain separate indexes that we do today for result
> > relations.
>
> That sounds like a good idea. es_result_relations is currently an array
> of ResultRelInfos, so that would leave a lot of unfilled structs in the
> array. But in on of your other threads, you proposed turning
> es_result_relations into an array of pointers anyway
> (https://www.postgresql.org/message-id/CA+HiwqE4k1Q2TLmCAvekw+8_NXepbnfUOamOeX=KpHRDTfSKxA@mail.gmail.com).

Okay, I am reorganizing the patches around that idea and will post an
update soon.


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