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

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