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

On Mon, Mar 3, 2025 at 8:19 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <3191030(dot)1740932840(at)sss(dot)pgh(dot)pa(dot)us>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sun, 02 Mar 2025 11:27:20 -0500,
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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?

Looking good, thanks

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

--
Regards
Junwang Zhao