Horton

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

Sutou Kouhei <kou@clear-code.com>

View thread View on postgr.es
From: Sutou Kouhei <kou@clear-code.com>
To: tgl@sss.pgh.pa.us
Cc: zhjwpku@gmail.com, sawada.mshk@gmail.com, michael@paquier.xyz, pgsql-hackers@postgresql.org
Date: 2025-03-03T00:19:12Z
Lists: pgsql-hackers

Attachments

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 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

Commits

  1. Refactor Copy{From|To}GetRoutine() to use pass-by-reference argument.

  2. Refactor COPY FROM to use format callback functions.

  3. Refactor COPY TO to use format callback functions.

  4. Another try to fix BF failure introduced in commit ddd5f4f54a.

  5. Revert "Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM"

  6. Improve COPY TO performance when server and client encodings match

  7. Simplify signature of CopyAttributeOutCSV() in copyto.c

  8. Revert "Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO"

  9. Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO

  10. Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM

  11. Add progress reporting of skipped tuples during COPY FROM.

  12. pgbench: Add \syncpipeline

  13. meson: Make gzip and tar optional

  14. Export the external file reader used in COPY FROM as APIs.