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(dot)vondra(at)enterprisedb(dot)com> wrote:
>> On 1/21/21 3:09 AM, tsunakawa(dot)takay(at)fujitsu(dot)com wrote:
>>> From: Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)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