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
