Re: Oddity in tuple routing for foreign partitions

(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