Thread: 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].  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]
https://www.postgresql.org/message-id/TYAPR01MB2990ECD1C68EA694DD0667E4FEE90%40TYAPR01MB2990.jpnprd01.prod.outlook.com

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