Re: Make COPY format extendable: Extract COPY TO format implementations

Hi,

In <CAD21AoA1s0nzjGU9t3N_uNdg3SZeOxXyH3rQfxYFEN3Y7JrKRQ(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 20 Nov 2024 14:14:27 -0800,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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