Re: Oddity in tuple routing for foreign partitions

(2018/04/19 16:43), Amit Langote wrote:
> On 2018/04/18 21:55, Etsuro Fujita wrote:
>> (2018/04/18 14:44), Amit Langote wrote:
>>> That the resultRelInfo
>>> received by BeginForeignInsert (when called by ExecInitRoutingInfo) could
>>> be a reused UPDATE result relation.

>>> 2. This is UPDATE and the resultRelInfo that's received in
>>>      BeginForeignInsert has been freshly created in ExecInitPartitionInfo
>>>      and it bears node->nominalRelation or 1 as its ri_RangeTableIndex

>>> In all three cases, I think we can rely on using ri_RangeTableIndex to
>>> fetch a valid "parent" RTE from es_range_table.
>>
>> I slept on this, I noticed there is another bug in case #2.

>> In case #2, since we initialize expressions for the partition's
>> ResultRelInfo including RETURNING by translating the attnos of the
>> corresponding expressions in the ResultRelInfo for the first subplan
>> target rel, I think we should replace the RTE for the first subplan target
>> rel, not the RTE for the nominal rel, with the new one created for the
>> foreign table.
>
> Ah, so in case #2, we use firstVarno (per ExecInitPartitionInfo terms) as
> varno throughout.  So, we'd like to put the new RTE in that slot.
>
> 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.

>> Attached is an updated version for fixing this issue.
>>
>>> Do you think we need to clarify this in a comment?
>>
>> Yeah, but I updated comments about this a little bit different way in the
>> attached.  Does that make sense?
>
> 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.

Best regards,
Etsuro Fujita