Re: Oddity in tuple routing for foreign partitions

Hello.

At Tue, 17 Apr 2018 10:10:38 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <fefb9510-e304-27da-e984-a2b0ac77927a(at)lab(dot)ntt(dot)co(dot)jp>
> Fujita-san,
>
> On 2018/04/16 20:25, Etsuro Fujita wrote:
> > Hi,
> >
> > While updating the fix-postgres_fdw-WCO-handling patch, I noticed that
> > the patch for tuple routing for foreign partitions doesn't work well
> > with remote triggers. Here is an example:
> >
> > postgres=# create table loct1 (a int check (a in (1)), b text);
> > postgres=# create function br_insert_trigfunc() returns trigger as
> > $$begin new.b := new.b || ' triggered !'; return new; end$$ language
> > plpgsql;
> > postgres=# create trigger loct1_br_insert_trigger before insert on loct1
> > for each row execute procedure br_insert_trigfunc();
> > postgres=# create table itrtest (a int, b text) partition by list (a);
> > postgres=# create foreign table remp1 (a int check (a in (1)), b text)
> > server loopback options (table_name 'loct1');
> > postgres=# alter table itrtest attach partition remp1 for values in (1);
> >
> > This behaves oddly:
> >
> > postgres=# insert into itrtest values (1, 'foo') returning *;
> > a | b
> > ---+-----
> > 1 | foo
> > (1 row)
> >
> > INSERT 0 1
> >
> > The new value of b in the RETURNING result should be concatenated with '
> > triggered !', as shown below:
> >
> > postgres=# select tableoid::regclass, * from itrtest ;
> > tableoid | a | b
> > ----------+---+-----------------
> > remp1 | 1 | foo triggered !
> > (1 row)
> >
> > The reason for that is: since that in ExecInitPartitionInfo, the core
> > calls BeginForeignInsert via ExecInitRoutingInfo before initializing
> > ri_returningList of ResultRelInfo for the partition, BeginForeignInsert
> > sees that the ri_returningList is NULL and incorrectly recognizes that
> > the local query doesn't have a RETURNING list.
>
> Good catch!
>
> > So, I moved the
> > ExecInitRoutingInfo call after building the ri_returningList (and before
> > handling ON CONFLICT because we reference the conversion map created by
> > that when handling that clause). The first version of the patch called
> > BeginForeignInsert that order, but I updated the patch incorrectly. :(
>
> I didn't notice that when reviewing either. Actually, I was under the
> impression that BeginForeignInsert consumes returningList from the
> ModifyTable node itself, not the ResultRelInfo, but now I see that
> ri_returningList was added exactly for BeginForeignInsert to consume.
> Good thing about that is that we get a Var-translated version instead of
> the original one that contains parent's attnos.
>
> > 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.

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

> > Attached is a patch
> > for fixing these issues. I'll add this to the open items list for PG11.
> > (After addressing this, I'll post an updated version of the
> > fix-postgres_fdw-WCO-handling patch.)
>
> Your patch seems to fix the issue and code changes seem fine to me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center