Re: Oddity in tuple routing for foreign partitions

On 2018/04/18 21:55, Etsuro Fujita wrote:
> (2018/04/18 14:44), Amit Langote wrote:
>> Oh, I didn't really consider this part carefully.  That the resultRelInfo
>> received by BeginForeignInsert (when called by ExecInitRoutingInfo) could
>> be a reused UPDATE result relation.  It might be possible to justify the
>> parent_rte/child_rte terminology by explaining it a bit better.
> 
>> 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.  Here is an
> example with the previous patch:
> 
> postgres=# create table utrtest (a int, b text) partition by list (a);
> postgres=# create table loct (a int check (a in (1)), b text);
> postgres=# create foreign table remp (a int check (a in (1)), b text)
> server loopback options (table_name 'loct');
> postgres=# create table locp (a int check (a in (2)), b text);
> postgres=# alter table utrtest attach partition remp for values in (1);
> postgres=# alter table utrtest attach partition locp for values in (2);
> postgres=# create trigger loct_br_insert_trigger before insert on loct for
> each row execute procedure br_insert_trigfunc();
> 
> where the trigger function is the one defined in an earlier email.
> 
> postgres=# insert into utrtest values (2, 'qux');
> INSERT 0 1
> postgres=# update utrtest set a = 1 where a = 2 returning *;
>  a |  b
> ---+-----
>  1 | qux
> (1 row)
> 
> UPDATE 1
> 
> Wrong result!  The result should be concatenated with ' triggered !'.
> 
> 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.

> 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.

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 */

Thanks,
Amit