Re: POC: postgres_fdw insert batching

On 1/23/21 9:31 AM, Amit Langote wrote:
> On Thu, Jan 21, 2021 at 11:36 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>> On 1/21/21 3:09 AM, tsunakawa.takay@fujitsu.com wrote:
>>> From: Tomas Vondra <tomas.vondra@enterprisedb.com>
>>>> Right, that's pretty much what I ended up doing (without the CMD_INSERT
>>>> check it'd add batching info to explain for updates too, for example).
>>>> I'll do a bit more testing on the attached patch, but I think that's the right fix to
>>>> push.
>>>
>>> Thanks to the outer check for operation ==  CMD_INSERT, the inner one became unnecessary.
>>>
>>
>> Right. I've pushed the fix, hopefully buildfarm will get happy again.
> 
> I was looking at this and it looks like we've got a problematic case
> where postgresGetForeignModifyBatchSize() is called from
> ExecInitRoutingInfo().
> 
> That case is when the insert is performed as part of a cross-partition
> update of a partitioned table containing postgres_fdw foreign table
> partitions.  Because we don't check the operation in
> ExecInitRoutingInfo() when calling GetForeignModifyBatchSize(), such
> inserts attempt to use batching.  However the ResultRelInfo may be one
> for the original update operation, so ri_FdwState contains a
> PgFdwModifyState with batch_size set to 0, because updates don't
> support batching.  As things stand now,
> postgresGetForeignModifyBatchSize() simply returns that, tripping the
> following Assert in the caller.
> 
> Assert(partRelInfo->ri_BatchSize >= 1);
> 
> Use this example to see the crash:
> 
> create table p (a int) partition by list (a);
> create table p1 (like p);
> create extension postgres_fdw;
> create server lb foreign data wrapper postgres_fdw ;
> create user mapping for current_user server lb;
> create foreign table fp1 (a int) server lb options (table_name 'p1');
> alter table p attach partition fp1 for values in (1);
> create or replace function report_trig_details() returns trigger as $$
> begin raise notice '% % on %', tg_when, tg_op, tg_relname; if tg_op =
> 'DELETE' then return old; end if; return new; end; $$ language
> plpgsql;
> create trigger trig before update on fp1 for each row execute function
> report_trig_details();
> create table p2 partition of p for values in (2);
> insert into p values (2);
> update p set a = 1;  -- crashes
> 
> So we let's check mtstate->operation == CMD_INSERT in
> ExecInitRoutingInfo() to prevent calling GetForeignModifyBatchSize()
> in cross-update situations where mtstate->operation would be
> CMD_UPDATE.
> 
> I've attached a patch.
> 

Thanks for catching this. I think it'd be good if the fix included a 
regression test. The example seems like a good starting point, not sure 
if it can be simplified further.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company