Re: partition routing layering in nodeModifyTable.c

On 07/10/2020 12:50, Amit Langote wrote:
> On Tue, Oct 6, 2020 at 12:45 AM Heikki Linnakangas <hlinnaka(at)iki(dot)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)

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

- Heikki