On 2018/04/26 15:27, Etsuro Fujita wrote: > (2018/04/26 12:43), Etsuro Fujita wrote: >> (2018/04/25 17:29), Amit Langote wrote: >>> On 2018/04/25 16:42, Kyotaro HORIGUCHI wrote: >>>> At Wed, 25 Apr 2018 11:19:23 +0900, Amit Langote wrote: >>>>> After the refactoring, it appears to me that we only need this much: >>>>> >>>>> + rte = makeNode(RangeTblEntry); >>>>> + rte->rtekind = RTE_RELATION; >>>>> + rte->relid = RelationGetRelid(rel); >>>>> + rte->relkind = RELKIND_FOREIGN_TABLE; >>>> >>>> Mmm.. That is, only relid is required to deparse (I don't mean >>>> that it should be refactored so.). On the other hand >>>> create_foreign_modify requires rte->checkAsUser as well. >> >> That's right. I took care of this in my version, but unfortuneately, >> that was ignored in the updated versions. Maybe the comments I added to >> the patch were not enough, though. >> >>> Hmm, I missed that we do need information from a proper RTE as well. So, >>> I suppose we should be doing this instead of creating the RTE for foreign >>> partition from scratch. >>> >>> + rte = list_nth(estate->es_range_table, resultRelation - 1); >>> + rte = copyObject(rte); >>> + rte->relid = RelationGetRelid(rel); >>> + rte->relkind = RELKIND_FOREIGN_TABLE; >> >> As I said upthread, we can use the RTE in the range table as-is if the >> foreign table is one of the UPDATE subplan partitions or the target >> specified in a COPY command. So, I created the patch that way because >> that would save cycles. Why not just use that RTE in those cases? >> >>> If we apply the other patch I proposed, resultRelation always points to >>> the correct RTE. >>> >>>>> I tried to do that. So, attached are two patches. >>>>> >>>>> 1. Fix for ExecInitPartitionInfo to use the right RT index to pass to >>>>> InitResultRelInfo >>>>> >>>>> 2. v5 of the patch to fix the bug of foreign partitions >>>>> >>>>> Thoughts? >> >> Actually, I also thought the former when creating the patch, but I left >> that as-is because I'm not sure that would be really safe; >> ExecConstraints looks at the RT index via >> GetInsertedColumns/GetUpdatedColumns, so what I thought was: that might >> cause unexpected behavior. Anyway, I think that the former is more like >> an improvement rather than a fix, so it would be better to leave that >> for another patch for PG12? > > Here is a new version I'd like to propose for fixing this issue without > the former patch. > > Thanks for working on this!
Sorry, didn't notice this before sending my patch, which I also marked v7. It's a bit different than yours and has comments with excerpts from your earlier versions. See if you find it helpful. Thanks, Amit
