(2018/04/26 20:06), Kyotaro HORIGUCHI wrote:
> 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.
Maybe I don't understand your words correctky, but in that UPDATE case,
I think that mtstate can have multiple ResultRelInfo.
>>>>>>> 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.
Thanks for reviewing!
Best regards,
Etsuro Fujita