Re: Oddity in tuple routing for foreign partitions

At Tue, 17 Apr 2018 16:41:31 +0900, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote in
<5AD5A52B.7050206@lab.ntt.co.jp>
> (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.

Yes, that's actually true but it seems quite bug-prone or at
least hard to understand. I understand that the
CheckValidResultRel(INSERT) is just a prerequisite for
ExecInitRoutingInfo but the relationship is obfucated after this
patch. If we have a strong reason to error-out as fast as
possible, the original code seems better to me..

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

Thank you, your understanding of my concern is right. Thanks for
the explanation.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center