(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
