(2018/04/20 9:48), Amit Langote wrote:
> 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.
Thank you!
Best regards,
Etsuro Fujita
