Re: Oddity in tuple routing for foreign partitions

Fujita-san,

Thanks for the updated patch.

On 2018/04/19 21:42, Etsuro Fujita wrote:
> (2018/04/19 16:43), Amit Langote wrote:
>> Would it be a good idea to explain *why* we need to replace the RTE in the
>> first place?  Afaics, it's for deparseColumnRef() to find the correct
>> relation when it uses planner_rt_fetch() to get the RTE.
>
> That might be a good idea, but one thing I'm concerned about is that since
> we might use the RTE in different ways in future developments, such a
> comment might be obsolete rather sooner.  So, I just added *for use by
> deparseInsertSql() and create_foreign_modify() below* to the comments
> shown below.  But I'd like to leave this to the committer.

OK, fine by me.

>> It looks generally good, although in the following:
>>
>> +    /*
>> +     * If the foreign table is a partition, temporarily replace the
>> parent's
>> +     * RTE in the range table with a new target RTE describing the foreign
>> +     * table for use by deparseInsertSql() and create_foreign_modify()
>> below.
>> +     */
>>
>> .. it could be mentioned that we don't switch either the RTE or the value
>> assigned to resultRelation if the RTE currently at resultRelation RT index
>> is the one created by the planner and refers to the same relation that
>> resultRelInfo does.
>
> Done.
>
>> Beside that, I noticed that the patch has a stray white-space at the end
>> of the following line:
>>
>> +                /* partition that isn't a subplan target rel */
>
> Fixed.
>
> Thanks for the review!  Attached is a new version of the patch.

Looks good.

Thanks,
Amit