On Wed, Oct 7, 2020 at 9:07 PM Heikki Linnakangas <hlinn...@iki.fi> wrote: > 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)
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=kphrdtfs...@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
