On 07/10/2020 12:50, Amit Langote wrote:
On Tue, Oct 6, 2020 at 12:45 AM Heikki Linnakangas <hlinn...@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=kphrdtfs...@mail.gmail.com).
- Heikki
