Re: Make COPY format extendable: Extract COPY TO format implementations

On Mon, Mar 3, 2025 at 8:19 AM 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?

Looking good, thanks

>
> I'll rebase the v35 patch set after this is fixed.
>
>
> Thanks,
> --
> kou



--
Regards
Junwang Zhao