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

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