Re: Oddity in tuple routing for foreign partitions

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