On 1/21/21 2:53 AM, Amit Langote wrote:
On Thu, Jan 21, 2021 at 10:42 AM Tomas Vondra
<tomas.von...@enterprisedb.com> wrote:
On 1/21/21 2:24 AM, Amit Langote wrote:
On Thu, Jan 21, 2021 at 9:56 AM Tomas Vondra
<tomas.von...@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.
+1 in general. To avoid looping uselessly in the case of
UPDATE/DELETE where batching can't be used today, I'd suggest putting
if (operation == CMD_INSERT) around the loop.
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.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 9c36860704..a4870d621a 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2797,18 +2797,30 @@ 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).
+ *
+ * We only do this for INSERT, so that for UPDATE/DELETE the value
+ * remains set to 0.
*/
- 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;
+ if (operation == CMD_INSERT)
+ {
+ 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
