Re: Oddity in tuple routing for foreign partitions - Mailing list pgsql-hackers

On 2018/04/17 11:13, Kyotaro HORIGUCHI wrote:
>>> Also, I removed the CheckValidResultRel check from ExecInitRoutingInfo
>>> and added that to ExecInitPartitionInfo right after the> InitResultRelInfo call,
>>> because it would be better to abort the
>>> operation as soon as we find the partition to be non-routable.
>>
>> Sounds fine.
> 
> 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.
      *


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

But maybe I misunderstood your question.

Thanks,
Amit