Re: Make COPY format extendable: Extract COPY TO format implementations - Mailing list pgsql-hackers

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
of128 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
From f21b48c7dd0b141c561e9c8a2c9f1d0e28aabfae Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <kou@clear-code.com>
Date: Mon, 3 Mar 2025 09:13:37 +0900
Subject: [PATCH] Use const pointer for CopyFormatOptions for
 Copy{To,From}GetRoutine()

We don't need to copy CopyFormatOptions here.

Reported-by: Junwang Zhao <zhjwpku@gmail.com>
Discussion: https://postgr.es/m/CAEG8a3L6YCpPksTQMzjD_CvwDEhW3D_t=5md9BvvdOs5k+TA=Q@mail.gmail.com
---
 src/backend/commands/copyfrom.c | 8 ++++----
 src/backend/commands/copyto.c   | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 198cee2bc48..bcf66f0adf8 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -153,11 +153,11 @@ static const CopyFromRoutine CopyFromRoutineBinary = {
 
 /* Return a COPY FROM routine for the given options */
 static const CopyFromRoutine *
-CopyFromGetRoutine(CopyFormatOptions opts)
+CopyFromGetRoutine(const CopyFormatOptions *opts)
 {
-    if (opts.csv_mode)
+    if (opts->csv_mode)
         return &CopyFromRoutineCSV;
-    else if (opts.binary)
+    else if (opts->binary)
         return &CopyFromRoutineBinary;
 
     /* default is text */
@@ -1574,7 +1574,7 @@ BeginCopyFrom(ParseState *pstate,
     ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */ , options);
 
     /* Set the format routine */
-    cstate->routine = CopyFromGetRoutine(cstate->opts);
+    cstate->routine = CopyFromGetRoutine(&cstate->opts);
 
     /* Process the target relation */
     cstate->rel = rel;
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 721d29f8e53..84a3f3879a8 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -174,11 +174,11 @@ static const CopyToRoutine CopyToRoutineBinary = {
 
 /* Return a COPY TO routine for the given options */
 static const CopyToRoutine *
-CopyToGetRoutine(CopyFormatOptions opts)
+CopyToGetRoutine(const CopyFormatOptions *opts)
 {
-    if (opts.csv_mode)
+    if (opts->csv_mode)
         return &CopyToRoutineCSV;
-    else if (opts.binary)
+    else if (opts->binary)
         return &CopyToRoutineBinary;
 
     /* default is text */
@@ -700,7 +700,7 @@ BeginCopyTo(ParseState *pstate,
     ProcessCopyOptions(pstate, &cstate->opts, false /* is_from */ , options);
 
     /* Set format routine */
-    cstate->routine = CopyToGetRoutine(cstate->opts);
+    cstate->routine = CopyToGetRoutine(&cstate->opts);
 
     /* Process the source/target relation or query */
     if (rel)
-- 
2.47.2