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
