It seems almost fine for me, but just one point..
At Thu, 26 Apr 2018 17:36:44 +0900, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote in
<5AE18F9C.6080805@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
