Re: POC: postgres_fdw insert batching

On Sun, Jan 24, 2021 at 2:17 AM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> On 1/23/21 9:31 AM, Amit Langote wrote:
> > 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.

Yes, it can be simplified by using a local join to prevent the update
of the foreign partition from being pushed to the remote server, for
which my example in the previous email used a local trigger. Note
that the update of the foreign partition to be done locally is a
prerequisite for this bug to occur.

I've added that simplified test case in the attached updated patch.

--
Amit Langote
EDB: http://www.enterprisedb.com