Subject: Re: POC: postgres_fdw insert batching
To: Amit Langote <amitlangote09@gmail.com>,
 "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>
Cc: 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>
 <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>
 <067ddf09-225c-e3c5-ea47-4dd39b34ebb0@enterprisedb.com>
 <CA+HiwqHrGA0HB5cjBL_0=Gk-u-dH=K8beAPibhOCFepU0kqndw@mail.gmail.com>
 <TYAPR01MB2990E3D10A122358419482B2FEBD0@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <CA+HiwqGjMVLEn8JoNT+XCeXA9SFECVvTEMawbzJ-RN_M+fxMTw@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <7ffcee1f-d55e-cbdf-5f3f-98f1c30537ef@enterprisedb.com>
Date: Mon, 15 Feb 2021 17:36:00 +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+HiwqGjMVLEn8JoNT+XCeXA9SFECVvTEMawbzJ-RN_M+fxMTw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/5/21 3:52 AM, Amit Langote wrote:
> Tsunakwa-san,
> 
> On Mon, Jan 25, 2021 at 1:21 PM tsunakawa.takay@fujitsu.com
> <tsunakawa.takay@fujitsu.com> wrote:
>> From: Amit Langote <amitlangote09@gmail.com>
>>> 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.
>>
>> Thank you, I was aware that UPDATE calls ExecInsert() but forgot about it partway.  Good catch (and my bad miss.)
> 
> It appears I had missed your reply, sorry.
> 
>> +       PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState ?
>> +                                                       (PgFdwModifyState *) resultRelInfo->ri_FdwState :
>> +                                                       NULL;
>>
>> This can be written as:
>>
>> +       PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
> 
> Facepalm, yes.
> 
> Patch updated.  Thanks for the review.
> 

Thanks for the patch, it seems fine to me. I wonder it the commit
message needs some tweaks, though. At the moment it says:

    Prevent FDW insert batching during cross-partition updates

but what the patch seems to be doing is simply initializing the info
only for CMD_INSERT operations. Which does the trick, but it affects
everything, i.e. all updates, no? Not just cross-partition updates.


regards

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