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