From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Amit Langote <amitlangote09@gmail.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 01:21:29 +0100
Message-ID: <261fbedb-1ced-82f7-e978-60cfe17bce07@enterprisedb.com> (raw)
In-Reply-To: <eb81a728-4ac3-823e-0017-54a9491f5600@enterprisedb.com>
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>
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
Attachments:
[text/x-patch] modifytable-fix.patch (1.9K, ../261fbedb-1ced-82f7-e978-60cfe17bce07@enterprisedb.com/2-modifytable-fix.patch)
download | inline diff:diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.cindex 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
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, a.lepikhov@postgrespro.ru, tomas.vondra@2ndquadrant.com
Subject: Re: POC: postgres_fdw insert batching
In-Reply-To: <261fbedb-1ced-82f7-e978-60cfe17bce07@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