Horton

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

Masahiko Sawada <sawada.mshk@gmail.com>

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

Attachments

On Sun, Mar 2, 2025 at 4:19 PM 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 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

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.