Re: POC: postgres_fdw insert batching

Amit:
Good catch.

bq. ExecInitRoutingInfo() that is in the charge of initialing

Should be 'ExecInitRoutingInfo() that is in charge of initializing'

Cheers

On Sat, Jan 23, 2021 at 12:31 AM Amit Langote <amitlangote09@gmail.com>
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.
>
>
>
>
> --
> Amit Langote
> EDB: http://www.enterprisedb.com
>