Subject: Re: POC: postgres_fdw insert batching
To: Amit Langote <amitlangote09@gmail.com>
Cc: "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>
 <CA+HiwqESrO5BKHtWZZTSbK26OWk-Uhx6DYtdVEkpdaoZ=uW5BA@mail.gmail.com>
 <OSBPR01MB29820BAA17AD3A79A49935EFFEA40@OSBPR01MB2982.jpnprd01.prod.outlook.com>
 <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>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <261fbedb-1ced-82f7-e978-60cfe17bce07@enterprisedb.com>
Date: Thu, 21 Jan 2021 01:21:29 +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: <eb81a728-4ac3-823e-0017-54a9491f5600@enterprisedb.com>
Content-Type: multipart/mixed;
 boundary="------------36DD44A54A605C0C7B4EAA1A"
Content-Language: en-US
Precedence: bulk

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

On 1/21/21 12:52 AM, Tomas Vondra wrote:
> Hmm, seems that florican doesn't like this :-(
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=florican&dt=2021-01-20%2023%3A08%3A15 
> 
> 
> It's a i386 machine running FreeBSD, so not sure what exactly it's picky 
> about. But when I tried running this under valgrind, I get some strange 
> failures in the new chunk in ExecInitModifyTable:
> 
>    /*
>     * Determine if the FDW supports batch insert and determine the batch
>     * 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;
> 
>    Assert(resultRelInfo->ri_BatchSize >= 1);
> 
> It seems as if the resultRelInfo is not initialized, or something like 
> that. I wouldn't be surprised if the 32-bit machine was pickier and 
> failing because of that.
> 
> A sample of the valgrind log is attached. It's pretty much just 
> repetitions of these three reports.
> 

OK, it's definitely accessing uninitialized memory, because the 
resultRelInfo (on line 2801, i.e. the "if" condition) looks like this:

(gdb) p resultRelInfo
$1 = (ResultRelInfo *) 0xe595988
(gdb) p *resultRelInfo
$2 = {type = 2139062142, ri_RangeTableIndex = 2139062143, 
ri_RelationDesc = 0x7f7f7f7f7f7f7f7f, ri_NumIndices = 2139062143, 
ri_IndexRelationDescs = 0x7f7f7f7f7f7f7f7f, ri_IndexRelationInfo = 
0x7f7f7f7f7f7f7f7f,
   ri_TrigDesc = 0x7f7f7f7f7f7f7f7f, ri_TrigFunctions = 
0x7f7f7f7f7f7f7f7f, ri_TrigWhenExprs = 0x7f7f7f7f7f7f7f7f, 
ri_TrigInstrument = 0x7f7f7f7f7f7f7f7f, ri_ReturningSlot = 
0x7f7f7f7f7f7f7f7f, ri_TrigOldSlot = 0x7f7f7f7f7f7f7f7f,
   ri_TrigNewSlot = 0x7f7f7f7f7f7f7f7f, ri_FdwRoutine = 
0x7f7f7f7f7f7f7f7f, ri_FdwState = 0x7f7f7f7f7f7f7f7f, 
ri_usesFdwDirectModify = 127, ri_NumSlots = 2139062143, ri_BatchSize = 
2139062143, ri_Slots = 0x7f7f7f7f7f7f7f7f,
   ri_PlanSlots = 0x7f7f7f7f7f7f7f7f, ri_WithCheckOptions = 
0x7f7f7f7f7f7f7f7f, ri_WithCheckOptionExprs = 0x7f7f7f7f7f7f7f7f, 
ri_ConstraintExprs = 0x7f7f7f7f7f7f7f7f, ri_GeneratedExprs = 
0x7f7f7f7f7f7f7f7f,
   ri_NumGeneratedNeeded = 2139062143, ri_junkFilter = 
0x7f7f7f7f7f7f7f7f, ri_returningList = 0x7f7f7f7f7f7f7f7f, 
ri_projectReturning = 0x7f7f7f7f7f7f7f7f, ri_onConflictArbiterIndexes = 
0x7f7f7f7f7f7f7f7f,
   ri_onConflict = 0x7f7f7f7f7f7f7f7f, ri_PartitionCheckExpr = 
0x7f7f7f7f7f7f7f7f, ri_PartitionRoot = 0x7f7f7f7f7f7f7f7f, 
ri_RootToPartitionMap = 0x8, ri_PartitionTupleSlot = 0x8, 
ri_ChildToRootMap = 0xe5952b0,
   ri_CopyMultiInsertBuffer = 0xe596740}
(gdb)

I may be wrong, but the most likely explanation seems to be this is due 
to the junk filter initialization, which simply moves past the end of 
the mtstate->resultRelInfo array.

It kinda seems the GetForeignModifyBatchSize call should happen before 
that block. The attached patch fixes this for me (i.e. regression tests 
pass with no valgrind reports.

Or did I get that wrong?

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

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

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 9c36860704..2ac4999dc8 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2672,6 +2672,23 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
 		}
 	}
 
+	/*
+	 * Determine if the FDW supports batch insert and determine the batch
+	 * 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;
+
+	Assert(resultRelInfo->ri_BatchSize >= 1);
+
 	/* select first subplan */
 	mtstate->mt_whichplan = 0;
 	subplan = (Plan *) linitial(node->plans);
@@ -2793,23 +2810,6 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
 		}
 	}
 
-	/*
-	 * Determine if the FDW supports batch insert and determine the batch
-	 * 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;
-
-	Assert(resultRelInfo->ri_BatchSize >= 1);
-
 	/*
 	 * Lastly, if this is not the primary (canSetTag) ModifyTable node, add it
 	 * to estate->es_auxmodifytables so that it will be run to completion by

--------------36DD44A54A605C0C7B4EAA1A--