Thread: Obsolete comment in ExecInsert()

Obsolete comment in ExecInsert()

From
Etsuro Fujita
Date:
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

Attachment

Re: Obsolete comment in ExecInsert()

From
Tom Lane
Date:
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



Re: Obsolete comment in ExecInsert()

From
Etsuro Fujita
Date:
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