Date: Wed, 26 Feb 2025 08:51:59 +0900 (JST)
Message-Id: <20250226.085159.1492932557196528740.kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: michael@paquier.xyz, zhjwpku@gmail.com, 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: 
 <CAD21AoBjzkL2Lv7j4teaHBZvNmKctQtH6X71kN_sj6Fm-+VvJQ@mail.gmail.com>
References: 
 <CAD21AoAni3cKToPfdShTsc0NmaJOtbJuUb=skyz3Udj7HZY7dA@mail.gmail.com>
	<20250221.114812.86438027655536928.kou@clear-code.com>
	<CAD21AoBjzkL2Lv7j4teaHBZvNmKctQtH6X71kN_sj6Fm-+VvJQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20250226.085159.1492932557196528740.kou%40clear-code.com>
Precedence: bulk

Hi,

In <CAD21AoBjzkL2Lv7j4teaHBZvNmKctQtH6X71kN_sj6Fm-+VvJQ@mail.gmail.com>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 25 Feb 2025 14:05:28 -0800,
  Masahiko Sawada <sawada.mshk@gmail.com> wrote:

> The first two patches are refactoring patches (+ small performance
> improvements). I've reviewed these patches again and attached the
> updated patches. I reorganized the function order and updated comments
> etc. I find that these patches are reasonably ready to push. Could you
> review these versions? I'm going to push them, barring objections and
> further comments.

Sure. Here are some minor comments:

0001:

Commit message:

> or CSV mode. The performance benchmark results showed ~5% performance
> gain intext or CSV mode.

intext -> in text

> --- a/src/backend/commands/copyto.c
> +++ b/src/backend/commands/copyto.c

> @@ -20,6 +20,7 @@

>  #include "commands/copy.h"
> +#include "commands/copyapi.h"

We can remove '#include "commands/copy.h"' because it's
included in copyapi.h. (0002 does it.)

> @@ -254,6 +502,35 @@ CopySendEndOfRow(CopyToState cstate)

> +/*
> + * Wrapper function of CopySendEndOfRow for text and CSV formats. Sends the
> + * the line termination and do common appropriate things for the end of row.
> + */

Sends the the line ->
Sends the line

> --- /dev/null
> +++ b/src/include/commands/copyapi.h

> +	/* End a COPY TO. This callback is called once at the end of COPY FROM */

The last "." is missing: ... COPY FROM.

0002:

Commit message:

> This change is a preliminary step towards making the COPY TO command
> extensible in terms of output formats.

COPY TO -> COPY FROM

> --- a/src/backend/commands/copyfromparse.c
> +++ b/src/backend/commands/copyfromparse.c

> @@ -1087,7 +1132,7 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,

>  static bool
> -CopyReadLine(CopyFromState cstate)
> +CopyReadLine(CopyFromState cstate, bool is_csv)

> @@ -1163,7 +1208,7 @@ CopyReadLine(CopyFromState cstate)

>  static bool
> -CopyReadLineText(CopyFromState cstate)
> +CopyReadLineText(CopyFromState cstate, bool is_csv)

We may want to add a comment why we don't use "inline" nor
"pg_attribute_always_inline" here:

https://www.postgresql.org/message-id/CAD21AoBNfKDbJnu-zONNpG820ZXYC0fuTSLrJ-UdRqU4qp2wog%40mail.gmail.com

> Yes, I'm not sure it's really necessary to make it inline since the
> benchmark results don't show much difference. Probably this is because
> the function has 'is_csv' in some 'if' branches but the compiler
> cannot optimize out the whole 'if' branches as most 'if' branches
> check 'is_csv' and other variables.

Or we can add "inline" not "pg_attribute_always_inline" here
as a hint for compiler.

> --- a/src/include/commands/copyapi.h
> +++ b/src/include/commands/copyapi.h

> @@ -52,4 +52,50 @@ typedef struct CopyToRoutine

> +	/* End a COPY FROM. This callback is called once at the end of COPY FROM */

The last "." is missing: ... COPY FROM.


I think that these patches are ready to push too.

Thanks,
-- 
kou