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

From: Masahiko Sawada <[email-masked]>
Date: Mon, 3 Mar 2025 11:06:39 -0800

On Sun, Mar 2, 2025 at 4:19 PM Sutou Kouhei <[email-masked]> wrote:
>
> Hi,
>
> In <[email-masked]>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sun, 02 Mar 2025 11:27:20 -0500,
>   Tom Lane <[email-masked]> 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.

Thank you for reporting the issue and making the patch.

I agree with the fix and the patch looks good to me. I've updated the
commit message and am going to push, barring any objections.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

---

From: Sutou Kouhei <[email-masked]>
Date: Wed, 05 Mar 2025 09:06:08 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 3 Mar 2025 11:06:39 -0800,

> commit message and am going to push, barring any objections.

Thanks!

I've rebased the patch set. Here is a summary again:

> 0001-0003 are for COPY TO and 0004-0007 are for COPY FROM.
>
> For COPY TO:
>
> 0001: Add support for adding custom COPY TO format. This
> uses tablesample like handler approach. We've discussed
> other approaches such as USING+CREATE XXX approach but it
> seems that other approaches are overkill for this case.
>
> See also: https://www.postgresql.org/message-id/flat/d838025aceeb19c9ff1db702fa55cabf%40postgrespro.ru#caca2799effc859f82f40ee8bec531d8
>
> 0002: Export CopyToStateData to implement custom COPY TO
> format as extension.
>
> 0003: Export a function and add a private space to
> CopyToStateData to implement custom COPY TO format as
> extension.
>
> We may want to squash 0002 and 0003 but splitting them will
> be easy to review. Because 0002 just moves existing codes
> (with some rename) and 0003 just adds some codes. If we
> squash 0002 and 0003, moving and adding are mixed.
>
> For COPY FROM:
>
> 0004: This is COPY FROM version of 0001.
>
> 0005: 0002 has COPY_ prefix -> COPY_DEST_ prefix change for
> enum CopyDest. This is similar change for enum CopySource.
>
> 0006: This is COPY FROM version of 0003.
>
> 0007: This is for easy to implement "ON_ERROR stop" and
> "LOG_VERBOSITY verbose" in extension.
>
> We may want to squash 0005-0007 like for 0002-0003.

Thanks,
-- 
kou

>From 479c601915b30e4f67e5ed047c6fbf3e35702ec6 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email-masked]>
Date: Mon, 25 Nov 2024 12:19:15 +0900
Subject: [PATCH v36 1/7] Add support for adding custom COPY TO format

This uses the handler approach like tablesample. The approach creates
an internal function that returns an internal struct. In this case,
a COPY TO handler returns a CopyToRoutine.

This also add a test module for custom COPY TO handler.
---
 src/backend/commands/copy.c                   | 79 ++++++++++++++++---
 src/backend/commands/copyto.c                 | 28 ++++++-
 src/backend/nodes/Makefile                    |  1 +
 src/backend/nodes/gen_node_support.pl         |  2 +
 src/backend/utils/adt/pseudotypes.c           |  1 +
 src/include/catalog/pg_proc.dat               |  6 ++
 src/include/catalog/pg_type.dat               |  6 ++
 src/include/commands/copy.h                   |  1 +
 src/include/commands/copyapi.h                |  2 +
 src/include/nodes/meson.build                 |  1 +
 src/test/modules/Makefile                     |  1 +
 src/test/modules/meson.build                  |  1 +
 src/test/modules/test_copy_format/.gitignore  |  4 +
 src/test/modules/test_copy_format/Makefile    | 23 ++++++
 .../expected/test_copy_format.out             | 17 ++++
 src/test/modules/test_copy_format/meson.build | 33 ++++++++
 .../test_copy_format/sql/test_copy_format.sql |  5 ++
 .../test_copy_format--1.0.sql                 |  8 ++
 .../test_copy_format/test_copy_format.c       | 63 +++++++++++++++
 .../test_copy_format/test_copy_format.control |  4 +
 20 files changed, 269 insertions(+), 17 deletions(-)
 mode change 100644 => 100755 src/backend/nodes/gen_node_support.pl
 create mode 100644 src/test/modules/test_copy_format/.gitignore
 create mode 100644 src/test/modules/test_copy_format/Makefile
 create mode 100644 src/test/modules/test_copy_format/expected/test_copy_format.out
 create mode 100644 src/test/modules/test_copy_format/meson.build
 create mode 100644 src/test/modules/test_copy_format/sql/test_copy_format.sql
 create mode 100644 src/test/modules/test_copy_format/test_copy_format--1.0.sql
 create mode 100644 src/test/modules/test_copy_format/test_copy_format.c
 create mode 100644 src/test/modules/test_copy_format/test_copy_format.control

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index cfca9d9dc29..8d94bc313eb 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -32,6 +32,7 @@
 #include "parser/parse_coerce.h"
 #include "parser/parse_collate.h"
 #include "parser/parse_expr.h"
+#include "parser/parse_func.h"
 #include "parser/parse_relation.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
@@ -476,6 +477,70 @@ defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate)
 	return COPY_LOG_VERBOSITY_DEFAULT;	/* keep compiler quiet */
 }
 
+/*
+ * Process the "format" option.
+ *
+ * This function checks whether the option value is a built-in format such as
+ * "text" and "csv" or not. If the option value isn't a built-in format, this
+ * function finds a COPY format handler that returns a CopyToRoutine (for
+ * is_from == false). If no COPY format handler is found, this function
+ * reports an error.
+ */
+static void
+ProcessCopyOptionFormat(ParseState *pstate,
+						CopyFormatOptions *opts_out,
+						bool is_from,
+						DefElem *defel)
+{
+	char	   *format;
+	Oid			funcargtypes[1];
+	Oid			handlerOid = InvalidOid;
+
+	format = defGetString(defel);
+
+	opts_out->csv_mode = false;
+	opts_out->binary = false;
+	/* built-in formats */
+	if (strcmp(format, "text") == 0)
+	{
+		/* "csv_mode == false && binary == false" means "text" */
+		return;
+	}
+	else if (strcmp(format, "csv") == 0)
+	{
+		opts_out->csv_mode = true;
+		return;
+	}
+	else if (strcmp(format, "binary") == 0)
+	{
+		opts_out->binary = true;
+		return;
+	}
+
+	/* custom format */
+	if (!is_from)
+	{
+		funcargtypes[0] = INTERNALOID;
+		handlerOid = LookupFuncName(list_make1(makeString(format)), 1,
+									funcargtypes, true);
+	}
+	if (!OidIsValid(handlerOid))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("COPY format \"%s\" not recognized", format),
+				 parser_errposition(pstate, defel->location)));
+
+	/* check that handler has correct return type */
+	if (get_func_rettype(handlerOid) != COPY_HANDLEROID)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("function %s must return type %s",
+						format, "copy_handler"),
+				 parser_errposition(pstate, defel->location)));
+
+	opts_out->handler = handlerOid;
+}
+
 /*
  * Process the statement option list for COPY.
  *
@@ -519,22 +584,10 @@ ProcessCopyOptions(ParseState *pstate,
 
 		if (strcmp(defel->defname, "format") == 0)
 		{
-			char	   *fmt = defGetString(defel);
-
 			if (format_specified)
 				errorConflictingDefElem(defel, pstate);
 			format_specified = true;
-			if (strcmp(fmt, "text") == 0)
-				 /* default format */ ;
-			else if (strcmp(fmt, "csv") == 0)
-				opts_out->csv_mode = true;
-			else if (strcmp(fmt, "binary") == 0)
-				opts_out->binary = true;
-			else
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("COPY format \"%s\" not recognized", fmt),
-						 parser_errposition(pstate, defel->location)));
+			ProcessCopyOptionFormat(pstate, opts_out, is_from, defel);
 		}
 		else if (strcmp(defel->defname, "freeze") == 0)
 		{
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 84a3f3879a8..fce8501dc30 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -150,6 +150,7 @@ static void CopySendInt16(CopyToState cstate, int16 val);
 
 /* text format */
 static const CopyToRoutine CopyToRoutineText = {
+	.type = T_CopyToRoutine,
 	.CopyToStart = CopyToTextLikeStart,
 	.CopyToOutFunc = CopyToTextLikeOutFunc,
 	.CopyToOneRow = CopyToTextOneRow,
@@ -158,6 +159,7 @@ static const CopyToRoutine CopyToRoutineText = {
 
 /* CSV format */
 static const CopyToRoutine CopyToRoutineCSV = {
+	.type = T_CopyToRoutine,
 	.CopyToStart = CopyToTextLikeStart,
 	.CopyToOutFunc = CopyToTextLikeOutFunc,
 	.CopyToOneRow = CopyToCSVOneRow,
@@ -166,6 +168,7 @@ static const CopyToRoutine CopyToRoutineCSV = {
 
 /* binary format */
 static const CopyToRoutine CopyToRoutineBinary = {
+	.type = T_CopyToRoutine,
 	.CopyToStart = CopyToBinaryStart,
 	.CopyToOutFunc = CopyToBinaryOutFunc,
 	.CopyToOneRow = CopyToBinaryOneRow,
@@ -176,13 +179,30 @@ static const CopyToRoutine CopyToRoutineBinary = {
 static const CopyToRoutine *
 CopyToGetRoutine(const CopyFormatOptions *opts)
 {
-	if (opts->csv_mode)
+	if (OidIsValid(opts->handler))
+	{
+		Datum		datum;
+		Node	   *routine;
+
+		datum = OidFunctionCall1(opts->handler, BoolGetDatum(false));
+		routine = (Node *) DatumGetPointer(datum);
+		if (routine == NULL || !IsA(routine, CopyToRoutine))
+			ereport(
+					ERROR,
+					(errcode(
+							 ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("COPY handler function "
+							"%u did not return "
+							"CopyToRoutine struct",
+							opts->handler)));
+		return castNode(CopyToRoutine, routine);
+	}
+	else if (opts->csv_mode)
 		return &CopyToRoutineCSV;
 	else if (opts->binary)
 		return &CopyToRoutineBinary;
-
-	/* default is text */
-	return &CopyToRoutineText;
+	else
+		return &CopyToRoutineText;
 }
 
 /* Implementation of the start callback for text and CSV formats */
diff --git a/src/backend/nodes/Makefile b/src/backend/nodes/Makefile
index 77ddb9ca53f..dc6c1087361 100644
--- a/src/backend/nodes/Makefile
+++ b/src/backend/nodes/Makefile
@@ -50,6 +50,7 @@ node_headers = \
 	access/sdir.h \
 	access/tableam.h \
 	access/tsmapi.h \
+	commands/copyapi.h \
 	commands/event_trigger.h \
 	commands/trigger.h \
 	executor/tuptable.h \
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
old mode 100644
new mode 100755
index 1a657f7e0ae..fb90635a245
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -62,6 +62,7 @@ my @all_input_files = qw(
   access/sdir.h
   access/tableam.h
   access/tsmapi.h
+  commands/copyapi.h
   commands/event_trigger.h
   commands/trigger.h
   executor/tuptable.h
@@ -86,6 +87,7 @@ my @nodetag_only_files = qw(
   access/sdir.h
   access/tableam.h
   access/tsmapi.h
+  commands/copyapi.h
   commands/event_trigger.h
   commands/trigger.h
   executor/tuptable.h
diff --git a/src/backend/utils/adt/pseudotypes.c b/src/backend/utils/adt/pseudotypes.c
index 317a1f2b282..f2ebc21ca56 100644
--- a/src/backend/utils/adt/pseudotypes.c
+++ b/src/backend/utils/adt/pseudotypes.c
@@ -370,6 +370,7 @@ PSEUDOTYPE_DUMMY_IO_FUNCS(fdw_handler);
 PSEUDOTYPE_DUMMY_IO_FUNCS(table_am_handler);
 PSEUDOTYPE_DUMMY_IO_FUNCS(index_am_handler);
 PSEUDOTYPE_DUMMY_IO_FUNCS(tsm_handler);
+PSEUDOTYPE_DUMMY_IO_FUNCS(copy_handler);
 PSEUDOTYPE_DUMMY_IO_FUNCS(internal);
 PSEUDOTYPE_DUMMY_IO_FUNCS(anyelement);
 PSEUDOTYPE_DUMMY_IO_FUNCS(anynonarray);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index cd9422d0bac..9e7737168c4 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7809,6 +7809,12 @@
 { oid => '3312', descr => 'I/O',
   proname => 'tsm_handler_out', prorettype => 'cstring',
   proargtypes => 'tsm_handler', prosrc => 'tsm_handler_out' },
+{ oid => '8753', descr => 'I/O',
+  proname => 'copy_handler_in', proisstrict => 'f', prorettype => 'copy_handler',
+  proargtypes => 'cstring', prosrc => 'copy_handler_in' },
+{ oid => '8754', descr => 'I/O',
+  proname => 'copy_handler_out', prorettype => 'cstring',
+  proargtypes => 'copy_handler', prosrc => 'copy_handler_out' },
 { oid => '267', descr => 'I/O',
   proname => 'table_am_handler_in', proisstrict => 'f',
   prorettype => 'table_am_handler', proargtypes => 'cstring',
diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat
index 6dca77e0a22..340e0cd0a8d 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -633,6 +633,12 @@
   typcategory => 'P', typinput => 'tsm_handler_in',
   typoutput => 'tsm_handler_out', typreceive => '-', typsend => '-',
   typalign => 'i' },
+{ oid => '8752',
+  descr => 'pseudo-type for the result of a copy to method function',
+  typname => 'copy_handler', typlen => '4', typbyval => 't', typtype => 'p',
+  typcategory => 'P', typinput => 'copy_handler_in',
+  typoutput => 'copy_handler_out', typreceive => '-', typsend => '-',
+  typalign => 'i' },
 { oid => '269',
   descr => 'pseudo-type for the result of a table AM handler function',
   typname => 'table_am_handler', typlen => '4', typbyval => 't', typtype => 'p',
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 06dfdfef721..332628d67cc 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -87,6 +87,7 @@ typedef struct CopyFormatOptions
 	CopyLogVerbosityChoice log_verbosity;	/* verbosity of logged messages */
 	int64		reject_limit;	/* maximum tolerable number of errors */
 	List	   *convert_select; /* list of column names (can be NIL) */
+	Oid			handler;		/* handler function for custom format routine */
 } CopyFormatOptions;
 
 /* These are private in commands/copy[from|to].c */
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
index 2a2d2f9876b..4f4ffabf882 100644
--- a/src/include/commands/copyapi.h
+++ b/src/include/commands/copyapi.h
@@ -22,6 +22,8 @@
  */
 typedef struct CopyToRoutine
 {
+	NodeTag		type;
+
 	/*
 	 * Set output function information. This callback is called once at the
 	 * beginning of COPY TO.
diff --git a/src/include/nodes/meson.build b/src/include/nodes/meson.build
index d1ca24dd32f..96e70e7f38b 100644
--- a/src/include/nodes/meson.build
+++ b/src/include/nodes/meson.build
@@ -12,6 +12,7 @@ node_support_input_i = [
   'access/sdir.h',
   'access/tableam.h',
   'access/tsmapi.h',
+  'commands/copyapi.h',
   'commands/event_trigger.h',
   'commands/trigger.h',
   'executor/tuptable.h',
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 4e4be3fa511..c9da440eed0 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -16,6 +16,7 @@ SUBDIRS = \
 		  spgist_name_ops \
 		  test_bloomfilter \
 		  test_copy_callbacks \
+		  test_copy_format \
 		  test_custom_rmgrs \
 		  test_ddl_deparse \
 		  test_dsa \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 2b057451473..d33bbbd4092 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -15,6 +15,7 @@ subdir('spgist_name_ops')
 subdir('ssl_passphrase_callback')
 subdir('test_bloomfilter')
 subdir('test_copy_callbacks')
+subdir('test_copy_format')
 subdir('test_custom_rmgrs')
 subdir('test_ddl_deparse')
 subdir('test_dsa')
diff --git a/src/test/modules/test_copy_format/.gitignore b/src/test/modules/test_copy_format/.gitignore
new file mode 100644
index 00000000000..5dcb3ff9723
--- /dev/null
+++ b/src/test/modules/test_copy_format/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_copy_format/Makefile b/src/test/modules/test_copy_format/Makefile
new file mode 100644
index 00000000000..8497f91624d
--- /dev/null
+++ b/src/test/modules/test_copy_format/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_copy_format/Makefile
+
+MODULE_big = test_copy_format
+OBJS = \
+	$(WIN32RES) \
+	test_copy_format.o
+PGFILEDESC = "test_copy_format - test custom COPY FORMAT"
+
+EXTENSION = test_copy_format
+DATA = test_copy_format--1.0.sql
+
+REGRESS = test_copy_format
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_copy_format
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_copy_format/expected/test_copy_format.out b/src/test/modules/test_copy_format/expected/test_copy_format.out
new file mode 100644
index 00000000000..adfe7d1572a
--- /dev/null
+++ b/src/test/modules/test_copy_format/expected/test_copy_format.out
@@ -0,0 +1,17 @@
+CREATE EXTENSION test_copy_format;
+CREATE TABLE public.test (a smallint, b integer, c bigint);
+INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789);
+COPY public.test FROM stdin WITH (FORMAT 'test_copy_format');
+ERROR:  COPY format "test_copy_format" not recognized
+LINE 1: COPY public.test FROM stdin WITH (FORMAT 'test_copy_format')...
+                                          ^
+COPY public.test TO stdout WITH (FORMAT 'test_copy_format');
+NOTICE:  test_copy_format: is_from=false
+NOTICE:  CopyToOutFunc: atttypid=21
+NOTICE:  CopyToOutFunc: atttypid=23
+NOTICE:  CopyToOutFunc: atttypid=20
+NOTICE:  CopyToStart: natts=3
+NOTICE:  CopyToOneRow: tts_nvalid=3
+NOTICE:  CopyToOneRow: tts_nvalid=3
+NOTICE:  CopyToOneRow: tts_nvalid=3
+NOTICE:  CopyToEnd
diff --git a/src/test/modules/test_copy_format/meson.build b/src/test/modules/test_copy_format/meson.build
new file mode 100644
index 00000000000..a45a2e0a039
--- /dev/null
+++ b/src/test/modules/test_copy_format/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) 2025, PostgreSQL Global Development Group
+
+test_copy_format_sources = files(
+  'test_copy_format.c',
+)
+
+if host_system == 'windows'
+  test_copy_format_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'test_copy_format',
+    '--FILEDESC', 'test_copy_format - test custom COPY FORMAT',])
+endif
+
+test_copy_format = shared_module('test_copy_format',
+  test_copy_format_sources,
+  kwargs: pg_test_mod_args,
+)
+test_install_libs += test_copy_format
+
+test_install_data += files(
+  'test_copy_format.control',
+  'test_copy_format--1.0.sql',
+)
+
+tests += {
+  'name': 'test_copy_format',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'regress': {
+    'sql': [
+      'test_copy_format',
+    ],
+  },
+}
diff --git a/src/test/modules/test_copy_format/sql/test_copy_format.sql b/src/test/modules/test_copy_format/sql/test_copy_format.sql
new file mode 100644
index 00000000000..810b3d8cedc
--- /dev/null
+++ b/src/test/modules/test_copy_format/sql/test_copy_format.sql
@@ -0,0 +1,5 @@
+CREATE EXTENSION test_copy_format;
+CREATE TABLE public.test (a smallint, b integer, c bigint);
+INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789);
+COPY public.test FROM stdin WITH (FORMAT 'test_copy_format');
+COPY public.test TO stdout WITH (FORMAT 'test_copy_format');
diff --git a/src/test/modules/test_copy_format/test_copy_format--1.0.sql b/src/test/modules/test_copy_format/test_copy_format--1.0.sql
new file mode 100644
index 00000000000..d24ea03ce99
--- /dev/null
+++ b/src/test/modules/test_copy_format/test_copy_format--1.0.sql
@@ -0,0 +1,8 @@
+/* src/test/modules/test_copy_format/test_copy_format--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_copy_format" to load this file. \quit
+
+CREATE FUNCTION test_copy_format(internal)
+	RETURNS copy_handler
+	AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_copy_format/test_copy_format.c b/src/test/modules/test_copy_format/test_copy_format.c
new file mode 100644
index 00000000000..b42d472d851
--- /dev/null
+++ b/src/test/modules/test_copy_format/test_copy_format.c
@@ -0,0 +1,63 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_copy_format.c
+ *		Code for testing custom COPY format.
+ *
+ * Portions Copyright (c) 2025, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_copy_format/test_copy_format.c
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "commands/copyapi.h"
+#include "commands/defrem.h"
+
+PG_MODULE_MAGIC;
+
+static void
+CopyToOutFunc(CopyToState cstate, Oid atttypid, FmgrInfo *finfo)
+{
+	ereport(NOTICE, (errmsg("CopyToOutFunc: atttypid=%d", atttypid)));
+}
+
+static void
+CopyToStart(CopyToState cstate, TupleDesc tupDesc)
+{
+	ereport(NOTICE, (errmsg("CopyToStart: natts=%d", tupDesc->natts)));
+}
+
+static void
+CopyToOneRow(CopyToState cstate, TupleTableSlot *slot)
+{
+	ereport(NOTICE, (errmsg("CopyToOneRow: tts_nvalid=%u", slot->tts_nvalid)));
+}
+
+static void
+CopyToEnd(CopyToState cstate)
+{
+	ereport(NOTICE, (errmsg("CopyToEnd")));
+}
+
+static const CopyToRoutine CopyToRoutineTestCopyFormat = {
+	.type = T_CopyToRoutine,
+	.CopyToOutFunc = CopyToOutFunc,
+	.CopyToStart = CopyToStart,
+	.CopyToOneRow = CopyToOneRow,
+	.CopyToEnd = CopyToEnd,
+};
+
+PG_FUNCTION_INFO_V1(test_copy_format);
+Datum
+test_copy_format(PG_FUNCTION_ARGS)
+{
+	bool		is_from = PG_GETARG_BOOL(0);
+
+	ereport(NOTICE,
+			(errmsg("test_copy_format: is_from=%s", is_from ? "true" : "false")));
+
+	PG_RETURN_POINTER(&CopyToRoutineTestCopyFormat);
+}
diff --git a/src/test/modules/test_copy_format/test_copy_format.control b/src/test/modules/test_copy_format/test_copy_format.control
new file mode 100644
index 00000000000..f05a6362358
--- /dev/null
+++ b/src/test/modules/test_copy_format/test_copy_format.control
@@ -0,0 +1,4 @@
+comment = 'Test code for custom COPY format'
+default_version = '1.0'
+module_pathname = '$libdir/test_copy_format'
+relocatable = true
-- 
2.47.2

>From 8768ecf89603767a3a3f9f96d569e5bc8c8f0c81 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email-masked]>
Date: Mon, 25 Nov 2024 13:58:33 +0900
Subject: [PATCH v36 2/7] Export CopyToStateData as private data

It's for custom COPY TO format handlers implemented as extension.

This just moves codes. This doesn't change codes except CopyDest enum
values. CopyDest/CopyFrom enum values such as COPY_FILE are conflicted
each other. So COPY_DEST_ prefix instead of COPY_ prefix is used for
CopyDest enum values. For example, COPY_FILE in CopyDest is renamed to
COPY_DEST_FILE.

Note that this isn't enough to implement custom COPY TO format
handlers as extension. We'll do the followings in a subsequent commit:

1. Add an opaque space for custom COPY TO format handler
2. Export CopySendEndOfRow() to flush buffer
---
 src/backend/commands/copyto.c          | 78 +++---------------------
 src/include/commands/copy.h            |  2 +-
 src/include/commands/copyto_internal.h | 83 ++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 70 deletions(-)
 create mode 100644 src/include/commands/copyto_internal.h

diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index fce8501dc30..99c2f2dd699 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -20,6 +20,7 @@
 
 #include "access/tableam.h"
 #include "commands/copyapi.h"
+#include "commands/copyto_internal.h"
 #include "commands/progress.h"
 #include "executor/execdesc.h"
 #include "executor/executor.h"
@@ -36,67 +37,6 @@
 #include "utils/rel.h"
 #include "utils/snapmgr.h"
 
-/*
- * Represents the different dest cases we need to worry about at
- * the bottom level
- */
-typedef enum CopyDest
-{
-	COPY_FILE,					/* to file (or a piped program) */
-	COPY_FRONTEND,				/* to frontend */
-	COPY_CALLBACK,				/* to callback function */
-} CopyDest;
-
-/*
- * This struct contains all the state variables used throughout a COPY TO
- * operation.
- *
- * Multi-byte encodings: all supported client-side encodings encode multi-byte
- * characters by having the first byte's high bit set. Subsequent bytes of the
- * character can have the high bit not set. When scanning data in such an
- * encoding to look for a match to a single-byte (ie ASCII) character, we must
- * use the full pg_encoding_mblen() machinery to skip over multibyte
- * characters, else we might find a false match to a trailing byte. In
- * supported server encodings, there is no possibility of a false match, and
- * it's faster to make useless comparisons to trailing bytes than it is to
- * invoke pg_encoding_mblen() to skip over them. encoding_embeds_ascii is true
- * when we have to do it the hard way.
- */
-typedef struct CopyToStateData
-{
-	/* format-specific routines */
-	const CopyToRoutine *routine;
-
-	/* low-level state data */
-	CopyDest	copy_dest;		/* type of copy source/destination */
-	FILE	   *copy_file;		/* used if copy_dest == COPY_FILE */
-	StringInfo	fe_msgbuf;		/* used for all dests during COPY TO */
-
-	int			file_encoding;	/* file or remote side's character encoding */
-	bool		need_transcoding;	/* file encoding diff from server? */
-	bool		encoding_embeds_ascii;	/* ASCII can be non-first byte? */
-
-	/* parameters from the COPY command */
-	Relation	rel;			/* relation to copy to */
-	QueryDesc  *queryDesc;		/* executable query to copy from */
-	List	   *attnumlist;		/* integer list of attnums to copy */
-	char	   *filename;		/* filename, or NULL for STDOUT */
-	bool		is_program;		/* is 'filename' a program to popen? */
-	copy_data_dest_cb data_dest_cb; /* function for writing data */
-
-	CopyFormatOptions opts;
-	Node	   *whereClause;	/* WHERE condition (or NULL) */
-
-	/*
-	 * Working state
-	 */
-	MemoryContext copycontext;	/* per-copy execution context */
-
-	FmgrInfo   *out_functions;	/* lookup info for output functions */
-	MemoryContext rowcontext;	/* per-row evaluation context */
-	uint64		bytes_processed;	/* number of bytes processed so far */
-} CopyToStateData;
-
 /* DestReceiver for COPY (query) TO */
 typedef struct
 {
@@ -421,7 +361,7 @@ SendCopyBegin(CopyToState cstate)
 	for (i = 0; i < natts; i++)
 		pq_sendint16(&buf, format); /* per-column formats */
 	pq_endmessage(&buf);
-	cstate->copy_dest = COPY_FRONTEND;
+	cstate->copy_dest = COPY_DEST_FRONTEND;
 }
 
 static void
@@ -468,7 +408,7 @@ CopySendEndOfRow(CopyToState cstate)
 
 	switch (cstate->copy_dest)
 	{
-		case COPY_FILE:
+		case COPY_DEST_FILE:
 			if (fwrite(fe_msgbuf->data, fe_msgbuf->len, 1,
 					   cstate->copy_file) != 1 ||
 				ferror(cstate->copy_file))
@@ -502,11 +442,11 @@ CopySendEndOfRow(CopyToState cstate)
 							 errmsg("could not write to COPY file: %m")));
 			}
 			break;
-		case COPY_FRONTEND:
+		case COPY_DEST_FRONTEND:
 			/* Dump the accumulated row as one CopyData message */
 			(void) pq_putmessage(PqMsg_CopyData, fe_msgbuf->data, fe_msgbuf->len);
 			break;
-		case COPY_CALLBACK:
+		case COPY_DEST_CALLBACK:
 			cstate->data_dest_cb(fe_msgbuf->data, fe_msgbuf->len);
 			break;
 	}
@@ -527,7 +467,7 @@ CopySendTextLikeEndOfRow(CopyToState cstate)
 {
 	switch (cstate->copy_dest)
 	{
-		case COPY_FILE:
+		case COPY_DEST_FILE:
 			/* Default line termination depends on platform */
 #ifndef WIN32
 			CopySendChar(cstate, '\n');
@@ -535,7 +475,7 @@ CopySendTextLikeEndOfRow(CopyToState cstate)
 			CopySendString(cstate, "\r\n");
 #endif
 			break;
-		case COPY_FRONTEND:
+		case COPY_DEST_FRONTEND:
 			/* The FE/BE protocol uses \n as newline for all platforms */
 			CopySendChar(cstate, '\n');
 			break;
@@ -920,12 +860,12 @@ BeginCopyTo(ParseState *pstate,
 	/* See Multibyte encoding comment above */
 	cstate->encoding_embeds_ascii = PG_ENCODING_IS_CLIENT_ONLY(cstate->file_encoding);
 
-	cstate->copy_dest = COPY_FILE;	/* default */
+	cstate->copy_dest = COPY_DEST_FILE; /* default */
 
 	if (data_dest_cb)
 	{
 		progress_vals[1] = PROGRESS_COPY_TYPE_CALLBACK;
-		cstate->copy_dest = COPY_CALLBACK;
+		cstate->copy_dest = COPY_DEST_CALLBACK;
 		cstate->data_dest_cb = data_dest_cb;
 	}
 	else if (pipe)
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 332628d67cc..6df1f8a3b9b 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -90,7 +90,7 @@ typedef struct CopyFormatOptions
 	Oid			handler;		/* handler function for custom format routine */
 } CopyFormatOptions;
 
-/* These are private in commands/copy[from|to].c */
+/* These are private in commands/copy[from|to]_internal.h */
 typedef struct CopyFromStateData *CopyFromState;
 typedef struct CopyToStateData *CopyToState;
 
diff --git a/src/include/commands/copyto_internal.h b/src/include/commands/copyto_internal.h
new file mode 100644
index 00000000000..1b58b36c0a3
--- /dev/null
+++ b/src/include/commands/copyto_internal.h
@@ -0,0 +1,83 @@
+/*-------------------------------------------------------------------------
+ *
+ * copyto_internal.h
+ *	  Internal definitions for COPY TO command.
+ *
+ *
+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/commands/copyto_internal.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef COPYTO_INTERNAL_H
+#define COPYTO_INTERNAL_H
+
+#include "commands/copy.h"
+#include "executor/execdesc.h"
+#include "executor/tuptable.h"
+#include "nodes/execnodes.h"
+
+/*
+ * Represents the different dest cases we need to worry about at
+ * the bottom level
+ */
+typedef enum CopyDest
+{
+	COPY_DEST_FILE,				/* to file (or a piped program) */
+	COPY_DEST_FRONTEND,			/* to frontend */
+	COPY_DEST_CALLBACK,			/* to callback function */
+} CopyDest;
+
+/*
+ * This struct contains all the state variables used throughout a COPY TO
+ * operation.
+ *
+ * Multi-byte encodings: all supported client-side encodings encode multi-byte
+ * characters by having the first byte's high bit set. Subsequent bytes of the
+ * character can have the high bit not set. When scanning data in such an
+ * encoding to look for a match to a single-byte (ie ASCII) character, we must
+ * use the full pg_encoding_mblen() machinery to skip over multibyte
+ * characters, else we might find a false match to a trailing byte. In
+ * supported server encodings, there is no possibility of a false match, and
+ * it's faster to make useless comparisons to trailing bytes than it is to
+ * invoke pg_encoding_mblen() to skip over them. encoding_embeds_ascii is true
+ * when we have to do it the hard way.
+ */
+typedef struct CopyToStateData
+{
+	/* format-specific routines */
+	const struct CopyToRoutine *routine;
+
+	/* low-level state data */
+	CopyDest	copy_dest;		/* type of copy source/destination */
+	FILE	   *copy_file;		/* used if copy_dest == COPY_FILE */
+	StringInfo	fe_msgbuf;		/* used for all dests during COPY TO */
+
+	int			file_encoding;	/* file or remote side's character encoding */
+	bool		need_transcoding;	/* file encoding diff from server? */
+	bool		encoding_embeds_ascii;	/* ASCII can be non-first byte? */
+
+	/* parameters from the COPY command */
+	Relation	rel;			/* relation to copy to */
+	QueryDesc  *queryDesc;		/* executable query to copy from */
+	List	   *attnumlist;		/* integer list of attnums to copy */
+	char	   *filename;		/* filename, or NULL for STDOUT */
+	bool		is_program;		/* is 'filename' a program to popen? */
+	copy_data_dest_cb data_dest_cb; /* function for writing data */
+
+	CopyFormatOptions opts;
+	Node	   *whereClause;	/* WHERE condition (or NULL) */
+
+	/*
+	 * Working state
+	 */
+	MemoryContext copycontext;	/* per-copy execution context */
+
+	FmgrInfo   *out_functions;	/* lookup info for output functions */
+	MemoryContext rowcontext;	/* per-row evaluation context */
+	uint64		bytes_processed;	/* number of bytes processed so far */
+} CopyToStateData;
+
+#endif							/* COPYTO_INTERNAL_H */
-- 
2.47.2

>From 427ec09b5cb438e7884d04027acdc207e9d4c80e Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email-masked]>
Date: Mon, 25 Nov 2024 14:01:18 +0900
Subject: [PATCH v36 3/7] Add support for implementing custom COPY TO format as
 extension

* Add CopyToStateData::opaque that can be used to keep data for custom
  COPY TO format implementation
* Export CopySendEndOfRow() to flush data in CopyToStateData::fe_msgbuf
  as CopyToStateFlush()
---
 src/backend/commands/copyto.c          | 12 ++++++++++++
 src/include/commands/copyapi.h         |  2 ++
 src/include/commands/copyto_internal.h |  3 +++
 3 files changed, 17 insertions(+)

diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 99c2f2dd699..f5ed3efbace 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -458,6 +458,18 @@ CopySendEndOfRow(CopyToState cstate)
 	resetStringInfo(fe_msgbuf);
 }
 
+/*
+ * Export CopySendEndOfRow() for extensions. We want to keep
+ * CopySendEndOfRow() as a static function for
+ * optimization. CopySendEndOfRow() calls in this file may be optimized by a
+ * compiler.
+ */
+void
+CopyToStateFlush(CopyToState cstate)
+{
+	CopySendEndOfRow(cstate);
+}
+
 /*
  * Wrapper function of CopySendEndOfRow for text and CSV formats. Sends the
  * line termination and do common appropriate things for the end of row.
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
index 4f4ffabf882..5c5ea6592e3 100644
--- a/src/include/commands/copyapi.h
+++ b/src/include/commands/copyapi.h
@@ -56,6 +56,8 @@ typedef struct CopyToRoutine
 	void		(*CopyToEnd) (CopyToState cstate);
 } CopyToRoutine;
 
+extern void CopyToStateFlush(CopyToState cstate);
+
 /*
  * API structure for a COPY FROM format implementation. Note this must be
  * allocated in a server-lifetime manner, typically as a static const struct.
diff --git a/src/include/commands/copyto_internal.h b/src/include/commands/copyto_internal.h
index 1b58b36c0a3..ce1c33a4004 100644
--- a/src/include/commands/copyto_internal.h
+++ b/src/include/commands/copyto_internal.h
@@ -78,6 +78,9 @@ typedef struct CopyToStateData
 	FmgrInfo   *out_functions;	/* lookup info for output functions */
 	MemoryContext rowcontext;	/* per-row evaluation context */
 	uint64		bytes_processed;	/* number of bytes processed so far */
+
+	/* For custom format implementation */
+	void	   *opaque;			/* private space */
 } CopyToStateData;
 
 #endif							/* COPYTO_INTERNAL_H */
-- 
2.47.2

>From fb9fb349e1d08a1adaf07658ccc46e8ea1439a80 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email-masked]>
Date: Mon, 25 Nov 2024 14:11:55 +0900
Subject: [PATCH v36 4/7] Add support for adding custom COPY FROM format

This uses the same handler for COPY TO and COPY FROM but uses
different routine. This uses CopyToRoutine for COPY TO and
CopyFromRoutine for COPY FROM. PostgreSQL calls a COPY TO/FROM handler
with "is_from" argument. It's true for COPY FROM and false for COPY
TO:

    copy_handler(true) returns CopyToRoutine
    copy_handler(false) returns CopyFromRoutine

This also add a test module for custom COPY FROM handler.
---
 src/backend/commands/copy.c                   | 13 +++----
 src/backend/commands/copyfrom.c               | 28 +++++++++++--
 src/include/catalog/pg_type.dat               |  2 +-
 src/include/commands/copyapi.h                |  2 +
 .../expected/test_copy_format.out             | 10 +++--
 .../test_copy_format/sql/test_copy_format.sql |  1 +
 .../test_copy_format/test_copy_format.c       | 39 ++++++++++++++++++-
 7 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 8d94bc313eb..b4417bb6819 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -483,8 +483,8 @@ defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate)
  * This function checks whether the option value is a built-in format such as
  * "text" and "csv" or not. If the option value isn't a built-in format, this
  * function finds a COPY format handler that returns a CopyToRoutine (for
- * is_from == false). If no COPY format handler is found, this function
- * reports an error.
+ * is_from == false) or CopyFromRountine (for is_from == true). If no COPY
+ * format handler is found, this function reports an error.
  */
 static void
 ProcessCopyOptionFormat(ParseState *pstate,
@@ -518,12 +518,9 @@ ProcessCopyOptionFormat(ParseState *pstate,
 	}
 
 	/* custom format */
-	if (!is_from)
-	{
-		funcargtypes[0] = INTERNALOID;
-		handlerOid = LookupFuncName(list_make1(makeString(format)), 1,
-									funcargtypes, true);
-	}
+	funcargtypes[0] = INTERNALOID;
+	handlerOid = LookupFuncName(list_make1(makeString(format)), 1,
+								funcargtypes, true);
 	if (!OidIsValid(handlerOid))
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index bcf66f0adf8..0809766f910 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -129,6 +129,7 @@ static void CopyFromBinaryEnd(CopyFromState cstate);
 
 /* text format */
 static const CopyFromRoutine CopyFromRoutineText = {
+	.type = T_CopyFromRoutine,
 	.CopyFromInFunc = CopyFromTextLikeInFunc,
 	.CopyFromStart = CopyFromTextLikeStart,
 	.CopyFromOneRow = CopyFromTextOneRow,
@@ -137,6 +138,7 @@ static const CopyFromRoutine CopyFromRoutineText = {
 
 /* CSV format */
 static const CopyFromRoutine CopyFromRoutineCSV = {
+	.type = T_CopyFromRoutine,
 	.CopyFromInFunc = CopyFromTextLikeInFunc,
 	.CopyFromStart = CopyFromTextLikeStart,
 	.CopyFromOneRow = CopyFromCSVOneRow,
@@ -145,6 +147,7 @@ static const CopyFromRoutine CopyFromRoutineCSV = {
 
 /* binary format */
 static const CopyFromRoutine CopyFromRoutineBinary = {
+	.type = T_CopyFromRoutine,
 	.CopyFromInFunc = CopyFromBinaryInFunc,
 	.CopyFromStart = CopyFromBinaryStart,
 	.CopyFromOneRow = CopyFromBinaryOneRow,
@@ -155,13 +158,30 @@ static const CopyFromRoutine CopyFromRoutineBinary = {
 static const CopyFromRoutine *
 CopyFromGetRoutine(const CopyFormatOptions *opts)
 {
-	if (opts->csv_mode)
+	if (OidIsValid(opts->handler))
+	{
+		Datum		datum;
+		Node	   *routine;
+
+		datum = OidFunctionCall1(opts->handler, BoolGetDatum(true));
+		routine = (Node *) DatumGetPointer(datum);
+		if (routine == NULL || !IsA(routine, CopyFromRoutine))
+			ereport(
+					ERROR,
+					(errcode(
+							 ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("COPY handler function "
+							"%u did not return "
+							"CopyFromRoutine struct",
+							opts->handler)));
+		return castNode(CopyFromRoutine, routine);
+	}
+	else if (opts->csv_mode)
 		return &CopyFromRoutineCSV;
 	else if (opts->binary)
 		return &CopyFromRoutineBinary;
-
-	/* default is text */
-	return &CopyFromRoutineText;
+	else
+		return &CopyFromRoutineText;
 }
 
 /* Implementation of the start callback for text and CSV formats */
diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat
index 340e0cd0a8d..63b7d65f982 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -634,7 +634,7 @@
   typoutput => 'tsm_handler_out', typreceive => '-', typsend => '-',
   typalign => 'i' },
 { oid => '8752',
-  descr => 'pseudo-type for the result of a copy to method function',
+  descr => 'pseudo-type for the result of a copy to/from method function',
   typname => 'copy_handler', typlen => '4', typbyval => 't', typtype => 'p',
   typcategory => 'P', typinput => 'copy_handler_in',
   typoutput => 'copy_handler_out', typreceive => '-', typsend => '-',
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
index 5c5ea6592e3..895c105d8d8 100644
--- a/src/include/commands/copyapi.h
+++ b/src/include/commands/copyapi.h
@@ -64,6 +64,8 @@ extern void CopyToStateFlush(CopyToState cstate);
  */
 typedef struct CopyFromRoutine
 {
+	NodeTag		type;
+
 	/*
 	 * Set input function information. This callback is called once at the
 	 * beginning of COPY FROM.
diff --git a/src/test/modules/test_copy_format/expected/test_copy_format.out b/src/test/modules/test_copy_format/expected/test_copy_format.out
index adfe7d1572a..016893e7026 100644
--- a/src/test/modules/test_copy_format/expected/test_copy_format.out
+++ b/src/test/modules/test_copy_format/expected/test_copy_format.out
@@ -2,9 +2,13 @@ CREATE EXTENSION test_copy_format;
 CREATE TABLE public.test (a smallint, b integer, c bigint);
 INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789);
 COPY public.test FROM stdin WITH (FORMAT 'test_copy_format');
-ERROR:  COPY format "test_copy_format" not recognized
-LINE 1: COPY public.test FROM stdin WITH (FORMAT 'test_copy_format')...
-                                          ^
+NOTICE:  test_copy_format: is_from=true
+NOTICE:  CopyFromInFunc: atttypid=21
+NOTICE:  CopyFromInFunc: atttypid=23
+NOTICE:  CopyFromInFunc: atttypid=20
+NOTICE:  CopyFromStart: natts=3
+NOTICE:  CopyFromOneRow
+NOTICE:  CopyFromEnd
 COPY public.test TO stdout WITH (FORMAT 'test_copy_format');
 NOTICE:  test_copy_format: is_from=false
 NOTICE:  CopyToOutFunc: atttypid=21
diff --git a/src/test/modules/test_copy_format/sql/test_copy_format.sql b/src/test/modules/test_copy_format/sql/test_copy_format.sql
index 810b3d8cedc..0dfdfa00080 100644
--- a/src/test/modules/test_copy_format/sql/test_copy_format.sql
+++ b/src/test/modules/test_copy_format/sql/test_copy_format.sql
@@ -2,4 +2,5 @@ CREATE EXTENSION test_copy_format;
 CREATE TABLE public.test (a smallint, b integer, c bigint);
 INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789);
 COPY public.test FROM stdin WITH (FORMAT 'test_copy_format');
+\.
 COPY public.test TO stdout WITH (FORMAT 'test_copy_format');
diff --git a/src/test/modules/test_copy_format/test_copy_format.c b/src/test/modules/test_copy_format/test_copy_format.c
index b42d472d851..abafc668463 100644
--- a/src/test/modules/test_copy_format/test_copy_format.c
+++ b/src/test/modules/test_copy_format/test_copy_format.c
@@ -18,6 +18,40 @@
 
 PG_MODULE_MAGIC;
 
+static void
+CopyFromInFunc(CopyFromState cstate, Oid atttypid,
+			   FmgrInfo *finfo, Oid *typioparam)
+{
+	ereport(NOTICE, (errmsg("CopyFromInFunc: atttypid=%d", atttypid)));
+}
+
+static void
+CopyFromStart(CopyFromState cstate, TupleDesc tupDesc)
+{
+	ereport(NOTICE, (errmsg("CopyFromStart: natts=%d", tupDesc->natts)));
+}
+
+static bool
+CopyFromOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls)
+{
+	ereport(NOTICE, (errmsg("CopyFromOneRow")));
+	return false;
+}
+
+static void
+CopyFromEnd(CopyFromState cstate)
+{
+	ereport(NOTICE, (errmsg("CopyFromEnd")));
+}
+
+static const CopyFromRoutine CopyFromRoutineTestCopyFormat = {
+	.type = T_CopyFromRoutine,
+	.CopyFromInFunc = CopyFromInFunc,
+	.CopyFromStart = CopyFromStart,
+	.CopyFromOneRow = CopyFromOneRow,
+	.CopyFromEnd = CopyFromEnd,
+};
+
 static void
 CopyToOutFunc(CopyToState cstate, Oid atttypid, FmgrInfo *finfo)
 {
@@ -59,5 +93,8 @@ test_copy_format(PG_FUNCTION_ARGS)
 	ereport(NOTICE,
 			(errmsg("test_copy_format: is_from=%s", is_from ? "true" : "false")));
 
-	PG_RETURN_POINTER(&CopyToRoutineTestCopyFormat);
+	if (is_from)
+		PG_RETURN_POINTER(&CopyFromRoutineTestCopyFormat);
+	else
+		PG_RETURN_POINTER(&CopyToRoutineTestCopyFormat);
 }
-- 
2.47.2

>From dcf1598a31557af0ade5758430884489a7c8e610 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email-masked]>
Date: Mon, 25 Nov 2024 14:19:34 +0900
Subject: [PATCH v36 5/7] Use COPY_SOURCE_ prefix for CopySource enum values

This is for consistency with CopyDest.
---
 src/backend/commands/copyfrom.c          |  4 ++--
 src/backend/commands/copyfromparse.c     | 10 +++++-----
 src/include/commands/copyfrom_internal.h |  6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 0809766f910..76662e04260 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1729,7 +1729,7 @@ BeginCopyFrom(ParseState *pstate,
 							pg_encoding_to_char(GetDatabaseEncoding()))));
 	}
 
-	cstate->copy_src = COPY_FILE;	/* default */
+	cstate->copy_src = COPY_SOURCE_FILE;	/* default */
 
 	cstate->whereClause = whereClause;
 
@@ -1857,7 +1857,7 @@ BeginCopyFrom(ParseState *pstate,
 	if (data_source_cb)
 	{
 		progress_vals[1] = PROGRESS_COPY_TYPE_CALLBACK;
-		cstate->copy_src = COPY_CALLBACK;
+		cstate->copy_src = COPY_SOURCE_CALLBACK;
 		cstate->data_source_cb = data_source_cb;
 	}
 	else if (pipe)
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index e8128f85e6b..17e51f02e04 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -180,7 +180,7 @@ ReceiveCopyBegin(CopyFromState cstate)
 	for (i = 0; i < natts; i++)
 		pq_sendint16(&buf, format); /* per-column formats */
 	pq_endmessage(&buf);
-	cstate->copy_src = COPY_FRONTEND;
+	cstate->copy_src = COPY_SOURCE_FRONTEND;
 	cstate->fe_msgbuf = makeStringInfo();
 	/* We *must* flush here to ensure FE knows it can send. */
 	pq_flush();
@@ -248,7 +248,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
 
 	switch (cstate->copy_src)
 	{
-		case COPY_FILE:
+		case COPY_SOURCE_FILE:
 			bytesread = fread(databuf, 1, maxread, cstate->copy_file);
 			if (ferror(cstate->copy_file))
 				ereport(ERROR,
@@ -257,7 +257,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
 			if (bytesread == 0)
 				cstate->raw_reached_eof = true;
 			break;
-		case COPY_FRONTEND:
+		case COPY_SOURCE_FRONTEND:
 			while (maxread > 0 && bytesread < minread && !cstate->raw_reached_eof)
 			{
 				int			avail;
@@ -340,7 +340,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
 				bytesread += avail;
 			}
 			break;
-		case COPY_CALLBACK:
+		case COPY_SOURCE_CALLBACK:
 			bytesread = cstate->data_source_cb(databuf, minread, maxread);
 			break;
 	}
@@ -1172,7 +1172,7 @@ CopyReadLine(CopyFromState cstate, bool is_csv)
 		 * after \. up to the protocol end of copy data.  (XXX maybe better
 		 * not to treat \. as special?)
 		 */
-		if (cstate->copy_src == COPY_FRONTEND)
+		if (cstate->copy_src == COPY_SOURCE_FRONTEND)
 		{
 			int			inbytes;
 
diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index c8b22af22d8..3a306e3286e 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -24,9 +24,9 @@
  */
 typedef enum CopySource
 {
-	COPY_FILE,					/* from file (or a piped program) */
-	COPY_FRONTEND,				/* from frontend */
-	COPY_CALLBACK,				/* from callback function */
+	COPY_SOURCE_FILE,			/* from file (or a piped program) */
+	COPY_SOURCE_FRONTEND,		/* from frontend */
+	COPY_SOURCE_CALLBACK,		/* from callback function */
 } CopySource;
 
 /*
-- 
2.47.2

>From d8657b0d0d295c82ec2807a9767bb7fbe18c85fa Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email-masked]>
Date: Mon, 25 Nov 2024 14:21:39 +0900
Subject: [PATCH v36 6/7] Add support for implementing custom COPY FROM format
 as extension

* Add CopyFromStateData::opaque that can be used to keep data for
  custom COPY From format implementation
* Export CopyGetData() to get the next data as
  CopyFromStateGetData()
---
 src/backend/commands/copyfromparse.c     | 11 +++++++++++
 src/include/commands/copyapi.h           |  2 ++
 src/include/commands/copyfrom_internal.h |  3 +++
 3 files changed, 16 insertions(+)

diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 17e51f02e04..d8fd238e72b 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -739,6 +739,17 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
 	return copied_bytes;
 }
 
+/*
+ * Export CopyGetData() for extensions. We want to keep CopyGetData() as a
+ * static function for optimization. CopyGetData() calls in this file may be
+ * optimized by a compiler.
+ */
+int
+CopyFromStateGetData(CopyFromState cstate, void *dest, int minread, int maxread)
+{
+	return CopyGetData(cstate, dest, minread, maxread);
+}
+
 /*
  * This function is exposed for use by extensions that read raw fields in the
  * next line. See NextCopyFromRawFieldsInternal() for details.
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
index 895c105d8d8..2044d8b8c4c 100644
--- a/src/include/commands/copyapi.h
+++ b/src/include/commands/copyapi.h
@@ -108,4 +108,6 @@ typedef struct CopyFromRoutine
 	void		(*CopyFromEnd) (CopyFromState cstate);
 } CopyFromRoutine;
 
+extern int	CopyFromStateGetData(CopyFromState cstate, void *dest, int minread, int maxread);
+
 #endif							/* COPYAPI_H */
diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index 3a306e3286e..af425cf5fd9 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -181,6 +181,9 @@ typedef struct CopyFromStateData
 #define RAW_BUF_BYTES(cstate) ((cstate)->raw_buf_len - (cstate)->raw_buf_index)
 
 	uint64		bytes_processed;	/* number of bytes processed so far */
+
+	/* For custom format implementation */
+	void	   *opaque;			/* private space */
 } CopyFromStateData;
 
 extern void ReceiveCopyBegin(CopyFromState cstate);
-- 
2.47.2

>From ddd4b9f09dfe7860195adf82dbaab5ee46e91cf0 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email-masked]>
Date: Wed, 27 Nov 2024 16:23:55 +0900
Subject: [PATCH v36 7/7] Add CopyFromSkipErrorRow() for custom COPY format
 extension

Extensions must call CopyFromSkipErrorRow() when CopyFromOneRow
callback reports an error by errsave(). CopyFromSkipErrorRow() handles
"ON_ERROR stop" and "LOG_VERBOSITY verbose" cases.
---
 src/backend/commands/copyfromparse.c          | 82 +++++++++++--------
 src/include/commands/copyapi.h                |  2 +
 .../expected/test_copy_format.out             | 47 +++++++++++
 .../test_copy_format/sql/test_copy_format.sql | 24 ++++++
 .../test_copy_format/test_copy_format.c       | 80 +++++++++++++++++-
 5 files changed, 198 insertions(+), 37 deletions(-)

diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index d8fd238e72b..2070f51a963 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -938,6 +938,51 @@ CopyFromCSVOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values,
 	return CopyFromTextLikeOneRow(cstate, econtext, values, nulls, true);
 }
 
+/*
+ * Call this when you report an error by errsave() in your CopyFromOneRow
+ * callback. This handles "ON_ERROR stop" and "LOG_VERBOSITY verbose" cases
+ * for you.
+ */
+void
+CopyFromSkipErrorRow(CopyFromState cstate)
+{
+	Assert(cstate->opts.on_error != COPY_ON_ERROR_STOP);
+
+	cstate->num_errors++;
+
+	if (cstate->opts.log_verbosity == COPY_LOG_VERBOSITY_VERBOSE)
+	{
+		/*
+		 * Since we emit line number and column info in the below notice
+		 * message, we suppress error context information other than the
+		 * relation name.
+		 */
+		Assert(!cstate->relname_only);
+		cstate->relname_only = true;
+
+		if (cstate->cur_attval)
+		{
+			char	   *attval;
+
+			attval = CopyLimitPrintoutLength(cstate->cur_attval);
+			ereport(NOTICE,
+					errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": \"%s\"",
+						   (unsigned long long) cstate->cur_lineno,
+						   cstate->cur_attname,
+						   attval));
+			pfree(attval);
+		}
+		else
+			ereport(NOTICE,
+					errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": null input",
+						   (unsigned long long) cstate->cur_lineno,
+						   cstate->cur_attname));
+
+		/* reset relname_only */
+		cstate->relname_only = false;
+	}
+}
+
 /*
  * Workhorse for CopyFromTextOneRow() and CopyFromCSVOneRow().
  *
@@ -1044,42 +1089,7 @@ CopyFromTextLikeOneRow(CopyFromState cstate, ExprContext *econtext,
 										(Node *) cstate->escontext,
 										&values[m]))
 		{
-			Assert(cstate->opts.on_error != COPY_ON_ERROR_STOP);
-
-			cstate->num_errors++;
-
-			if (cstate->opts.log_verbosity == COPY_LOG_VERBOSITY_VERBOSE)
-			{
-				/*
-				 * Since we emit line number and column info in the below
-				 * notice message, we suppress error context information other
-				 * than the relation name.
-				 */
-				Assert(!cstate->relname_only);
-				cstate->relname_only = true;
-
-				if (cstate->cur_attval)
-				{
-					char	   *attval;
-
-					attval = CopyLimitPrintoutLength(cstate->cur_attval);
-					ereport(NOTICE,
-							errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": \"%s\"",
-								   (unsigned long long) cstate->cur_lineno,
-								   cstate->cur_attname,
-								   attval));
-					pfree(attval);
-				}
-				else
-					ereport(NOTICE,
-							errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": null input",
-								   (unsigned long long) cstate->cur_lineno,
-								   cstate->cur_attname));
-
-				/* reset relname_only */
-				cstate->relname_only = false;
-			}
-
+			CopyFromSkipErrorRow(cstate);
 			return true;
 		}
 
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
index 2044d8b8c4c..500ece7d5bb 100644
--- a/src/include/commands/copyapi.h
+++ b/src/include/commands/copyapi.h
@@ -110,4 +110,6 @@ typedef struct CopyFromRoutine
 
 extern int	CopyFromStateGetData(CopyFromState cstate, void *dest, int minread, int maxread);
 
+extern void CopyFromSkipErrorRow(CopyFromState cstate);
+
 #endif							/* COPYAPI_H */
diff --git a/src/test/modules/test_copy_format/expected/test_copy_format.out b/src/test/modules/test_copy_format/expected/test_copy_format.out
index 016893e7026..b9a6baa85c0 100644
--- a/src/test/modules/test_copy_format/expected/test_copy_format.out
+++ b/src/test/modules/test_copy_format/expected/test_copy_format.out
@@ -1,6 +1,8 @@
 CREATE EXTENSION test_copy_format;
 CREATE TABLE public.test (a smallint, b integer, c bigint);
 INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789);
+-- 987 is accepted.
+-- 654 is a hard error because ON_ERROR is stop by default.
 COPY public.test FROM stdin WITH (FORMAT 'test_copy_format');
 NOTICE:  test_copy_format: is_from=true
 NOTICE:  CopyFromInFunc: atttypid=21
@@ -8,7 +10,50 @@ NOTICE:  CopyFromInFunc: atttypid=23
 NOTICE:  CopyFromInFunc: atttypid=20
 NOTICE:  CopyFromStart: natts=3
 NOTICE:  CopyFromOneRow
+NOTICE:  CopyFromOneRow
+ERROR:  invalid value: "6"
+CONTEXT:  COPY test, line 2, column a: "6"
+-- 987 is accepted.
+-- 654 is a soft error because ON_ERROR is ignore.
+COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore);
+NOTICE:  test_copy_format: is_from=true
+NOTICE:  CopyFromInFunc: atttypid=21
+NOTICE:  CopyFromInFunc: atttypid=23
+NOTICE:  CopyFromInFunc: atttypid=20
+NOTICE:  CopyFromStart: natts=3
+NOTICE:  CopyFromOneRow
+NOTICE:  CopyFromOneRow
+NOTICE:  CopyFromOneRow
+NOTICE:  1 row was skipped due to data type incompatibility
 NOTICE:  CopyFromEnd
+-- 987 is accepted.
+-- 654 is a soft error because ON_ERROR is ignore.
+COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore, LOG_VERBOSITY verbose);
+NOTICE:  test_copy_format: is_from=true
+NOTICE:  CopyFromInFunc: atttypid=21
+NOTICE:  CopyFromInFunc: atttypid=23
+NOTICE:  CopyFromInFunc: atttypid=20
+NOTICE:  CopyFromStart: natts=3
+NOTICE:  CopyFromOneRow
+NOTICE:  CopyFromOneRow
+NOTICE:  skipping row due to data type incompatibility at line 2 for column "a": "6"
+NOTICE:  CopyFromOneRow
+NOTICE:  1 row was skipped due to data type incompatibility
+NOTICE:  CopyFromEnd
+-- 987 is accepted.
+-- 654 is a soft error because ON_ERROR is ignore.
+-- 321 is a hard error.
+COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore);
+NOTICE:  test_copy_format: is_from=true
+NOTICE:  CopyFromInFunc: atttypid=21
+NOTICE:  CopyFromInFunc: atttypid=23
+NOTICE:  CopyFromInFunc: atttypid=20
+NOTICE:  CopyFromStart: natts=3
+NOTICE:  CopyFromOneRow
+NOTICE:  CopyFromOneRow
+NOTICE:  CopyFromOneRow
+ERROR:  too much lines: 3
+CONTEXT:  COPY test, line 3
 COPY public.test TO stdout WITH (FORMAT 'test_copy_format');
 NOTICE:  test_copy_format: is_from=false
 NOTICE:  CopyToOutFunc: atttypid=21
@@ -18,4 +63,6 @@ NOTICE:  CopyToStart: natts=3
 NOTICE:  CopyToOneRow: tts_nvalid=3
 NOTICE:  CopyToOneRow: tts_nvalid=3
 NOTICE:  CopyToOneRow: tts_nvalid=3
+NOTICE:  CopyToOneRow: tts_nvalid=3
+NOTICE:  CopyToOneRow: tts_nvalid=3
 NOTICE:  CopyToEnd
diff --git a/src/test/modules/test_copy_format/sql/test_copy_format.sql b/src/test/modules/test_copy_format/sql/test_copy_format.sql
index 0dfdfa00080..86db71bce7f 100644
--- a/src/test/modules/test_copy_format/sql/test_copy_format.sql
+++ b/src/test/modules/test_copy_format/sql/test_copy_format.sql
@@ -1,6 +1,30 @@
 CREATE EXTENSION test_copy_format;
 CREATE TABLE public.test (a smallint, b integer, c bigint);
 INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789);
+-- 987 is accepted.
+-- 654 is a hard error because ON_ERROR is stop by default.
 COPY public.test FROM stdin WITH (FORMAT 'test_copy_format');
+987
+654
+\.
+-- 987 is accepted.
+-- 654 is a soft error because ON_ERROR is ignore.
+COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore);
+987
+654
+\.
+-- 987 is accepted.
+-- 654 is a soft error because ON_ERROR is ignore.
+COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore, LOG_VERBOSITY verbose);
+987
+654
+\.
+-- 987 is accepted.
+-- 654 is a soft error because ON_ERROR is ignore.
+-- 321 is a hard error.
+COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore);
+987
+654
+321
 \.
 COPY public.test TO stdout WITH (FORMAT 'test_copy_format');
diff --git a/src/test/modules/test_copy_format/test_copy_format.c b/src/test/modules/test_copy_format/test_copy_format.c
index abafc668463..96a54dab7ec 100644
--- a/src/test/modules/test_copy_format/test_copy_format.c
+++ b/src/test/modules/test_copy_format/test_copy_format.c
@@ -14,6 +14,7 @@
 #include "postgres.h"
 
 #include "commands/copyapi.h"
+#include "commands/copyfrom_internal.h"
 #include "commands/defrem.h"
 
 PG_MODULE_MAGIC;
@@ -34,8 +35,85 @@ CopyFromStart(CopyFromState cstate, TupleDesc tupDesc)
 static bool
 CopyFromOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls)
 {
+	int			n_attributes = list_length(cstate->attnumlist);
+	char	   *line;
+	int			line_size = n_attributes + 1;	/* +1 is for new line */
+	int			read_bytes;
+
 	ereport(NOTICE, (errmsg("CopyFromOneRow")));
-	return false;
+
+	cstate->cur_lineno++;
+	line = palloc(line_size);
+	read_bytes = CopyFromStateGetData(cstate, line, line_size, line_size);
+	if (read_bytes == 0)
+		return false;
+	if (read_bytes != line_size)
+		ereport(ERROR,
+				(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+				 errmsg("one line must be %d bytes: %d",
+						line_size, read_bytes)));
+
+	if (cstate->cur_lineno == 1)
+	{
+		/* Success */
+		TupleDesc	tupDesc = RelationGetDescr(cstate->rel);
+		ListCell   *cur;
+		int			i = 0;
+
+		foreach(cur, cstate->attnumlist)
+		{
+			int			attnum = lfirst_int(cur);
+			int			m = attnum - 1;
+			Form_pg_attribute att = TupleDescAttr(tupDesc, m);
+
+			if (att->atttypid == INT2OID)
+			{
+				values[i] = Int16GetDatum(line[i] - '0');
+			}
+			else if (att->atttypid == INT4OID)
+			{
+				values[i] = Int32GetDatum(line[i] - '0');
+			}
+			else if (att->atttypid == INT8OID)
+			{
+				values[i] = Int64GetDatum(line[i] - '0');
+			}
+			nulls[i] = false;
+			i++;
+		}
+	}
+	else if (cstate->cur_lineno == 2)
+	{
+		/* Soft error */
+		TupleDesc	tupDesc = RelationGetDescr(cstate->rel);
+		int			attnum = lfirst_int(list_head(cstate->attnumlist));
+		int			m = attnum - 1;
+		Form_pg_attribute att = TupleDescAttr(tupDesc, m);
+		char		value[2];
+
+		cstate->cur_attname = NameStr(att->attname);
+		value[0] = line[0];
+		value[1] = '\0';
+		cstate->cur_attval = value;
+		errsave((Node *) cstate->escontext,
+				(
+				 errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+				 errmsg("invalid value: \"%c\"", line[0])));
+		CopyFromSkipErrorRow(cstate);
+		cstate->cur_attname = NULL;
+		cstate->cur_attval = NULL;
+		return true;
+	}
+	else
+	{
+		/* Hard error */
+		ereport(ERROR,
+				(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+				 errmsg("too much lines: %llu",
+						(unsigned long long) cstate->cur_lineno)));
+	}
+
+	return true;
 }
 
 static void
-- 
2.47.2

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 17 Mar 2025 13:50:03 -0700

> I've rebased the patch set. Here is a summary again:

Thank you for updating the patches. Here are some review comments on
the 0001 patch:

I think that built-in formats also need to have their handler
functions. This seems to be a conventional way for customizable
features such as tablesample and access methods, and we can simplify
this function.

---
I think we need to update the documentation to describe how users can
define the handler functions and what each callback function is
responsible for.

Regards,

---

From: Sutou Kouhei <[email-masked]>
Date: Wed, 19 Mar 2025 11:56:17 +0900 (JST)

Hi,

In <CAD21AoDU=[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 17 Mar 2025 13:50:03 -0700,

> features such as tablesample and access methods, and we can simplify

OK. 0008 in the attached v37 patch set does it.

> define the handler functions and what each callback function is

I agree with it but we haven't finalized public APIs yet. Can
we defer it after we finalize public APIs? (Proposed public
APIs exist in 0003, 0006 and 0007.)

And could someone help (take over if possible) writing a
document for this feature? I'm not good at writing a
document in English... 0009 in the attached v37 patch set
has a draft of it. It's based on existing documents in
doc/src/sgml/ and *.h.

0001-0007 aren't changed from v36 patch set.

Thanks,
-- 
kou

---

From: "David G. Johnston" <[email-masked]>
Date: Wed, 19 Mar 2025 17:49:49 -0700

> has a draft of it. It's based on existing documents in
I haven't touched the innards of the structs aside from changing
programlisting to synopsis.  And redoing the two section opening paragraphs
to better integrate with the content in the chapter opening.

The rest I kinda went to town on...

David J.

diff --git a/doc/src/sgml/copy-handler.sgml b/doc/src/sgml/copy-handler.sgml
index f602debae6..9d2897a104 100644
--- a/doc/src/sgml/copy-handler.sgml
+++ b/doc/src/sgml/copy-handler.sgml
@@ -10,56 +10,72 @@
  <para>
   <productname>PostgreSQL</productname> supports
   custom <link linkend="sql-copy"><literal>COPY</literal></link>
-  handlers. The <literal>COPY</literal> handlers can use different copy
format
-  instead of built-in <literal>text</literal>, <literal>csv</literal>
-  and <literal>binary</literal>.
+  handlers; adding additional <replaceable>format_name</replaceable>
options
+  to the <literal>FORMAT</literal> clause.
  </para>

  <para>
-  At the SQL level, a table sampling method is represented by a single SQL
-  function, typically implemented in C, having the signature
-<programlisting>
-format_name(internal) RETURNS copy_handler
-</programlisting>
-  The name of the function is the same name appearing in
-  the <literal>FORMAT</literal> option. The <type>internal</type> argument
is
-  a dummy that simply serves to prevent this function from being called
-  directly from an SQL command. The real argument is <literal>bool
-  is_from</literal>. If the handler is used by <literal>COPY
FROM</literal>,
-  it's <literal>true</literal>. If the handler is used by <literal>COPY
-  FROM</literal>, it's <literal>false</literal>.
+  At the SQL level, a copy handler method is represented by a single SQL
+  function (see <xref linkend="sql-createfunction"/>), typically
implemented in
+  C, having the signature
+<synopsis>
+<replaceable>format_name</replaceable>(internal) RETURNS
<literal>copy_handler</literal>
+</synopsis>
+  The function's name is then accepted as a valid
<replaceable>format_name</replaceable>.
+  The return pseudo-type <literal>copy_handler</literal> informs the
system that
+  this function needs to be registered as a copy handler.
+  The <type>internal</type> argument is a dummy that prevents
+  this function from being called directly from an SQL command.  As the
+  handler implementation must be server-lifetime immutable; this SQL
function's
+  volatility should be marked immutable.  The
<literal>link_symbol</literal>
+  for this function is the name of the implementation function, described
next.
  </para>

  <para>
-  The function must return <type>CopyFromRoutine *</type> when
-  the <literal>is_from</literal> argument is <literal>true</literal>.
-  The function must return <type>CopyToRoutine *</type> when
-  the <literal>is_from</literal> argument is <literal>false</literal>.
+  The implementation function signature expected for the function named
+  in the <literal>link_symbol</literal> is:
+<synopsis>
+Datum
+<replaceable>copy_format_handler</replaceable>(PG_FUNCTION_ARGS)
+</synopsis>
+  The convention for the name is to replace the word
+  <replaceable>format</replaceable> in the placeholder above with the
value given
+  to <replaceable>format_name</replaceable> in the SQL function.
+  The first argument is a <type>boolean</type> that indicates whether the
handler
+  must provide a pointer to its implementation for <literal>COPY
FROM</literal>
+  (a <type>CopyFromRoutine *</type>). If <literal>false</literal>, the
handler
+  must provide a pointer to its implementation of <literal>COPY
TO</literal>
+  (a <type>CopyToRoutine *</type>).  These structs are declared in
+  <filename>src/include/commands/copyapi.h</filename>.
  </para>

  <para>
-  The <type>CopyFromRoutine</type> and <type>CopyToRoutine</type> struct
types
-  are declared in <filename>src/include/commands/copyapi.h</filename>,
-  which see for additional details.
+  The structs hold pointers to implementation functions for
+  initializing, starting, processing rows, and ending a copy operation.
+  The specific structures vary a bit between <literal>COPY FROM</literal>
and
+  <literal>COPY TO</literal> so the next two sections describes each
+  in detail.
  </para>

  <sect1 id="copy-handler-from">
   <title>Copy From Handler</title>

   <para>
-   The <literal>COPY</literal> handler function for <literal>COPY
-   FROM</literal> returns a <type>CopyFromRoutine</type> struct containing
-   pointers to the functions described below. All functions are required.
+   The opening to this chapter describes how the executor will call the
+   main handler function with, in this case,
+   a <type>boolean</type> <literal>true</literal>, and expect to receive a
+   <type>CopyFromRoutine *</type> <type>Datum</type>.  This section
describes
+   the components of the <type>CopyFromRoutine</type> struct.
   </para>

   <para>
-<programlisting>
+<synopsis>
 void
 CopyFromInFunc(CopyFromState cstate,
                Oid atttypid,
                FmgrInfo *finfo,
                Oid *typioparam);
-</programlisting>
+</synopsis>

    This sets input function information for the
    given <literal>atttypid</literal> attribute. This function is called
once
@@ -110,11 +126,11 @@ CopyFromInFunc(CopyFromState cstate,
   </para>

   <para>
-<programlisting>
+<synopsis>
 void
 CopyFromStart(CopyFromState cstate,
               TupleDesc tupDesc);
-</programlisting>
+</synopsis>

    This starts a <literal>COPY FROM</literal>. This function is called
once at
    the beginning of <literal>COPY FROM</literal>.
@@ -144,13 +160,13 @@ CopyFromStart(CopyFromState cstate,
   </para>

   <para>
-<programlisting>
+<synopsis>
 bool
 CopyFromOneRow(CopyFromState cstate,
                ExprContext *econtext,
                Datum *values,
                bool *nulls);
-</programlisting>
+</synopsis>

    This reads one row from the source and fill <literal>values</literal>
    and <literal>nulls</literal>. If there is one or more tuples to be read,
@@ -202,10 +218,10 @@ CopyFromOneRow(CopyFromState cstate,
   </para>

   <para>
-<programlisting>
+<synopsis>
 void
 CopyFromEnd(CopyFromState cstate);
-</programlisting>
+</synopsis>

    This ends a <literal>COPY FROM</literal>. This function is called once
at
    the end of <literal>COPY FROM</literal>.
@@ -232,18 +248,20 @@ CopyFromEnd(CopyFromState cstate);
   <title>Copy To Handler</title>

   <para>
-   The <literal>COPY</literal> handler function for <literal>COPY
-   TO</literal> returns a <type>CopyToRoutine</type> struct containing
-   pointers to the functions described below. All functions are required.
+   The opening to this chapter describes how the executor will call the
+   main handler function with, in this case,
+   a <type>boolean</type> <literal>false</literal>, and expect to receive a
+   <type>CopyInRoutine *</type> <type>Datum</type>.  This section describes
+   the components of the <type>CopyInRoutine</type> struct.
   </para>

   <para>
-<programlisting>
+<synopsis>
 void
 CopyToOutFunc(CopyToState cstate,
               Oid atttypid,
               FmgrInfo *finfo);
-</programlisting>
+</synopsis>

    This sets output function information for the
    given <literal>atttypid</literal> attribute. This function is called
once
@@ -284,11 +302,11 @@ CopyToOutFunc(CopyToState cstate,
   </para>

   <para>
-<programlisting>
+<synopsis>
 void
 CopyToStart(CopyToState cstate,
             TupleDesc tupDesc);
-</programlisting>
+</synopsis>

    This starts a <literal>COPY TO</literal>. This function is called once
at
    the beginning of <literal>COPY TO</literal>.
@@ -316,11 +334,11 @@ CopyToStart(CopyToState cstate,
   </para>

   <para>
-<programlisting>
+<synopsis>
 bool
 CopyToOneRow(CopyToState cstate,
              TupleTableSlot *slot);
-</programlisting>
+</synopsis>

    This writes one row stored in <literal>slot</literal> to the
destination.

@@ -347,10 +365,10 @@ CopyToOneRow(CopyToState cstate,
   </para>

   <para>
-<programlisting>
+<synopsis>
 void
 CopyToEnd(CopyToState cstate);
-</programlisting>
+</synopsis>

    This ends a <literal>COPY TO</literal>. This function is called once at
    the end of <literal>COPY TO</literal>.

---

From: Sutou Kouhei <[email-masked]>
Date: Thu, 20 Mar 2025 10:24:55 +0900 (JST)

Hi,

In <CAKFQuwaMAFMHqxDXR=SxA0mDjdmntrwxZd2w=[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 19 Mar 2025 17:49:49 -0700,

> The rest I kinda went to town on...

Thanks!!! It's very helpful!!!

I've applied your patch. 0009 is only changed.

Thanks,
-- 
kou

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 21 Mar 2025 17:31:54 -0700

On Tue, Mar 18, 2025 at 7:56 PM Sutou Kouhei <[email-masked]> wrote:

>   "Re: Make COPY format extendable: Extract COPY TO format
> implementations" on Mon, 17 Mar 2025 13:50:03 -0700,
tl/dr;

We need to exclude from our SQL function search any function that doesn't
declare copy_handler as its return type.
("function text must return type copy_handler" is not an acceptable error
message)

We need to accept identifiers in FORMAT and parse the optional catalog,
schema, and object name portions.
(Restrict our function search to the named schema if provided.)

Detail:

Fun thing...(not sure how much of this is covered above: I do see, but
didn't scour, the security discussion):

-- create some poison
create function public.text(internal) returns boolean language c as
'/home/davidj/gotya/gotya', 'gotit';
CREATE FUNCTION

-- inject it
postgres=# set search_path to public,pg_catalog;
SET

-- watch it die
postgres=# copy (select 1) to stdout (format text);
ERROR:  function text must return type copy_handler
LINE 1: copy (select 1) to stdout (format text);

I'm especially concerned about extensions here.

We shouldn't be locating any SQL function that doesn't have a copy_handler
return type.  Unfortunately, LookupFuncName seems incapable of doing what
we want here.  I suggest we create a new lookup routine where we can
specify the return argument type as a required element.  That would cleanly
mitigate the denial-of-service attack/accident vector demonstrated above
(the text returning function should have zero impact on how this feature
behaves).  If someone does create a handler SQL function without using
copy_handler return type we'd end up showing "COPY format 'david' not
recognized" - a developer should be able to figure out they didn't put a
correct return type on their handler function and that is why the system
did not register it.

A second concern is simply people wanting to name things the same; or, why
namespaces were invented.

Can we just accept a proper identifier after FORMAT so we can use
schema-qualified names?

(FORMAT "davescopyformat"."david")

We can special case the internal schema-less names and internally force
pg_catalog to avoid them being shadowed.

David J.

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 21 Mar 2025 22:07:50 -0700

On Fri, Mar 21, 2025 at 5:32 PM David G. Johnston
<[email-masked]> wrote:
> We need to exclude from our SQL function search any function that doesn't declare copy_handler as its return type.
> ("function text must return type copy_handler" is not an acceptable error message)
>
> We need to accept identifiers in FORMAT and parse the optional catalog, schema, and object name portions.
> Fun thing...(not sure how much of this is covered above: I do see, but didn't scour, the security discussion):
>
> -- create some poison
> create function public.text(internal) returns boolean language c as '/home/davidj/gotya/gotya', 'gotit';
> CREATE FUNCTION
>
> -- inject it
> We shouldn't be locating any SQL function that doesn't have a copy_handler return type.  Unfortunately, LookupFuncName seems incapable of doing what we want here.  I suggest we create a new lookup routine where we can specify the return argument type as a required element.  That would cleanly mitigate the denial-of-service attack/accident vector demonstrated above (the text returning function should have zero impact on how this feature behaves).  If someone does create a handler SQL function without using copy_handler return type we'd end up showing "COPY format 'david' not recognized" - a developer should be able to figure out they didn't put a correct return type on their handler function and that is why the system did not register it.

Just to be clear, the patch checks the function's return type before calling it:

       funcargtypes[0] = INTERNALOID;
       handlerOid = LookupFuncName(list_make1(makeString(format)), 1,

funcargtypes, true);
       if (!OidIsValid(handlerOid))
               ereport(ERROR,
                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                                errmsg("COPY format \"%s\" not
recognized", format),
                                parser_errposition(pstate, defel->location)));

       /* check that handler has correct return type */
       if (get_func_rettype(handlerOid) != COPY_HANDLEROID)
               ereport(ERROR,
                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                                errmsg("function %s must return type %s",
                                               format, "copy_handler"),
                                parser_errposition(pstate, defel->location)));

So would changing the error message to like "COPY format 'text' not
recognized" untangle your concern?

FYI the same is true for TABLESAMPLE; it invokes a function with the
specified method name and checks the returned Node type:

=# select * from pg_class tablesample text (0);
ERROR:  function text must return type tsm_handler

A difference between TABLESAMPLE and COPY format is that the former
accepts a qualified name but the latter doesn't:

=# create extension tsm_system_rows ;
=# create schema s1;
=# create function s1.system_rows(internal) returns void language c as
'tsm_system_rows.so', 'tsm_system_rows_handler';
=# \df *.system_rows
                          List of functions
 Schema |    Name     | Result data type | Argument data types | Type
--------+-------------+------------------+---------------------+------
 public | system_rows | tsm_handler      | internal            | func
 s1     | system_rows | void             | internal            | func
(2 rows)
postgres(1:1194923)=# select count(*) from pg_class tablesample system_rows(0);
 count
-------
     0
(1 row)

postgres(1:1194923)=# select count(*) from pg_class tablesample
s1.system_rows(0);
ERROR:  function s1.system_rows must return type tsm_handler

> A second concern is simply people wanting to name things the same; or, why namespaces were invented.

Yeah, I think that the custom COPY format should support qualified
names at least.

Regards,

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 21 Mar 2025 22:23:56 -0700

On Friday, March 21, 2025, Masahiko Sawada <[email-masked]> wrote:

> > We need to exclude from our SQL function search any function that
> doesn't declare copy_handler as its return type.
> > We shouldn't be locating any SQL function that doesn't have a
> copy_handler return type.  Unfortunately, LookupFuncName seems incapable of
> doing what we want here.  I suggest we create a new lookup routine where we
> can specify the return argument type as a required element.  That would
> cleanly mitigate the denial-of-service attack/accident vector demonstrated
> above (the text returning function should have zero impact on how this
> feature behaves).  If someone does create a handler SQL function without
> using copy_handler return type we'd end up showing "COPY format 'david' not

In my example above copy should not fail at all.  The text function created
in public that returns Boolean would never be seen and the real one in
pg_catalog would then be found and behave as expected.

> ERROR:  function text must return type tsm_handler

Then this would benefit from the new function I suggest creating since it
apparently has the same, IMO, bug.

David J.

---

From: "David G. Johnston" <[email-masked]>
Date: Sat, 22 Mar 2025 10:11:10 -0700

> apparently has the same, IMO, bug.
Concretely like I posted here:
https://www.postgresql.org/message-id/[email-masked]

David J.

---

From: Masahiko Sawada <[email-masked]>
Date: Sun, 23 Mar 2025 02:01:59 -0700

> I've applied your patch. 0009 is only changed.

Thank you for updating the patches. I've reviewed the main part of
supporting the custom COPY format. Here are some random comments:

---
+/*

I think this comment needs to be updated as the part "If the option
value isn't ..." is no longer true.

I think we don't necessarily need to create a separate function
ProcessCopyOptionFormat for processing the format option.

We need more regression tests for handling the given format name. For example,

- more various input patterns.
- a function with the specified format name exists but it returns an
unexpected Node.
- looking for a handler function in a different namespace.
etc.

---
I think that we should accept qualified names too as the format name
like tablesample does. That way, different extensions implementing the
same format can be used.

---
+        if (routine == NULL || !IsA(routine, CopyFromRoutine))
+                ereport(
+                                ERROR,
+                                (errcode(
+
ERRCODE_INVALID_PARAMETER_VALUE),
+                                 errmsg("COPY handler function "
+                                                "%u did not return "
+                                                "CopyFromRoutine struct",
+                                                opts->handler)));

It's not conventional to put a new line between 'ereport(' and 'ERROR'
(similarly between 'errcode(' and 'ERRCODE_...'. Also, we don't need
to split the error message into multiple lines as it's not long.

---
+        if (routine == NULL || !IsA(routine, CopyToRoutine))
+                ereport(
+                                ERROR,
+                                (errcode(
+
ERRCODE_INVALID_PARAMETER_VALUE),
+                                 errmsg("COPY handler function "
+                                                "%u did not return "
+                                                "CopyToRoutine struct",
+                                                opts->handler)));

Same as the above comment.

---

s/method function/format function/

---
+        Oid                    handler;                /* handler
function for custom format routine */

'handler' is used also for built-in formats.

---
+static void

OIDs could be changed across major versions even for built-in types. I
think it's better to avoid using it for tests.

---
+static void
+CopyToOneRow(CopyToState cstate, TupleTableSlot *slot)
+{
+        ereport(NOTICE, (errmsg("CopyToOneRow: tts_nvalid=%u",
slot->tts_nvalid)));
+}

Similar to the above comment, the field name 'tts_nvalid' might also
be changed in the future, let's use another name.

---

I'd suggest not using the same function names as the fields.

---
+/*

Is there any reason to use a different name for public functions?

---
+/*
+CopyFromStateGetData(CopyFromState cstate, void *dest, int minread,
int maxread)

The same as the above comment.

---

How about renaming 'private'?

---
I've not reviewed the documentation patch yet but I think the patch
seems to miss the updates to the description of the FORMAT option in
the COPY command section.

---
I think we can reorganize the patch set as follows:

1. Create copyto_internal.h and change COPY_XXX to COPY_SOURCE_XXX and
COPY_DEST_XXX accordingly.
2. Support custom format for both COPY TO and COPY FROM.
3. Expose necessary helper functions such as CopySendEndOfRow().
4. Add CopyFromSkipErrorRow().
5. Documentation.

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 24 Mar 2025 17:45:10 -0700

> I've applied your patch. 0009 is only changed.

FYI I've implemented an extension to add JSON Lines format as a custom
COPY format[1] to check the usability of the COPY format APIs. I think
that the exposed APIs are fairly simple and minimum. I didn't find the
deficiency and excess of exposed APIs for helping extensions but I
find that it would be better to describe what the one-row callback
should do to utilize the abstracted destination. For example, in order
to use CopyToStateFlush() to write out to the destination, extensions
should write the data to cstate->fe_msgbuf. We expose
CopyToStateFlush() but not for any functions to write data there such
as CopySendString(). It was a bit inconvenient to me but I managed to
write the data directly there by #include'ing copyto_internal.h.

Regards,

[1] https://github.com/MasahikoSawada/pg_copy_jsonlines

---

From: Sutou Kouhei <[email-masked]>
Date: Thu, 27 Mar 2025 12:28:40 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sun, 23 Mar 2025 02:01:59 -0700,

> ProcessCopyOptionFormat for processing the format option.

Hmm. I think that this separated function will increase
readability by reducing indentation. But I've removed the
separation as you suggested. So the comment is also removed
entirely.

0002 includes this.

> - looking for a handler function in a different namespace.

I've added the following tests:

* Wrong input type handler without namespace
* Wrong input type handler with namespace
* Wrong return type handler without namespace
* Wrong return type handler with namespace
* Wrong return value (Copy*Routine isn't returned) handler without namespace
* Wrong return value (Copy*Routine isn't returned) handler with namespace
* Nonexistent handler
* Invalid qualified name
* Valid handler without namespace and without search_path
* Valid handler without namespace and with search_path
* Valid handler with namespace

0002 also includes this.

> like tablesample does. That way, different extensions implementing the

Implemented. It's implemented after parsing SQL. Is it OK?
(It seems that tablesample does it in parsing SQL.)

Because "WITH (FORMAT XXX)" is processed as a generic option
in gram.y. All generic options are processed as strings. So
I keep this.

Syntax is "COPY ... WITH (FORMAT 'NAMESPACE.HANDLER_NAME')"
not "COPY ... WITH (FORMAT 'NAMESPACE'.'HANDLER_NAME')"
because of this choice.

0002 also includes this.

> to split the error message into multiple lines as it's not long.

Oh, sorry. I can't remember why I used this... I think I
trusted pgindent...

> s/method function/format function/

Good catch. I used "handler function" not "format function"
because we use "handler" in other places.

> 'handler' is used also for built-in formats.

Updated in 0004.

> think it's better to avoid using it for tests.

Oh, I didn't know it. I've changed to use type name instead
of OID. It'll be more stable than OID.

> be changed in the future, let's use another name.

Hmm. If the field name is changed, we need to change this
code. So changing tests too isn't strange. Anyway, I used
more generic text.

> I'd suggest not using the same function names as the fields.

OK. I've added "Test" prefix.

> Is there any reason to use a different name for public functions?

In this patch set, I use "CopyFrom"/"CopyTo" prefixes for
public APIs for custom COPY FORMAT handler extensions. It
will help understanding related APIs. Is it strange in
PostgreSQL?

> How about renaming 'private'?

We should not use "private" because it's a keyword in
C++. If we use "private" here, we can't include this file
from C++ code.

> the COPY command section.

I defer this for now. We can revisit the last documentation
patch after we finalize our API. (Or could someone help us?)

> 4. Add CopyFromSkipErrorRow().

The attached v39 patch set uses the followings:

0001: Create copyto_internal.h and change COPY_XXX to
      COPY_SOURCE_XXX and COPY_DEST_XXX accordingly.
      (Same as 1. in your suggestion)
0002: Support custom format for both COPY TO and COPY FROM.
      (Same as 2. in your suggestion)
0003: Expose necessary helper functions such as CopySendEndOfRow()
      and add CopyFromSkipErrorRow().
      (3. + 4. in your suggestion)
0004: Define handler functions for built-in formats.
      (Not included in your suggestion)
0005: Documentation. (WIP)
      (Same as 5. in your suggestion)

We can merge 0001 quickly, right?

Thanks,
-- 
kou

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 28 Mar 2025 22:37:03 -0700

> * Valid handler with namespace

Probably we can merge these newly added four files into one .sql file?

Also we need to add some comments to describe what these queries test.
For example, it's not clear to me at a glance what queries in
no-schema.sql are going to test as there is no comment there.

> (It seems that tablesample does it in parsing SQL.)

I think it's okay.

One problem in the following chunk I can see is:

+           qualified_format = stringToQualifiedNameList(format, NULL);
+           DeconstructQualifiedName(qualified_format, &schema, &fmt);
+           if (!schema || strcmp(schema, "pg_catalog") == 0)
+           {
+               if (strcmp(fmt, "csv") == 0)
+                   opts_out->csv_mode = true;
+               else if (strcmp(fmt, "binary") == 0)
+                   opts_out->binary = true;
+           }

Non-qualified names depend on the search_path value so it's not
necessarily a built-in format. If the user specifies 'csv' with
seach_patch = 'myschema, pg_catalog', the COPY command unnecessarily
sets csv_mode true. I think we can instead check if the retrieved
handler function's OID matches the built-in formats' ones. Also, it's
weired to me that cstate has csv_mode and binary fields even though
the format should have already been known by the callback functions.

Regarding the documentation for the existing options, it says "...
only when not using XXX format." some places, where XXX can be
replaced with binary or CSV. Once we support custom formats, 'non-CSV
mode' would actually include custom formats as well, so we need to
update the description too.

> code.

Yes, but if we use independe name in the NOTICE message we would not
need to update the expected files.

> will help understanding related APIs. Is it strange in

I see your point. Probably we need to find a better name as the name
CopyToStateFlush doesn't sound well like this API should be called
only once at the end of a row (IOW user might try to call it multiple
times to 'flush' the state while processing a row). How about
CopyToEndOfRow()?

> C++. If we use "private" here, we can't include this file

Understood.

>       (Same as 5. in your suggestion)

Can we merge 0002 and 0004?

> We can merge 0001 quickly, right?

I don't think it makes sense to push only 0001 as it's a completely
preliminary patch for subsequent patches. It would be prudent to push
it once other patches are ready too.

Regards,

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 28 Mar 2025 22:57:56 -0700

> the format should have already been known by the callback functions.

I considered it a feature that a schema-less reference to text, csv, or
binary always resolves to the core built-in handlers.  As does an
unspecified format default of text.

To use an extension that chooses to override that format name would require
schema qualification.

David J.

---

From: Sutou Kouhei <[email-masked]>
Date: Sat, 29 Mar 2025 17:57:53 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 28 Mar 2025 22:37:03 -0700,

> Probably we can merge these newly added four files into one .sql file?

I know that src/test/regress/sql/ uses this style (one .sql
file includes many test patterns in one large category). I
understand that the style is preferable in
src/test/regress/sql/ because src/test/regress/sql/ has
tests for many categories.

But do we need to follow the style in
src/test/modules/*/sql/ too? If we use the style in
src/test/modules/*/sql/, we need to have only one .sql in
src/test/modules/*/sql/ because src/test/modules/*/ are for
each category.

And the current .sql per sub-category style is easy to debug
(at least for me). For example, if we try qualified name
cases on debugger, we can use "\i sql/schema.sql" instead of
extracting target statements from .sql that includes many
unrelated statements. (Or we can use "\i sql/all.sql" and
many GDB "continue"s.)

BTW, it seems that src/test/modules/test_ddl_deparse/sql/
uses .sql per sub-category style. Should we use one .sql
file for sql/test/modules/test_copy_format/sql/? If it's
required for merging this patch set, I'll do it.

> no-schema.sql are going to test as there is no comment there.

Hmm. You refer no_schema.sql in 0002, right?

----
DROP TABLE public.test;
DROP EXTENSION test_copy_format;
----

In general, COPY FORMAT tests focus on "COPY FROM WITH
(FORMAT ...)" and "COPY TO WITH (FORMAT ...)". And the file
name "no_schema" shows that it doesn't use qualified
name. Based on this, I feel that the above content is very
straightforward without any comment.

What should we add as comments? For example, do we need the
following comments?

----
-- This extension includes custom COPY handler: test_copy_format
CREATE EXTENSION test_copy_format;
-- Test data
CREATE TABLE public.test (a smallint, b integer, c bigint);
INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789);
-- Use custom COPY handler, test_copy_format, without
-- schema for FROM.
COPY public.test FROM stdin WITH (FORMAT 'test_copy_format');
\.
-- Use custom COPY handler, test_copy_format, without
-- schema for TO.
COPY public.test TO stdout WITH (FORMAT 'test_copy_format');
-- Cleanup
DROP TABLE public.test;
DROP EXTENSION test_copy_format;
----

> sets csv_mode true.

I think that we should always use built-in COPY handlers for
(not-qualified) "text", "csv" and "binary" for
compatibility. If we allow custom COPY handlers for
(not-qualified) "text", "csv" and "binary", pg_dump or
existing dump may be broken. Because we must use the same
COPY handler when we dump (COPY TO) and we restore (COPY
FROM).

BTW, the current implementation always uses
pg_catalog.{text,csv,binary} for (not-qualified) "text",
"csv" and "binary" even when there are
myschema.{text,csv,binary}. See
src/test/modules/test_copy_format/sql/builtin.sql. But I
haven't looked into it why...

>                     I think we can instead check if the retrieved
> handler function's OID matches the built-in formats' ones.

I agree that the approach is clear than the current
implementation. I'll use it when I create the next patch
set.

>                                                            Also, it's

You refer CopyFomratOptions::{csv_mode,binary} not
Copy{To,From}StateData, right? And you suggest that we
should replace all opts.csv_mode and opts.binary with
opts.handler == F_CSV and opts.handler == F_BINARY, right?

We can do it but I suggest that we do it as a refactoring
(or cleanup) in a separated patch for easy to review.

> update the description too.

I agree with you.

> times to 'flush' the state while processing a row). How about

CopyToStateFlush() can be called multiple times in a row. It
can also be called only once with multiple rows. Because it
just flushes the current buffer.

Existing CopySendEndOfRow() is called at the end of a
row. (Buffer is flushed at the end of row.) So I think that
the "EndOfRow" was chosen.

Some custom COPY handlers may not be row based. For example,
Apache Arrow COPY handler doesn't flush buffer for each row.
So, we should provide "flush" API not "end of row" API for
extensibility.

> Can we merge 0002 and 0004?

Can we do it when we merge this patch set if it's still
desirable at the time? Because:

* I think that separated 0002 and 0004 patches are easier to
  review than squashed 0002 and 0004 patch.
* I still think that someone may don't like defining COPY
  handlers for built-in formats. If we don't define COPY
  handlers for built-in formats finally, we can just drop
  0004.

> it once other patches are ready too.

Hmm. I feel that 0001 is a refactoring category patch like
merged patches. In general, distinct enum value names are
easier to understand.

BTW, does the "other patches" include the documentation
patch...?

Thanks,
-- 
kou

---

From: "David G. Johnston" <[email-masked]>
Date: Sat, 29 Mar 2025 09:12:00 -0700

>   handlers for built-in formats finally, we can just drop

We should (and usually do) dog-food APIs when reasonable and this situation
seems quite reasonable.  I'd push back quite a bit about publishing this
without any internal code leveraging it.

> merged patches. In general, distinct enum value names are
I'm for pushing 0001.  We've had copyfrom_internal.h for a while now and
this seems like a simple refactor to make that area of the code cleaner via
symmetry.

David J.

---

From: "David G. Johnston" <[email-masked]>
Date: Sat, 29 Mar 2025 09:48:22 -0700

>       (Same as 5. in your suggestion)
I don't think this module should be responsible for testing the validity of
"qualified names in a string literal" behavior.  Having some of the tests
use a schema qualification, and I'd suggest explicit
double-quoting/case-folding, wouldn't hurt just to demonstrate it's
possible, and how extensions should be referenced, but definitely don't
need tests to prove the broken cases are indeed broken.  This relies on an
existing API that has its own tests.  It is definitely pointlessly
redundant to have 6 tests that only differ from 6 other tests in their use
of a schema qualification.

I prefer keeping 0002 and 0004 separate.  In particular, keeping the design
choice of "unqualified internal format names ignore search_path" should
stand out as its own commit.

David J.

---

From: Sutou Kouhei <[email-masked]>
Date: Sun, 30 Mar 2025 11:31:26 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 29 Mar 2025 09:48:22 -0700,

> of a schema qualification.

You suggest the followings, right?

1. Add tests for "Schema.Name" with mixed cases
2. Remove the following 6 tests in
   src/test/modules/test_copy_format/sql/invalid.sql

   ----
   COPY public.test FROM stdin WITH (FORMAT 'test_schema.test_copy_format_wrong_input_type');
   COPY public.test TO stdout WITH (FORMAT 'test_schema.test_copy_format_wrong_input_type');
   COPY public.test FROM stdin WITH (FORMAT 'test_schema.test_copy_format_wrong_return_type');
   COPY public.test TO stdout WITH (FORMAT 'test_schema.test_copy_format_wrong_return_type');
   COPY public.test FROM stdin WITH (FORMAT 'test_schema.test_copy_format_wrong_return_value');
   COPY public.test TO stdout WITH (FORMAT 'test_schema.test_copy_format_wrong_return_value');
   ----

   because we have the following 6 tests:

   ----
   SET search_path = public,test_schema;
   COPY public.test FROM stdin WITH (FORMAT 'test_copy_format_wrong_input_type');
   COPY public.test TO stdout WITH (FORMAT 'test_copy_format_wrong_input_type');
   COPY public.test FROM stdin WITH (FORMAT 'test_copy_format_wrong_return_type');
   COPY public.test TO stdout WITH (FORMAT 'test_copy_format_wrong_return_type');
   COPY public.test FROM stdin WITH (FORMAT 'test_copy_format_wrong_return_value');
   COPY public.test TO stdout WITH (FORMAT 'test_copy_format_wrong_return_value');
   RESET search_path;
   ----
3. Remove the following tests because the behavior must be
   tested in other places:

   ----
   COPY public.test FROM stdin WITH (FORMAT 'nonexistent');
   COPY public.test TO stdout WITH (FORMAT 'nonexistent');
   COPY public.test FROM stdin WITH (FORMAT 'invalid.qualified.name');
   COPY public.test TO stdout WITH (FORMAT 'invalid.qualified.name');
   ----

Does it miss something?

1.: There is no difference between single-quoting and
    double-quoting here. Because the information what quote
    was used for the given FORMAT value isn't remained
    here. Should we update gram.y?

2.: I don't have a strong opinion for it. If nobody objects
    it, I'll remove them.

3.: I don't have a strong opinion for it. If nobody objects
    it, I'll remove them.

Thanks,
-- 
kou

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 31 Mar 2025 10:05:34 -0700

> > +        .CopyFromEnd = CopyFromEnd,
In trying to document the current API I'm strongly disliking it.  Namely,
the amount of internal code an extension needs to care about/copy-paste to
create a working handler.

Presently, pg_type defines text and binary I/O routines and the text/csv
formats use the text I/O while binary uses binary I/O - for all
attributes.  The CopyFromInFunc API allows for each attribute to somehow
have its I/O format individualized.  But I don't see how that is practical
or useful, and it adds burden on API users.

I suggest we remove both .CopyFromInFunc and .CopyFromStart/End and add a
property to CopyFromRoutine (.ioMode?) with values of either Copy_IO_Text
or Copy_IO_Binary and then just branch to either:

CopyFromTextLikeInFunc & CopyFromTextLikeStart/End
or
CopyFromBinaryInFunc & CopyFromStart/End

So, in effect, the only method an extension needs to write is converting
to/from the 'serialized' form to the text/binary form (text being near
unanimous).

In a similar manner, the amount of boilerplate within CopyFromOneRow seems
undesirable from an API perspective.

cstate->cur_attname = NameStr(att->attname);
cstate->cur_attval = string;

if (string != NULL)
    nulls[m] = false;

if (cstate->defaults[m])
{
    /* We must have switched into the per-tuple memory context */
    Assert(econtext != NULL);
    Assert(CurrentMemoryContext == econtext->ecxt_per_tuple_memory);

    values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
}

/*
    * If ON_ERROR is specified with IGNORE, skip rows with soft errors
    */
else if (!InputFunctionCallSafe(&in_functions[m],
                                string,
                                typioparams[m],
                                att->atttypmod,
                                (Node *) cstate->escontext,
                                &values[m]))
{
    CopyFromSkipErrorRow(cstate);
    return true;
}

cstate->cur_attname = NULL;
cstate->cur_attval = NULL;

It seems to me that CopyFromOneRow could simply produce a *string
collection,
one cell per attribute, and NextCopyFrom could do all of the above on a
for-loop over *string

The pg_type and pg_proc catalogs are not extensible so the API can and
should be limited to
producing the, usually text, values that are ready to be passed into the
text I/O routines and all
the work to find and use types and functions left in the template code.

I haven't looked at COPY TO but I am expecting much the same. The API
should simply receive
the content of the type I/O output routine (binary or text as it dictates)
for each output attribute, by
row, and be expected to take those values and produce a final output from
them.

David J.

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 31 Mar 2025 11:51:30 -0700

On Sat, Mar 29, 2025 at 9:49 AM David G. Johnston
<[email-masked]> wrote:
> I prefer keeping 0002 and 0004 separate.  In particular, keeping the design choice of "unqualified internal format names ignore search_path" should stand out as its own commit.

What is the point of having separate commits for already-agreed design
choices? I guess that it would make it easier to revert that decision.
But I think it makes more sense that if we agree with "unqualified
internal format names ignore search_path" the original commit includes
that decision and describes it in the commit message. If we want to
change that design based on the discussion later on, we can have a
separate commit that makes that change and has the link to the
discussion.

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 31 Mar 2025 12:35:23 -0700

> required for merging this patch set, I'll do it.

I'm not sure that the regression test queries are categorized in the
same way as in test_ddl_deparse. While the former have separate .sql
files for different types of inputs (valid inputs and invalid inputs
etc.) , which seems finer graind, the latter has .sql files for each
DDL command.

Most of the queries under test_copy_format/sql verifies the input
patterns of the FORMAT option. I find that the regression tests
included in that directory probably should focus on testing new
callback APIs and some regression tests for FORMAT option handling can
be moved into the normal regression test suite (e.g., in copy.sql or a
new file like copy_format.sql). IIUC testing for invalid input
patterns can be done even without creating artificial wrong handler
functions.

> DROP EXTENSION test_copy_format;

I'd like to see in the comment what the tests expect. Taking the
following queries as an example,

it would help readers understand the test case better if we have a
comment like for example:

-- Specify the custom format name without schema. We test if both
-- COPY TO and COPY FROM can find the correct handler function
-- in public schema.

> COPY handler when we dump (COPY TO) and we restore (COPY

I agreed.

> haven't looked into it why...

Sorry, I don't follow that. IIUC test_copy_format extension doesn't
create a handler function in myschema schema, and SQLs in builtin.sql
seem to work as expected (specifying a non-qualified built-in format
unconditionally uses the built-in format).

> Copy{To,From}StateData, right?

Yes. I referred to the wrong one.

> And you suggest that we

I think that csv_mode and binary are used mostly in
ProcessCopyOptions() so probably we can use local variables for that.
I find there are two other places where to use csv_mode:
NextCopyFromRawFields() and CopyToTextLikeStart(). I think we can
simply check the handler function's OID there, or we can define macros
like COPY_FORMAT_IS_TEXT/CSV/BINARY checking the OID and use them
there.

> So, we should provide "flush" API not "end of row" API for

Okay, understood.

> merged patches. In general, distinct enum value names are

Right, but the patches that have already been merged contributed to
speed up COPY commands, but 0001 patch also introduces
copyto_internal.h, which is not used by anyone in a case where the
custom copy format patch is not merged. Without adding
copyto_internal.h changing enum value names less makes sense to me.

> BTW, does the "other patches" include the documentation

I think that when pushing the main custom COPY format patch, we need
to include the documentation changes into it.

Regards,

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 31 Mar 2025 13:42:16 -0700

On Mon, Mar 31, 2025 at 11:52 AM Masahiko Sawada <[email-masked]>
wrote:

> > I prefer keeping 0002 and 0004 separate.  In particular, keeping the
> design choice of "unqualified internal format names ignore search_path"
> should stand out as its own commit.
>

Fair.  Comment withdrawn.  Though I was referring to the WIP patches; I
figured the final patch would squash this all together in any case.

David J.