Date: Thu, 21 Nov 2024 11:55:31 +0900 (JST)
Message-Id: <20241121.115531.1600295431613712107.kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: 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: 
 <CAD21AoA1s0nzjGU9t3N_uNdg3SZeOxXyH3rQfxYFEN3Y7JrKRQ@mail.gmail.com>
References: <20241119.103115.418773618386970482.kou@clear-code.com>
	<CAD21AoByMxtzg9TExQCm2jm1oCN1DwFtNj7Cf_ewSyAwV1c7iQ@mail.gmail.com>
	<CAD21AoA1s0nzjGU9t3N_uNdg3SZeOxXyH3rQfxYFEN3Y7JrKRQ@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/20241121.115531.1600295431613712107.kou%40clear-code.com>
Precedence: bulk

Hi,

In <CAD21AoA1s0nzjGU9t3N_uNdg3SZeOxXyH3rQfxYFEN3Y7JrKRQ@mail.gmail.com>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 20 Nov 2024 14:14:27 -0800,
  Masahiko Sawada <sawada.mshk@gmail.com> wrote:

> I've extracted the changes to refactor COPY TO/FROM to use the format
> callback routines from v23 patch set, which seems to be a better patch
> split to me. Also, I've reviewed these changes and made some changes
> on top of them. The attached patches are:
> 
> 0001: make COPY TO use CopyToRoutine.
> 0002: minor changes to 0001 patch. will be fixed up.
> 0003: make COPY FROM use CopyFromRoutine.
> 0004: minor changes to 0003 patch. will be fixed up.
> 
> I've confirmed that v24 has a similar performance improvement to v23.
> Please check these extractions and minor change suggestions.

Thanks. Here are my comments:

0002:

+/* TEXT format */

"text" may be better than "TEXT". We use "text" not "TEXT"
in other places.

+static const CopyToRoutine CopyToRoutineText = {
+	.CopyToStart = CopyToTextLikeStart,
+	.CopyToOutFunc = CopyToTextLikeOutFunc,
+	.CopyToOneRow = CopyToTextOneRow,
+	.CopyToEnd = CopyToTextLikeEnd,
+};

+/* BINARY format */

"binary" may be better than "BINARY". We use "binary" not
"BINARY" in other places.

+static const CopyToRoutine CopyToRoutineBinary = {
+	.CopyToStart = CopyToBinaryStart,
+	.CopyToOutFunc = CopyToBinaryOutFunc,
+	.CopyToOneRow = CopyToBinaryOneRow,
+	.CopyToEnd = CopyToBinaryEnd,
+};

+/* Return COPY TO routines for the given option */

option ->
options

+static const CopyToRoutine *
+CopyToGetRoutine(CopyFormatOptions opts)


0003:

diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
index 99981b1579..224fda172e 100644
--- a/src/include/commands/copyapi.h
+++ b/src/include/commands/copyapi.h
@@ -56,4 +56,46 @@ typedef struct CopyToRoutine
 	void		(*CopyToEnd) (CopyToState cstate);
 } CopyToRoutine;
 
+/*
+ * API structure for a COPY FROM format implementation.	 Note this must be

Should we remove a tab character here?

0004:

diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index e77986f9a9..7f1de8a42b 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -106,31 +106,65 @@ typedef struct CopyMultiInsertInfo
 /* non-export function prototypes */
 static void ClosePipeFromProgram(CopyFromState cstate);
 
-
 /*
- * CopyFromRoutine implementations for text and CSV.
+ * built-in format-specific routines. One-row callbacks are defined in

built-in ->
Built-in

 /*
- * CopyFromTextLikeInFunc
- *
- * Assign input function data for a relation's attribute in text/CSV format.
+ * COPY FROM routines for built-in formats.
++

"+" ->
" *"

+/* TEXT format */

TEXT -> text?

+/* BINARY format */

BINARY -> binary?

+/* Return COPY FROM routines for the given option */

option ->
options

+static const CopyFromRoutine *
+CopyFromGetRoutine(CopyFormatOptions opts)

diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 0447c4df7e..5416583e94 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c

+static bool CopyFromTextLikeOneRow(CopyFromState cstate, ExprContext *econtext,
+								   Datum *values, bool *nulls, bool is_csv);

Oh, I didn't know that we don't need inline in a function
declaration.
 
@@ -1237,7 +1219,7 @@ CopyReadLine(CopyFromState cstate, bool is_csv)
 /*
  * CopyReadLineText - inner loop of CopyReadLine for text mode
  */
-static pg_attribute_always_inline bool
+static bool
 CopyReadLineText(CopyFromState cstate, bool is_csv)

Is this an intentional change?
CopyReadLineText() has "bool in_csv".


Thanks,
-- 
kou