Re: Oddity in tuple routing for foreign partitions

(2018/04/17 16:10), Amit Langote wrote:
> On 2018/04/17 11:13, Kyotaro HORIGUCHI wrote:
>> If I'm reading this correctly, ExecInitParititionInfo calls
>> ExecInitRoutingInfo so currently CheckValidityResultRel is called
>> for the child when partrel is created in ExecPrepareTupleRouting.
>> But the move of CheckValidResultRel results in letting partrel
>> just created in ExecPrepareTupleRouting not be checked at all
>> since ri_ParititionReadyForRouting is always set true in
>> ExecInitPartitionInfo.
>
> I thought the same upon reading the email, but it seems that the patch
> does add CheckValidResultRel() in ExecInitPartitionInfo() as well:
>
> @@ -335,6 +335,13 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
>                         estate->es_instrument);
>
>       /*
> +     * Verify result relation is a valid target for an INSERT.  An UPDATE
> of a
> +     * partition-key becomes a DELETE+INSERT operation, so this check is
> still
> +     * required when the operation is CMD_UPDATE.
> +     */
> +    CheckValidResultRel(leaf_part_rri, CMD_INSERT);
> +
> +    /*
>        * Since we've just initialized this ResultRelInfo, it's not in any list
>        * attached to the estate as yet.  Add it, so that it can be found later.
>        *

That's right.  So, the validity check would be applied to a partition 
created in ExecPrepareTupleRouting as well.

>>>> Another
>>>> thing I noticed is the RT index of the foreign partition set to 1 in
>>>> postgresBeginForeignInsert to create a remote query; that would not work
>>>> for cases where the partitioned table has an RT index larger than 1 (eg,
>>>> the partitioned table is not the primary ModifyTable node); in which
>>>> cases, since the varno of Vars belonging to the foreign partition in the
>>>> RETURNING expressions is the same as the partitioned table, calling
>>>> deparseReturningList with the RT index set to 1 against the RETURNING
>>>> expressions would produce attrs_used to be NULL, leading to postgres_fdw
>>>> not retrieving actually inserted data from the remote, even if remote
>>>> triggers might change those data.  So, I fixed this as well, by setting
>>>> the RT index accordingly to the partitioned table.
>>>
>>> Check.
>>
>> I'm not sure but is it true that the parent is always at
>> resultRelation - 1?
>
> Unless I'm missing something, EState.es_range_table contains *all* range
> table entries that were created by the planner.  So, if the planner
> assigned resultRelation as the RT index for the parent, then it seems we
> can rely on getting the same entry back with
> list_nth_cell(estate->es_range_table, resultRelation - 1).  That seems
> true even if the parent wasn't the *primary* target relation.  For
> example, the following works with the patch:
>
> -- in addition to the tables shown in Fujita-san's email, define one more
> -- local partition
> create table locp2 partition of itrtest for values in (2);
>
> -- simple case
> insert into itrtest values (1, 'foo') returning *;
>   a |        b
> ---+-----------------
>   1 | foo triggered !
> (1 row)
>
> -- itrtest (the parent) appears as both the primary target relation and
> -- otherwise.  The latter in the form of being mentioned in the with query
>
> with ins (a, b) as (insert into itrtest values (1, 'foo') returning *)
> insert into itrtest select a + 1, b from ins returning *;
>   a |        b
> ---+-----------------
>   2 | foo triggered !
> (1 row)

I agree with that.  Thanks for the explanation and the example, Amit!

Maybe my explanation (or the naming parent_rte/child_rte in the patch) 
was not necessarily good, so I guess that that confused horiguchi-san. 
Let me explain.  In the INSERT/COPY-tuple-routing case, as explained by 
Amit, the RTE at that position in the EState's range table is the one 
for the partitioned table of a given partition, so the statement would 
be true.  BUT in the UPDATE-tuple-routing case, the RTE is the one for 
the given partition, not for the partitioned table, so the statement 
would not be true.  In the latter case, we don't need to create a child 
RTE and replace the original RTE with it, but I handled both cases the 
same way for simplicity.

Thanks for the comments, Horiguchi-san!

Best regards,
Etsuro Fujita