Hi,
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on
Sun, 02 Mar 2025 11:27:20 -0500,
Tom Lane <[email protected]> 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
>From f21b48c7dd0b141c561e9c8a2c9f1d0e28aabfae Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email protected]>
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 <[email protected]>
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
