From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com>
Cc: Zhihong Yu <zyu@yugabyte.com>
Cc: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: POC: postgres_fdw insert batching
Date: Sat, 23 Jan 2021 18:16:54 +0100
Message-ID: <067ddf09-225c-e3c5-ea47-4dd39b34ebb0@enterprisedb.com> (raw)
In-Reply-To: <CA+HiwqHTnfTG52XxNZriFTr+1r1VyrLm_u-xU5cUuXW-XuLUcA@mail.gmail.com>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@enterprisedb.com, amitlangote09@gmail.com, tsunakawa.takay@fujitsu.com, zyu@yugabyte.com, a.lepikhov@postgrespro.ru, tomas.vondra@2ndquadrant.com
Subject: Re: POC: postgres_fdw insert batching
In-Reply-To: <067ddf09-225c-e3c5-ea47-4dd39b34ebb0@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox