Re: Oddity in tuple routing for foreign partitions

(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?

Best regards,
Etsuro Fujita