Subject: Re: POC: postgres_fdw insert batching
To: Amit Langote <amitlangote09@gmail.com>
Cc: Zhihong Yu <zyu@yugabyte.com>,
 "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.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>
 <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>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <4faa3d09-0fc8-c4d6-7aec-87428fabd161@enterprisedb.com>
Date: Thu, 21 Jan 2021 02:42:03 +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+HiwqF2nZX2VxZy0tL_BfuEQgof=w3b54Ei73LdwOx7sVemNg@mail.gmail.com>
Content-Type: multipart/mixed;
 boundary="------------6D43F4FAD81726010FF254B9"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------6D43F4FAD81726010FF254B9
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit


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

--------------6D43F4FAD81726010FF254B9
Content-Type: text/x-patch; charset=UTF-8;
 name="modifytable-fix-2.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="modifytable-fix-2.patch"

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 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

--------------6D43F4FAD81726010FF254B9--