Date: Mon, 03 Mar 2025 09:19:12 +0900 (JST)
Message-Id: <20250303.091912.305263537922114619.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
Subject: Re: Make COPY format extendable: Extract COPY TO format
 implementations
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: <3191030.1740932840@sss.pgh.pa.us>
References: <20250301.115009.424844407736647598.kou@clear-code.com>
	<CAEG8a3L6YCpPksTQMzjD_CvwDEhW3D_t=5md9BvvdOs5k+TA=Q@mail.gmail.com>
	<3191030.1740932840@sss.pgh.pa.us>
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Mon_Mar__3_09_19_12_2025_387)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20250303.091912.305263537922114619.kou%40clear-code.com>
Precedence: bulk

----Next_Part(Mon_Mar__3_09_19_12_2025_387)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

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

----Next_Part(Mon_Mar__3_09_19_12_2025_387)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="0001-Use-const-pointer-for-CopyFormatOptions-for-Copy-To-.patch"

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


----Next_Part(Mon_Mar__3_09_19_12_2025_387)----