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

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?

I'll rebase the v35 patch set after this is fixed.

Thanks,
--
kou