On Sun, Mar 2, 2025 at 4:19 PM Sutou Kouhei <k...@clear-code.com> wrote:
>
> Hi,
>
> In <3191030.1740932...@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 <t...@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 threshold of 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
v2-0001-Refactor-Copy-From-To-GetRoutine-to-use-pass-by-r.patch
Description: Binary data
