It seems almost fine for me, but just one point.. At Thu, 26 Apr 2018 17:36:44 +0900, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote in <5ae18f9c.6080...@lab.ntt.co.jp> > (2018/04/26 15:35), Amit Langote wrote: > > On 2018/04/26 12:43, Etsuro Fujita wrote: > > + resultRelation == plan->nominalRelation) > > + resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex; > > + } > > Seems like a better change than mine; because this simplifies the > logic.
Please rewrite it to use not array reference, but pointer reference if one mtstate logically holds just one resultRelInfo. > > I added the block inside the if because I agree with your comment > > below > > that the changes to ExecInitPartitionInfo are better kept for later to > > think more carefully about the dependencies it might break. > > OK > > >>> 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. > > > > OK, I have to agree here that we better leave 1 to be looked at later. > > > > After this change, GetInsertedColumns/GetUpdatedColumns will start > > returning a different set of columns in some cases than it does now. > > Currently, it *always* returns a set containing root table's attribute > > numbers, even for UPDATE. But with this change, for UPDATE, it will > > start > > returning the set containing the first subplan target rel's attribute > > numbers, which could be any partition with arbitrarily different > > attribute > > numbers. > > > >> 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? > > > > I agree, so I'm dropping the patch for 1. > > OK, let's focus on #2! > > > See attached an updated version with changes as described above. > > Looks good to me. Thanks for the updated version! Agreed on all points above. regards. -- Kyotaro Horiguchi NTT Open Source Software Center