On Sun, Mar 2, 2025 at 4:19 PM Sutou Kouhei <kou@clear-code.com> wrote:
>
> Hi,
>
> In <3191030.1740932840@sss.pgh.pa.us>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sun, 02 Mar 2025 11:27:20 -0500,
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> >> While review another thread (Emitting JSON to file using COPY TO),
> >> I found the recently committed patches on this thread pass the
> >> CopyFormatOptions struct directly rather a pointer of the struct
> >> as a function parameter of CopyToGetRoutine and CopyFromGetRoutine.
> >
> > Coverity is unhappy about that too:
> >
> > /srv/coverity/git/pgsql-git/postgresql/src/backend/commands/copyto.c: 177 in CopyToGetRoutine()
> > 171 .CopyToOneRow = CopyToBinaryOneRow,
> > 172 .CopyToEnd = CopyToBinaryEnd,
> > 173 };
> > 174
> > 175 /* Return a COPY TO routine for the given options */
> > 176 static const CopyToRoutine *
> >>>> CID 1643911: Performance inefficiencies (PASS_BY_VALUE)
> >>>> Passing parameter opts of type "CopyFormatOptions" (size 184 bytes) by value, which exceeds the low
thresholdof 128 bytes.
> > 177 CopyToGetRoutine(CopyFormatOptions opts)
> > 178 {
> > 179 if (opts.csv_mode)
> > 180 return &CopyToRoutineCSV;
> >
> > (and likewise for CopyFromGetRoutine). I realize that these
> > functions aren't called often enough for performance to be an
> > overriding concern, but it still seems like poor style.
> >
> >> Then I took a quick look at the newly rebased patch set and
> >> found Sutou has already fixed this issue.
> >
> > +1, except I'd suggest declaring the parameters as
> > "const CopyFormatOptions *opts".
>
> Thanks for pointing out this (and sorry for missing this in
> our reviews...)!
>
> How about the attached patch?
>
> I'll rebase the v35 patch set after this is fixed.
Thank you for reporting the issue and making the patch.
I agree with the fix and the patch looks good to me. I've updated the
commit message and am going to push, barring any objections.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
