Subject: Re: POC: postgres_fdw insert batching
To: Amit Langote <amitlangote09@gmail.com>
Cc: "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>,
 Zhihong Yu <zyu@yugabyte.com>, "Andrey V. Lepikhov"
 <a.lepikhov@postgrespro.ru>, Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <20200628151002.7x5laxwpgvkyiu3q@development>
 <CA+HiwqGiwxSc8kc0PgPef5DgnP7NTRHVyAZJG3zxY4XBivbfLQ@mail.gmail.com>
 <9d94c863-ce7c-ea7f-6b84-834c98929e2f@enterprisedb.com>
 <CA+HiwqHTeQKqRpCA3TGZSCYTVOcUoUdXeSrRfjRRcmT1CfM1oA@mail.gmail.com>
 <7e70a11e-cf5d-2e5f-a802-274c5e0e236b@enterprisedb.com>
 <eb81a728-4ac3-823e-0017-54a9491f5600@enterprisedb.com>
 <CALNJ-vSt7Fn=oj4NxAyCDQQF-1yxmP=LYyQwciApg4L+QdYJQw@mail.gmail.com>
 <3ca9496f-0827-bf21-b709-593de63440a5@enterprisedb.com>
 <CA+HiwqF2nZX2VxZy0tL_BfuEQgof=w3b54Ei73LdwOx7sVemNg@mail.gmail.com>
 <4faa3d09-0fc8-c4d6-7aec-87428fabd161@enterprisedb.com>
 <CA+HiwqGUk0SSQ379aiEUkGrN_C981L8jUtgOPHoc9cR6NoFPHw@mail.gmail.com>
 <ebbd67c6-054a-f5eb-39a9-95e088f1c985@enterprisedb.com>
 <TYAPR01MB29903DF617C2B868C2477738FEA10@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <0fc809f6-7c3c-6d06-7ea3-f10765dfdf08@enterprisedb.com>
 <CA+HiwqHTnfTG52XxNZriFTr+1r1VyrLm_u-xU5cUuXW-XuLUcA@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <067ddf09-225c-e3c5-ea47-4dd39b34ebb0@enterprisedb.com>
Date: Sat, 23 Jan 2021 18:16:54 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: 
 <CA+HiwqHTnfTG52XxNZriFTr+1r1VyrLm_u-xU5cUuXW-XuLUcA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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