Obsolete comment in ExecInsert()
Hi,
While reviewing the “Fast COPY FROM based on batch insert" patch, I
noticed this comment introduced in commit b663a4136:
/*
* If a certain number of tuples have already been accumulated, or
* a tuple has come for a different relation than that for the
* accumulated tuples, perform the batch insert
*/
if (resultRelInfo->ri_NumSlots == resultRelInfo->ri_BatchSize)
{
ExecBatchInsert(mtstate, resultRelInfo,
resultRelInfo->ri_Slots,
resultRelInfo->ri_PlanSlots,
resultRelInfo->ri_NumSlots,
estate, canSetTag);
resultRelInfo->ri_NumSlots = 0;
}
I think the “or a tuple has come for a different relation than that
for the accumulated tuples" part in the comment is a leftover from an
earlier version of the patch [1]/messages/by-id/TYAPR01MB2990ECD1C68EA694DD0667E4FEE90@TYAPR01MB2990.jpnprd01.prod.outlook.com. As the code shows, we do not handle
that case anymore, so I think we should remove that part from the
comment. Attached is a patch for that.
Best regards,
Etsuro Fujita
[1]: /messages/by-id/TYAPR01MB2990ECD1C68EA694DD0667E4FEE90@TYAPR01MB2990.jpnprd01.prod.outlook.com
Attachments:
Remove-obsolete-comment.patchapplication/octet-stream; name=Remove-obsolete-comment.patchDownload
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 775960827d..4d53da56c7 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -795,9 +795,8 @@ ExecInsert(ModifyTableContext *context,
if (resultRelInfo->ri_BatchSize > 1)
{
/*
- * If a certain number of tuples have already been accumulated, or
- * a tuple has come for a different relation than that for the
- * accumulated tuples, perform the batch insert
+ * If a certain number of tuples have already been accumulated,
+ * perform the batch insert
*/
if (resultRelInfo->ri_NumSlots == resultRelInfo->ri_BatchSize)
{
Etsuro Fujita <etsuro.fujita@gmail.com> writes:
I think the “or a tuple has come for a different relation than that
for the accumulated tuples" part in the comment is a leftover from an
earlier version of the patch [1]. As the code shows, we do not handle
that case anymore, so I think we should remove that part from the
comment. Attached is a patch for that.
+1, but what remains still seems awkwardly worded. How about something
like "When we've reached the desired batch size, perform the insertion"?
regards, tom lane
Hi,
On Wed, Sep 28, 2022 at 11:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Etsuro Fujita <etsuro.fujita@gmail.com> writes:
I think the “or a tuple has come for a different relation than that
for the accumulated tuples" part in the comment is a leftover from an
earlier version of the patch [1]. As the code shows, we do not handle
that case anymore, so I think we should remove that part from the
comment. Attached is a patch for that.+1, but what remains still seems awkwardly worded. How about something
like "When we've reached the desired batch size, perform the insertion"?
+1 for that change. Pushed that way.
Thanks for reviewing!
Best regards,
Etsuro Fujita