Re: Oddity in tuple routing for foreign partitions

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.

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

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

Thanks,
Amit