Re: Oddity in tuple routing for foreign partitions

Hi Amit,

(2018/04/17 10:10), Amit Langote wrote:
> On 2018/04/16 20:25, Etsuro Fujita wrote:
>> 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
abortthe
 
>> 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 for the review!

Best regards,
Etsuro Fujita