From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Zhihong Yu <zyu@yugabyte.com>
Cc: tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.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: Thu, 21 Jan 2021 02:42:03 +0100
Message-ID: <4faa3d09-0fc8-c4d6-7aec-87428fabd161@enterprisedb.com> (raw)
In-Reply-To: <CA+HiwqF2nZX2VxZy0tL_BfuEQgof=w3b54Ei73LdwOx7sVemNg@mail.gmail.com>
References: <20200628151002.7x5laxwpgvkyiu3q@development>
<16fcfd4c-2d5e-67f7-fa69-9b5d119bac52@enterprisedb.com>
<TYAPR01MB299026DB0E91E513664C162FFEA30@TYAPR01MB2990.jpnprd01.prod.outlook.com>
<ba58f299-d50b-f347-4a0f-778cb6ac5588@enterprisedb.com>
<TYAPR01MB2990A053D902EDF6244090BDFEA30@TYAPR01MB2990.jpnprd01.prod.outlook.com>
<CA+HiwqH-7+L0B7iNWcAe8auWch=-EnbRkLYMG-27K4rUKsWKOQ@mail.gmail.com>
<TYAPR01MB29903CBD68C9DA42B67D9EB6FEA30@TYAPR01MB2990.jpnprd01.prod.outlook.com>
<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>
On 1/21/21 2:24 AM, Amit Langote wrote:
> On Thu, Jan 21, 2021 at 9:56 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>> On 1/21/21 1:17 AM, Zhihong Yu wrote:
>>> Hi,
>>> The assignment to resultRelInfo is done when junk_filter_needed is true:
>>>
>>> if (junk_filter_needed)
>>> {
>>> resultRelInfo = mtstate->resultRelInfo;
>>>
>>> Should the code for determining batch size access mtstate->resultRelInfo
>>> directly ?
>>>
>>
>> IMO the issue is that code iterates over all plans and moves to the next
>> for each one:
>>
>> resultRelInfo++;
>>
>> so it ends up pointing past the last element, hence the failures. So
>> yeah, either the code needs to move before the loop (per my patch), or
>> we need to access mtstate->resultRelInfo directly.
>
> Accessing mtstate->resultRelInfo directly would do. The only
> constraint on where this block should be placed is that
> ri_projectReturning must be valid as of calling
> GetForeignModifyBatchSize(), as Tsunakawa-san pointed out upthread.
> So, after this block in ExecInitModifyTable:
>
> /*
> * Initialize RETURNING projections if needed.
> */
> if (node->returningLists)
> {
> ....
> /*
> * Build a projection for each result rel.
> */
> resultRelInfo = mtstate->resultRelInfo;
> foreach(l, node->returningLists)
> {
> List *rlist = (List *) lfirst(l);
>
> resultRelInfo->ri_returningList = rlist;
> resultRelInfo->ri_projectReturning =
> ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps,
> resultRelInfo->ri_RelationDesc->rd_att);
> resultRelInfo++;
> }
> }
>
Right. But I think Tom is right this should initialize ri_BatchSize for
all the resultRelInfo elements, not just the first one. Per the attached
patch, which resolves the issue both on x86_64 and armv7l for me.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[text/x-patch] modifytable-fix-2.patch (1.4K, ../4faa3d09-0fc8-c4d6-7aec-87428fabd161@enterprisedb.com/2-modifytable-fix-2.patch)
download | inline diff:diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.cindex 9c36860704..10febcae8a 100644--- a/src/backend/executor/nodeModifyTable.c+++ b/src/backend/executor/nodeModifyTable.c@@ -2798,17 +2798,23 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
* size (a FDW may support batching, but it may be disabled for the
* server/table).
*/
- if (!resultRelInfo->ri_usesFdwDirectModify &&- operation == CMD_INSERT &&- resultRelInfo->ri_FdwRoutine != NULL &&- resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize &&- resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert)- resultRelInfo->ri_BatchSize =- resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(resultRelInfo);- else- resultRelInfo->ri_BatchSize = 1;+ resultRelInfo = mtstate->resultRelInfo;+ for (i = 0; i < nplans; i++)+ {+ if (!resultRelInfo->ri_usesFdwDirectModify &&+ operation == CMD_INSERT &&+ resultRelInfo->ri_FdwRoutine != NULL &&+ resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize &&+ resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert)+ resultRelInfo->ri_BatchSize =+ resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(resultRelInfo);+ else+ resultRelInfo->ri_BatchSize = 1;++ Assert(resultRelInfo->ri_BatchSize >= 1);- Assert(resultRelInfo->ri_BatchSize >= 1);+ resultRelInfo++;+ }
/*
* Lastly, if this is not the primary (canSetTag) ModifyTable node, add it
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, zyu@yugabyte.com, tsunakawa.takay@fujitsu.com, a.lepikhov@postgrespro.ru, tomas.vondra@2ndquadrant.com
Subject: Re: POC: postgres_fdw insert batching
In-Reply-To: <4faa3d09-0fc8-c4d6-7aec-87428fabd161@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