Subject: Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

From: Andres Freund <[email-masked]>
Date: Sun, 5 Feb 2023 15:37:23 -0800

H,

On 2023-02-03 13:27:24 +0300, Damir Belyalov wrote:
> @@ -625,6 +628,173 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
>  	miinfo->bufferedBytes += tuplen;
>  }
>  
> +/*
> + * Safely reads source data, converts to a tuple and fills tuple buffer.
> + * Skips some data in the case of failed conversion if data source for
> + * a next tuple can be surely read without a danger.
> + */
> +static bool
> +SafeCopying(CopyFromState cstate, ExprContext *econtext, TupleTableSlot *myslot)

> +	BeginInternalSubTransaction(NULL);
> +	CurrentResourceOwner = sfcstate->oldowner;

I don't think this is the right approach. Creating a subtransaction for
each row will cause substantial performance issues.

We now can call data type input functions without throwing errors, see
InputFunctionCallSafe(). Use that to avoid throwing an error instead of
catching it.

Greetings,

Andres Freund

---

From: Damir Belyalov <[email-masked]>
Date: Mon, 6 Feb 2023 08:00:27 +0300

Hi, Andres!

Thank you for reviewing.

> each row will cause substantial performance issues.

Subtransactions aren't created for each row. The block of rows in one
subtransaction is 1000 (SAFE_BUFFER_SIZE) and can be changed. There is also
a constraint for the number of bytes MAX_SAFE_BUFFER_BYTES in safe_buffer:
 while (sfcstate->saved_tuples < SAFE_BUFFER_SIZE &&
     sfcstate->safeBufferBytes < MAX_SAFE_BUFFER_BYTES)

> InputFunctionCallSafe(). Use that to avoid throwing an error instead of
InputFunctionCallSafe() is good for detecting errors from input-functions
but there are such errors from NextCopyFrom () that can not be detected
with InputFunctionCallSafe(), e.g. "wrong number of columns in row''. Do
you offer to process input-function errors separately from other errors?
Now all errors are processed in one "switch" loop in PG_CATCH, so this
change can complicate code.

Regards,
Damir Belyalov
Postgres Professional

---

From: Tom Lane <[email-masked]>
Date: Mon, 06 Feb 2023 00:12:17 -0500

> subtransaction is 1000 (SAFE_BUFFER_SIZE) and can be changed.

I think that at this point, any patch that involves adding subtransactions
to COPY is dead on arrival; whether it's batched or not is irrelevant.
(It's not like batching has no downsides.)

> with InputFunctionCallSafe(), e.g. "wrong number of columns in row''.

If you want to deal with those, then there's more work to be done to make
those bits non-error-throwing.  But there's a very finite amount of code
involved and no obvious reason why it couldn't be done.  The major problem
here has always been the indefinite amount of code implicated by calling
datatype input functions, and we have now created a plausible answer to
that problem.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Sun, 05 Feb 2023 21:52:33 -0800

Hi, 

>(It's not like batching has no downsides.)

Indeed.

>datatype input functions, and we have now created a plausible answer to

I'm not even sure it makes sense to avoid that kind of error. And invalid column count or such is something quite different than failing some data type input routine, or falling a constraint. 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

---

From: Tom Lane <[email-masked]>
Date: Mon, 06 Feb 2023 01:00:34 -0500

>> involved and no obvious reason why it couldn't be done.

> I'm not even sure it makes sense to avoid that kind of error. And
> invalid column count or such is something quite different than failing
> some data type input routine, or falling a constraint.

I think it could be reasonable to put COPY's overall-line-format
requirements on the same level as datatype input format violations.
I agree that trying to trap every kind of error is a bad idea,
for largely the same reason that the soft-input-errors patches
only trap certain kinds of errors: it's too hard to tell whether
an error is an "internal" error that it's scary to continue past.

			regards, tom lane

---

From: torikoshia <[email-masked]>
Date: Mon, 27 Feb 2023 14:58:29 +0900

> an error is an "internal" error that it's scary to continue past.

Is it a bad idea to limit the scope of allowing errors to 'soft' errors 
in InputFunctionCallSafe()?

I think it could be still useful for some usecases.

   diff --git a/src/test/regress/sql/copy2.sql 
b/src/test/regress/sql/copy2.sql

   +-- tests for IGNORE_DATATYPE_ERRORS option
   +CREATE TABLE check_ign_err (n int, m int[], k int);
   +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS;
   +1  {1} 1
   +a  {2} 2
   +3  {3} 3333333333
   +4  {a, 4}  4
   +
   +5  {5} 5
   +\.
   +SELECT * FROM check_ign_err;

   diff --git a/src/test/regress/expected/copy2.out 
b/src/test/regress/expected/copy2.out
   index 090ef6c7a8..08e8056fc1 100644

   +-- tests for IGNORE_DATATYPE_ERRORS option
   +CREATE TABLE check_ign_err (n int, m int[], k int);
   +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS;
   +WARNING:  invalid input syntax for type integer: "a"
   +WARNING:  value "3333333333" is out of range for type integer
   +WARNING:  invalid input syntax for type integer: "a"
   +WARNING:  invalid input syntax for type integer: ""
   +SELECT * FROM check_ign_err;
   + n |  m  | k
   +---+-----+---
   + 1 | {1} | 1
   + 5 | {5} | 5
   +(2 rows)

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

From 16877d4cdd64db5f85bed9cd559e618d8211e598 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email-masked]>
Date: Mon, 27 Feb 2023 12:02:16 +0900
Subject: [PATCH v1] Add COPY option IGNORE_DATATYPE_ERRORS

---
 src/backend/commands/copy.c              |  8 ++++++++
 src/backend/commands/copyfrom.c          | 11 +++++++++++
 src/backend/commands/copyfromparse.c     | 12 ++++++++++--
 src/backend/parser/gram.y                |  8 +++++++-
 src/bin/psql/tab-complete.c              |  3 ++-
 src/include/commands/copy.h              |  1 +
 src/include/commands/copyfrom_internal.h |  2 ++
 src/include/parser/kwlist.h              |  1 +
 src/test/regress/expected/copy2.out      | 14 ++++++++++++++
 src/test/regress/sql/copy2.sql           | 12 ++++++++++++
 10 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e34f583ea7..2f1cfb3f4d 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -410,6 +410,7 @@ ProcessCopyOptions(ParseState *pstate,
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
+	bool		ignore_datatype_errors_specified= false;
 	ListCell   *option;
 
 	/* Support external use for option sanity checking */
@@ -449,6 +450,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_datatype_errors") == 0)
+		{
+			if (ignore_datatype_errors_specified)
+				errorConflictingDefElem(defel, pstate);
+			ignore_datatype_errors_specified= true;
+			opts_out->ignore_datatype_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index af52faca6d..24eec6a27d 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -959,6 +959,7 @@ CopyFrom(CopyFromState cstate)
 	{
 		TupleTableSlot *myslot;
 		bool		skip_tuple;
+		ErrorSaveContext escontext = {T_ErrorSaveContext};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -991,10 +992,20 @@ CopyFrom(CopyFromState cstate)
 
 		ExecClearTuple(myslot);
 
+		if (cstate->opts.ignore_datatype_errors)
+		{
+			escontext.details_wanted = true;
+			cstate->escontext = escontext;
+		}
+
 		/* Directly store the values/nulls array in the slot */
 		if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
 			break;
 
+		/* Soft error occured, skip this tuple */
+		if(cstate->escontext.error_occurred)
+			continue;
+
 		ExecStoreVirtualTuple(myslot);
 
 		/*
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 91b564c2bc..12b1780fd6 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -70,6 +70,7 @@
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
+#include "nodes/miscnodes.h"
 #include "pgstat.h"
 #include "port/pg_bswap.h"
 #include "utils/builtins.h"
@@ -938,10 +939,17 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 
 			cstate->cur_attname = NameStr(att->attname);
 			cstate->cur_attval = string;
-			values[m] = InputFunctionCall(&in_functions[m],
+			if (!InputFunctionCallSafe(&in_functions[m],
 										  string,
 										  typioparams[m],
-										  att->atttypmod);
+										  att->atttypmod,
+										  (Node *) &cstate->escontext,
+										  &values[m]))
+			{
+					ereport(WARNING,
+							  errmsg("%s", cstate->escontext.error_data->message));
+					return true;
+			}
 			if (string != NULL)
 				nulls[m] = false;
 			cstate->cur_attname = NULL;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a0138382a1..d79d293c0d 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -701,7 +701,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 	HANDLER HAVING HEADER_P HOLD HOUR_P
 
-	IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE
+	IDENTITY_P IF_P IGNORE_DATATYPE_ERRORS ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE
 	INCLUDING INCREMENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P
 	INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER
 	INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION
@@ -3378,6 +3378,10 @@ copy_opt_item:
 				{
 					$$ = makeDefElem("freeze", (Node *) makeBoolean(true), @1);
 				}
+			| IGNORE_DATATYPE_ERRORS
+				{
+					$$ = makeDefElem("ignore_datatype_errors", (Node *)makeBoolean(true), @1);
+				}
 			| DELIMITER opt_as Sconst
 				{
 					$$ = makeDefElem("delimiter", (Node *) makeString($3), @1);
@@ -16821,6 +16825,7 @@ unreserved_keyword:
 			| HOUR_P
 			| IDENTITY_P
 			| IF_P
+			| IGNORE_DATATYPE_ERRORS
 			| IMMEDIATE
 			| IMMUTABLE
 			| IMPLICIT_P
@@ -17375,6 +17380,7 @@ bare_label_keyword:
 			| HOLD
 			| IDENTITY_P
 			| IF_P
+			| IGNORE_DATATYPE_ERRORS
 			| ILIKE
 			| IMMEDIATE
 			| IMMUTABLE
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5e1882eaea..a363351d3b 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2857,7 +2857,8 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "("))
 		COMPLETE_WITH("FORMAT", "FREEZE", "DELIMITER", "NULL",
 					  "HEADER", "QUOTE", "ESCAPE", "FORCE_QUOTE",
-					  "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING");
+					  "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING",
+					  "IGNORE_DATATYPE_ERRORS");
 
 	/* Complete COPY <sth> FROM|TO filename WITH (FORMAT */
 	else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "FORMAT"))
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 8e5f6ff148..a7eb0f8883 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -42,6 +42,7 @@ typedef struct CopyFormatOptions
 								 * -1 if not specified */
 	bool		binary;			/* binary format? */
 	bool		freeze;			/* freeze rows on loading? */
+	bool		ignore_datatype_errors;  /* ignore rows with datatype errors */
 	bool		csv_mode;		/* Comma Separated Value format? */
 	CopyHeaderChoice header_line;	/* header line? */
 	char	   *null_print;		/* NULL marker string (server encoding!) */
diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index 7b1c4327bd..d74c633481 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -16,6 +16,7 @@
 
 #include "commands/copy.h"
 #include "commands/trigger.h"
+#include "nodes/miscnodes.h"
 
 /*
  * Represents the different source cases we need to worry about at
@@ -94,6 +95,7 @@ typedef struct CopyFromStateData
 	AttrNumber	num_defaults;
 	FmgrInfo   *in_functions;	/* array of input functions for each attrs */
 	Oid		   *typioparams;	/* array of element types for in_functions */
+	ErrorSaveContext escontext; /* soft error trapper during in_functions execution */
 	int		   *defmap;			/* array of default att numbers */
 	ExprState **defexprs;		/* array of default att expressions */
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index bb36213e6f..1d7f9efbc0 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -196,6 +196,7 @@ PG_KEYWORD("hold", HOLD, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("hour", HOUR_P, UNRESERVED_KEYWORD, AS_LABEL)
 PG_KEYWORD("identity", IDENTITY_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("if", IF_P, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("ignore_datatype_errors", IGNORE_DATATYPE_ERRORS, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("ilike", ILIKE, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("immediate", IMMEDIATE, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("immutable", IMMUTABLE, UNRESERVED_KEYWORD, BARE_LABEL)
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 090ef6c7a8..08e8056fc1 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -666,6 +666,20 @@ SELECT * FROM instead_of_insert_tbl;
 (2 rows)
 
 COMMIT;
+-- tests for IGNORE_DATATYPE_ERRORS option
+CREATE TABLE check_ign_err (n int, m int[], k int);
+COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS;
+WARNING:  invalid input syntax for type integer: "a"
+WARNING:  value "3333333333" is out of range for type integer
+WARNING:  invalid input syntax for type integer: "a"
+WARNING:  invalid input syntax for type integer: ""
+SELECT * FROM check_ign_err;
+ n |  m  | k 
+---+-----+---
+ 1 | {1} | 1
+ 5 | {5} | 5
+(2 rows)
+
 -- clean up
 DROP TABLE forcetest;
 DROP TABLE vistest;
diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql
index b0de82c3aa..380adfce96 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -464,6 +464,18 @@ test1
 SELECT * FROM instead_of_insert_tbl;
 COMMIT;
 
+-- tests for IGNORE_DATATYPE_ERRORS option
+CREATE TABLE check_ign_err (n int, m int[], k int);
+COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS;
+1	{1}	1
+a	{2}	2
+3	{3}	3333333333
+4	{a, 4}	4
+
+5	{5}	5
+\.
+SELECT * FROM check_ign_err;
+
 -- clean up
 DROP TABLE forcetest;
 DROP TABLE vistest;
-- 
2.25.1

---

From: Damir Belyalov <[email-masked]>
Date: Tue, 28 Feb 2023 17:28:06 +0300

Hello

Tested patch on all cases: CIM_SINGLE, CIM_MULTI, CIM_MULTI_CONDITION. As
expected it works.
Also added a description to copy.sgml and made a review on patch.

I added 'ignored_errors' integer parameter that should be output after the
option is finished.
All errors were added to the system logfile with full detailed context.
Maybe it's better to log only error message.
file:///home/abc13/Documents/todo_copy/postgres/v2-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch

Regards, Damir Belyalov
Postgres Professional

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 6 Mar 2023 15:03:38 +0100

> On 28 Feb 2023, at 15:28, Damir Belyalov <[email-masked]> wrote:

> Tested patch on all cases: CIM_SINGLE, CIM_MULTI, CIM_MULTI_CONDITION. As expected it works.
> I added 'ignored_errors' integer parameter that should be output after the option is finished.
> All errors were added to the system logfile with full detailed context. Maybe it's better to log only error message.

FWIW, Greenplum has a similar construct (but which also logs the errors in the
db) where data type errors are skipped as long as the number of errors don't
exceed a reject limit.  If the reject limit is reached then the COPY fails:

	LOG ERRORS [ SEGMENT REJECT LIMIT <count> [ ROWS | PERCENT ]]

IIRC the gist of this was to catch then the user copies the wrong input data or
plain has a broken file.  Rather than finding out after copying n rows which
are likely to be garbage the process can be restarted.

This version of the patch has a compiler error in the error message:

copyfrom.c: In function ‘CopyFrom’:
copyfrom.c:1008:29: error: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘uint64’ {aka ‘long long unsigned int’} [-Werror=format=]
1008 | ereport(WARNING, errmsg("Errors: %ld", cstate->ignored_errors));
     |                          ^~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~
     |                                              |
     |                                              uint64 {aka long long unsigned int}

On that note though, it seems to me that this error message leaves a bit to be
desired with regards to the level of detail.

--
Daniel Gustafsson

---

From: torikoshia <[email-masked]>
Date: Tue, 07 Mar 2023 11:07:07 +0900

>> Also added a description to copy.sgml and made a review on patch.
Thanks for your tests and improvements!

>> All errors were added to the system logfile with full detailed 
>> context. Maybe it's better to log only error message.
Certainly.

> copyfrom.c:1008:29: error: format ‘%ld’ expects argument of type ‘long
> int’, but argument 2 has type ‘uint64’ {aka ‘long long unsigned int’}
> [-Werror=format=]
+1.
I felt just logging "Error: %ld" would make people wonder the meaning of 
the %ld. Logging something like ""Error: %ld data type errors were 
found" might be clearer.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

---

From: Damir Belyalov <[email-masked]>
Date: Tue, 7 Mar 2023 11:35:32 +0300

> are likely to be garbage the process can be restarted.

I think this is a matter for discussion. The same question is: "Where to
log errors to separate files or to the system logfile?".
IMO it's better for users to log short-detailed error message to system
logfile and not output errors to the terminal.

This version of the patch has a compiler error in the error message:
Yes, corrected it. Changed "ignored_errors" to int64 because "processed"
(used for counting copy rows) is int64.

> the %ld. Logging something like ""Error: %ld data type errors were

Thanks. For more clearance change the message to: "Errors were found: %".

Regards, Damir Belyalov
Postgres Professional
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index c25b52d0cb..706b929947 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
 
     FORMAT <replaceable class="parameter">format_name</replaceable>
     FREEZE [ <replaceable class="parameter">boolean</replaceable> ]
+    IGNORE_DATATYPE_ERRORS [ <replaceable class="parameter">boolean</replaceable> ]
     DELIMITER '<replaceable class="parameter">delimiter_character</replaceable>'
     NULL '<replaceable class="parameter">null_string</replaceable>'
     HEADER [ <replaceable class="parameter">boolean</replaceable> | MATCH ]
@@ -233,6 +234,17 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term><literal>IGNORE_DATATYPE_ERRORS</literal></term>
+    <listitem>
+     <para>
+      Drops rows that contain malformed data while copying. These are rows
+      with columns where the data type's input-function raises an error.
+      Outputs warnings about rows with incorrect data to system logfile.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>DELIMITER</literal></term>
     <listitem>
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e34f583ea7..0334894014 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -410,6 +410,7 @@ ProcessCopyOptions(ParseState *pstate,
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
+	bool		ignore_datatype_errors_specified= false;
 	ListCell   *option;
 
 	/* Support external use for option sanity checking */
@@ -449,6 +450,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_datatype_errors") == 0)
+		{
+			if (ignore_datatype_errors_specified)
+				errorConflictingDefElem(defel, pstate);
+			ignore_datatype_errors_specified = true;
+			opts_out->ignore_datatype_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 29cd1cf4a6..facfc44def 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -949,10 +949,14 @@ CopyFrom(CopyFromState cstate)
 	errcallback.previous = error_context_stack;
 	error_context_stack = &errcallback;
 
+	if (cstate->opts.ignore_datatype_errors)
+		cstate->ignored_errors = 0;
+
 	for (;;)
 	{
 		TupleTableSlot *myslot;
 		bool		skip_tuple;
+		ErrorSaveContext escontext = {T_ErrorSaveContext};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -985,9 +989,26 @@ CopyFrom(CopyFromState cstate)
 
 		ExecClearTuple(myslot);
 
+		if (cstate->opts.ignore_datatype_errors)
+		{
+			escontext.details_wanted = true;
+			cstate->escontext = escontext;
+		}
+
 		/* Directly store the values/nulls array in the slot */
 		if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
+		{
+			if (cstate->opts.ignore_datatype_errors && cstate->ignored_errors > 0)
+				ereport(WARNING, errmsg("Errors were found: %lld", (long long) cstate->ignored_errors));
 			break;
+		}
+
+		/* Soft error occured, skip this tuple */
+		if (cstate->escontext.error_occurred)
+		{
+			ExecClearTuple(myslot);
+			continue;
+		}

diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 91b564c2bc..9c36b0dc8b 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -70,6 +70,7 @@
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
+#include "nodes/miscnodes.h"
 #include "pgstat.h"
 #include "port/pg_bswap.h"
 #include "utils/builtins.h"
@@ -938,10 +939,23 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 
-										  string,
-										  typioparams[m],
+			/* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype errors */
+									   string,
+									   typioparams[m],
+				cstate->ignored_errors++;
+				ereport(LOG,

index 8f12af799b..0f290cd6ff 100644
 
index 7b1c4327bd..b9ce636f7b 100644

 /*
@@ -94,6 +95,8 @@ typedef struct CopyFromStateData
+	int64		ignored_errors;	/* total number of ignored errors */
index 090ef6c7a8..525e3bc454 100644
@@ -666,6 +666,17 @@ SELECT * FROM instead_of_insert_tbl;
 (2 rows)
 
 COMMIT;
+WARNING:  Errors were found: 4

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 7 Mar 2023 10:09:23 +0100

> On 7 Mar 2023, at 09:35, Damir Belyalov <[email-masked]> wrote:

> Thanks. For more clearance change the message to: "Errors were found: %". 

I'm not convinced that this adds enough clarity to assist the user.  We also
shouldn't use "error" in a WARNING log since the user has explicitly asked to
skip rows on error, so it's not an error per se. How about something like:

  ereport(WARNING,
          (errmsg("%ld rows were skipped due to data type incompatibility", cstate->ignored_errors),
           errhint("Skipped rows can be inspected in the database log for reprocessing.")));

--
Daniel Gustafsson

---

From: torikoshia <[email-masked]>
Date: Fri, 17 Mar 2023 21:23:14 +0900

On 2023-03-07 18:09, Daniel Gustafsson wrote:
> skip rows on error, so it's not an error per se.
+1

> How about something like:
> 
>   ereport(WARNING,
>           (errmsg("%ld rows were skipped due to data type
> incompatibility", cstate->ignored_errors),
Since skipped rows cannot be inspected in the log when 
log_error_verbosity is set to terse,
it might be better without this errhint.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

---

From: torikoshia <[email-masked]>
Date: Wed, 22 Mar 2023 22:34:20 +0900

> it might be better without this errhint.

Removed errhint.

Modified some codes since v3 couldn't be applied HEAD anymore.

Also modified v3 patch as below:

> 65 +   if (cstate->opts.ignore_datatype_errors)
> 66 +       cstate->ignored_errors = 0;
> 67 +

It seems not necessary since cstate is initialized by palloc0() in 
BeginCopyFrom().

> 134 +               ereport(LOG,
> 135 +                       errmsg("%s", 
> cstate->escontext.error_data->message));
> 136 +
> 137 +               return true;

Since LOG means 'Reports information of interest to administrators'
according to the manual[1], datatype error should not be logged as
LOG. I put it back in WARNING.

[1] 
https://www.postgresql.org/docs/current/runtime-config-logging.html#RUNTIME-CONFIG-SEVERITY-LEVELS

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

From 6764d7e0f21ca266d7426cb922fd00e5138ec857 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email-masked]>
Date: Wed, 22 Mar 2023 22:00:15 +0900
Subject: [PATCH v4] Add new COPY option IGNORE_DATATYPE_ERRORS

Add new COPY option IGNORE_DATATYPE_ERRORS.

Currently entire COPY fails even when there is one unexpected
data regarding data type or range.
IGNORE_DATATYPE_ERRORS ignores these errors and skips them and
COPY data which don't contain problem.

This patch uses the soft error handling infrastructure, which
is introduced by d9f7f5d32f20.

Author: Damir Belyalov, Atsushi Torikoshi

---
 doc/src/sgml/ref/copy.sgml               | 12 ++++++++++++
 src/backend/commands/copy.c              |  8 ++++++++
 src/backend/commands/copyfrom.c          | 20 ++++++++++++++++++++
 src/backend/commands/copyfromparse.c     | 19 +++++++++++++++----
 src/backend/parser/gram.y                |  8 +++++++-
 src/include/commands/copy.h              |  1 +
 src/include/commands/copyfrom_internal.h |  3 +++
 src/include/parser/kwlist.h              |  1 +
 src/test/regress/expected/copy2.out      | 15 +++++++++++++++
 src/test/regress/sql/copy2.sql           | 12 ++++++++++++
 10 files changed, 94 insertions(+), 5 deletions(-)

index 5e591ed2e6..168b1c05d9 100644
 
@@ -234,6 +235,17 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term><literal>IGNORE_DATATYPE_ERRORS</literal></term>
+    <listitem>
+     <para>
+      Drops rows that contain malformed data while copying. These are rows
+      with columns where the data type's input-function raises an error.
+      Outputs warnings about rows with incorrect data to system logfile.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>DELIMITER</literal></term>
     <listitem>
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f14fae3308..02d911abbe 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -419,6 +419,7 @@ ProcessCopyOptions(ParseState *pstate,
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
+	bool		ignore_datatype_errors_specified = false;
 	ListCell   *option;
 
@@ -458,6 +459,13 @@ ProcessCopyOptions(ParseState *pstate,
index 80bca79cd0..85c47f54b2 100644
@@ -953,6 +953,7 @@ CopyFrom(CopyFromState cstate)
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -985,9 +986,28 @@ CopyFrom(CopyFromState cstate)
 
 		ExecClearTuple(myslot);
 
+						errmsg("%zd rows were skipped due to data type incompatibility",
+							cstate->ignored_errors));

index 3853902a16..b06c44e298 100644
@@ -956,10 +957,20 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 				values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
 			else
 
 			cstate->cur_attname = NULL;
 			cstate->cur_attval = NULL;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index efe88ccf9d..22bf63b42b 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -701,7 +701,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);

 	INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P
@@ -16827,6 +16831,7 @@ unreserved_keyword:
@@ -17382,6 +17387,7 @@ bare_label_keyword:
index 33175868f6..c2e55ac21f 100644
index ac2c16f8b8..1164c71631 100644

 /*
 								 * default value */
 	int		   *defmap;			/* array of default att numbers related to
 								 * missing att */
 	ExprState **defexprs;		/* array of default att expressions for all
index 753e9ee174..5ea159e879 100644
index 8e33eee719..a6bf3d66a4 100644
@@ -666,6 +666,21 @@ SELECT * FROM instead_of_insert_tbl;
 (2 rows)
 
 COMMIT;
+WARNING:  4 rows were skipped due to data type incompatibility
index d759635068..c934029314 100644

base-commit: d69c404c4cc5985d8ae5b5ed38bed3400b317f82
-- 
2.25.1

---

From: Andres Freund <[email-masked]>
Date: Wed, 22 Mar 2023 10:50:00 -0700

Hi,

Tom, see below - I wonder if should provide one more piece of infrastructure
around the saved error stuff...

Have you measured whether this has negative performance effects when *NOT*
using the new option?

As-is this does not work with FORMAT BINARY - and converting the binary input
functions to support soft errors won't happen for 16. So I think you need to
raise an error if BINARY and IGNORE_DATATYPE_ERRORS are specified.

> +			cstate->escontext = escontext;

I think it might be worth pulling this out of the loop. That does mean you'd
have to reset escontext.error_occurred after an error, but that doesn't seem
too bad, you need to do other cleanup anyway.

> +							errmsg("%s", cstate->escontext.error_data->message));

That isn't right - you loose all the details of the message. As is you'd also
leak the error context.

I think the best bet for now is to do something like
    /* adjust elevel so we don't jump out */
    cstate->escontext.error_data->elevel = WARNING;
    /* despite the name, this won't raise an error if elevel < ERROR */
    ThrowErrorData(cstate->escontext.error_data);

I wonder if we ought to provide a wrapper for this? It could e.g. know to
mention the original elevel and such?

I don't think NextCopyFrom() is the right place to emit this warning - it
e.g. is also called from file_fdw.c, which might want to do something else
with the error. From a layering POV it seems cleaner to do this in
CopyFrom(). You already have a check for escontext.error_occurred there
anyway.

>  					$$ = makeDefElem("delimiter", (Node *) makeString($3), @1);

I think we shouldn't add a new keyword for this, but only support this via
/* new COPY option syntax */
copy_generic_opt_list:
			copy_generic_opt_elem

Further increasing the size of the grammar with random keywords when we have
more generic ways to represent them seems unnecessary.

> +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS;

I suggest adding a few more tests:

- COPY with a datatype error that can't be handled as a soft error
- test documenting that COPY FORMAT BINARY is incompatible with IGNORE_DATATYPE_ERRORS
- a soft error showing the error context - although that will require some
  care to avoid the function name + line in the output

Greetings,

Andres Freund

---

From: torikoshia <[email-masked]>
Date: Fri, 24 Mar 2023 18:27:08 +0900

>     ThrowErrorData(cstate->escontext.error_data);

Thanks for your reviewing!
I'll try to fix it this way for the time being.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

---

From: torikoshia <[email-masked]>
Date: Mon, 27 Mar 2023 22:51:32 +0900

On 2023-03-23 02:50, Andres Freund wrote:
Thanks again for your review.
Attached v5 patch.

> Have you measured whether this has negative performance effects when 

I loaded 10000000 rows of pgbench_accounts on my laptop and compared the 
elapsed time.
GUCs changed from the default are logging_collector = on, 
log_error_verbosity = verbose.

Three tests were run under each condition and the middle of them is 
listed below:

- patch NOT applied(36f40ce2dc66): 35299ms
- patch applied, without IGNORE_DATATYPE_ERRORS: 34409ms
- patch applied, with IGNORE_DATATYPE_ERRORS: 35510ms

It seems there are no significant degradation.

Also tested the elapsed time when loading data which has some datatype 
error with IGNORE_DATATYPE_ERRORS:
- data has 100 rows of error: 35269ms
- data has 1000 rows of error: 34577ms
- data has 5000000 rows of error: 48925ms

5000000 rows of error consumes much time, but it seems to be influenced 
by logging time.
Here are test results under log_min_messages and client_min_messages are 
'error':
- data has 5000000 data type error: 23972ms
- data has 0 data type error: 34320ms

Now conversely, when there were many data type errors, it consumes less 
time.
This seems like a reasonable result since the amount of skipped data is 
increasing.

> raise an error if BINARY and IGNORE_DATATYPE_ERRORS are specified.

Added the option check.

> too bad, you need to do other cleanup anyway.

Pull this out of the loop and added process for resetting escontext.

>     ThrowErrorData(cstate->escontext.error_data);

As I mentioned in one previous email, added above codes for now.

> CopyFrom(). You already have a check for escontext.error_occurred there

Agreed.

> more generic ways to represent them seems unnecessary.

Agreed.

> - COPY with a datatype error that can't be handled as a soft error

Added a test for cases with missing columns.
However it's not datatype error and not what you expected, is it?

> - test documenting that COPY FORMAT BINARY is incompatible with 

Added it.

>   care to avoid the function name + line in the output

I assume you mean a test to check the server log, but I haven't come up 
with a way to do it.
Adding a TAP test might do it, but I think it would be overkill to add 
one just for this.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

From 6b646d96a9c2a310836693452deb2128636d1beb Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email-masked]>
Date: Mon, 27 Mar 2023 22:15:49 +0900
Subject: [PATCH v5] Add new COPY option IGNORE_DATATYPE_ERRORS

Currently entire COPY fails when there exists unexpected
data regarding data type or range.
In some cases, it would be useful to skip copying such data
and continue copying and IGNORE_DATATYPE_ERRORS does this.

Author: Damir Belyalov, Atsushi Torikoshi
---
 doc/src/sgml/ref/copy.sgml               | 13 ++++++++++
 src/backend/commands/copy.c              | 15 ++++++++++-
 src/backend/commands/copyfrom.c          | 32 ++++++++++++++++++++++++
 src/backend/commands/copyfromparse.c     | 17 ++++++++++---
 src/bin/psql/tab-complete.c              |  3 ++-
 src/include/commands/copy.h              |  1 +
 src/include/commands/copyfrom_internal.h |  3 +++
 src/test/regress/expected/copy2.out      | 22 ++++++++++++++++
 src/test/regress/sql/copy2.sql           | 19 ++++++++++++++
 9 files changed, 119 insertions(+), 6 deletions(-)

index 5e591ed2e6..cea56d65eb 100644
 
@@ -234,6 +235,18 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+      This option is not allowed when using binary format.  Note that this
+      is only supported in current <command>COPY</command> syntax.
index f14fae3308..aa50cc1ee1 100644
 
@@ -576,7 +584,7 @@ ProcessCopyOptions(ParseState *pstate,
 	}
 
 	/*
-	 * Check for incompatible options (must do these two before inserting
+	 * Check for incompatible options (must do these before inserting
 	 * defaults)
 	 */
 	if (opts_out->binary && opts_out->delim)
@@ -594,6 +602,11 @@ ProcessCopyOptions(ParseState *pstate,
 				(errcode(ERRCODE_SYNTAX_ERROR),
 				 errmsg("cannot specify DEFAULT in BINARY mode")));
 
+	if (opts_out->binary && opts_out->ignore_datatype_errors)
+		ereport(ERROR,
+				(errcode(ERRCODE_SYNTAX_ERROR),
+				 errmsg("cannot specify IGNORE_DATATYPE_ERRORS in BINARY mode")));
+
 	/* Set defaults for omitted options */
 	if (!opts_out->delim)
 		opts_out->delim = opts_out->csv_mode ? "," : "\t";
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 80bca79cd0..9e23cd45d5 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -652,6 +652,7 @@ CopyFrom(CopyFromState cstate)
 	bool		has_before_insert_row_trig;
 	bool		has_instead_insert_row_trig;
 	bool		leafpart_use_multi_insert = false;
+	ErrorSaveContext escontext = {T_ErrorSaveContext};
 
 	Assert(cstate->rel);
 	Assert(list_length(cstate->range_table) == 1);
@@ -752,6 +753,13 @@ CopyFrom(CopyFromState cstate)
 		ti_options |= TABLE_INSERT_FROZEN;
 	}
 
+	/* Set up soft error handler for IGNORE_DATATYPE_ERRORS */
+	if (cstate->opts.ignore_datatype_errors)
+	{
+		escontext.details_wanted = true;
+		cstate->escontext = escontext;
+	}
+
 	/*
 	 * We need a ResultRelInfo so we can use the regular executor's
 	 * index-entry-making machinery.  (There used to be a huge amount of code
@@ -987,7 +995,31 @@ CopyFrom(CopyFromState cstate)
 
 		/* Directly store the values/nulls array in the slot */
 		if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
+		{
+			if (cstate->opts.ignore_datatype_errors && cstate->ignored_errors > 0)
+				ereport(WARNING,
+						errmsg("%zd rows were skipped due to data type incompatibility",
+							cstate->ignored_errors));
 			break;
+		}
+
+		/* Soft error occured, skip this tuple and cleanup the escontext */
+		if (cstate->escontext.error_occurred)
+		{
+			ErrorSaveContext new_escontext = {T_ErrorSaveContext};
+
+			/* adjust elevel so we don't jump out */
+			cstate->escontext.error_data->elevel = WARNING;
+			/* despite the name, this won't raise an error since elevel is WARNING now */
+			ThrowErrorData(cstate->escontext.error_data);
+
+			ExecClearTuple(myslot);
+
+			new_escontext.details_wanted = true;
+			cstate->escontext = new_escontext;
+
+			continue;
+		}

index 3853902a16..6b0447782d 100644
@@ -956,10 +957,18 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 
 			cstate->cur_attname = NULL;
 			cstate->cur_attval = NULL;
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 42e87b9e49..4b443d4ea7 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2857,7 +2857,8 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "("))
 		COMPLETE_WITH("FORMAT", "FREEZE", "DELIMITER", "NULL",
 					  "HEADER", "QUOTE", "ESCAPE", "FORCE_QUOTE",
-					  "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING", "DEFAULT");
+					  "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING", "DEFAULT",
+					  "IGNORE_DATATYPE_ERRORS");

 
 /*
  * Represents the different source cases we need to worry about at
@@ -94,6 +95,8 @@ typedef struct CopyFromStateData
 								 * default value */
 	FmgrInfo   *in_functions;	/* array of input functions for each attrs */
 	Oid		   *typioparams;	/* array of element types for in_functions */
+	ErrorSaveContext escontext; /* soft error trapper during in_functions execution */
+	int64		ignored_errors;	/* total number of ignored errors */
 	int		   *defmap;			/* array of default att numbers related to
 								 * missing att */
 	ExprState **defexprs;		/* array of default att expressions for all
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 8e33eee719..99f70f44ed 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -82,6 +82,8 @@ COPY x to stdin (format BINARY, delimiter ',');
 ERROR:  cannot specify DELIMITER in BINARY mode
 COPY x to stdin (format BINARY, null 'x');
 ERROR:  cannot specify NULL in BINARY mode
+COPY x to stdin (format BINARY, ignore_datatype_errors);
+ERROR:  cannot specify IGNORE_DATATYPE_ERRORS in BINARY mode
 COPY x to stdin (format TEXT, force_quote(a));
 ERROR:  COPY force quote available only in CSV mode
 COPY x from stdin (format CSV, force_quote(a));
@@ -666,6 +668,25 @@ SELECT * FROM instead_of_insert_tbl;
 (2 rows)
 
 COMMIT;
+-- tests for IGNORE_DATATYPE_ERRORS option
+CREATE TABLE check_ign_err (n int, m int[], k int);
+COPY check_ign_err FROM STDIN WITH (IGNORE_DATATYPE_ERRORS);
+WARNING:  invalid input syntax for type integer: "a"
+WARNING:  value "3333333333" is out of range for type integer
+WARNING:  invalid input syntax for type integer: "a"
+WARNING:  invalid input syntax for type integer: ""
+WARNING:  4 rows were skipped due to data type incompatibility
+SELECT * FROM check_ign_err;
+ n |  m  | k 
+---+-----+---
+ 1 | {1} | 1
+ 5 | {5} | 5
+(2 rows)
+
+-- test missing data: should fail
+COPY check_ign_err FROM STDIN WITH (IGNORE_DATATYPE_ERRORS);
+ERROR:  missing data for column "k"
+CONTEXT:  COPY check_ign_err, line 1: "1	{1}"
 -- clean up
 DROP TABLE forcetest;
 DROP TABLE vistest;
@@ -680,6 +701,7 @@ DROP TABLE instead_of_insert_tbl;
 DROP VIEW instead_of_insert_tbl_view;
 DROP VIEW instead_of_insert_tbl_view_2;
 DROP FUNCTION fun_instead_of_insert_tbl();
+DROP TABLE check_ign_err;
 --
 -- COPY FROM ... DEFAULT
 --
diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql
index d759635068..e88f27bbfd 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -70,6 +70,7 @@ COPY x from stdin (encoding 'sql_ascii', encoding 'sql_ascii');
 -- incorrect options
 COPY x to stdin (format BINARY, delimiter ',');
 COPY x to stdin (format BINARY, null 'x');
+COPY x to stdin (format BINARY, ignore_datatype_errors);
 COPY x to stdin (format TEXT, force_quote(a));
 COPY x from stdin (format CSV, force_quote(a));
 COPY x to stdout (format TEXT, force_not_null(a));
@@ -464,6 +465,23 @@ test1
 SELECT * FROM instead_of_insert_tbl;
 COMMIT;
 
+-- tests for IGNORE_DATATYPE_ERRORS option
+CREATE TABLE check_ign_err (n int, m int[], k int);
+COPY check_ign_err FROM STDIN WITH (IGNORE_DATATYPE_ERRORS);
+1	{1}	1
+a	{2}	2
+3	{3}	3333333333
+4	{a, 4}	4
+
+5	{5}	5
+\.
+SELECT * FROM check_ign_err;
+
+-- test missing data: should fail
+COPY check_ign_err FROM STDIN WITH (IGNORE_DATATYPE_ERRORS);
+1	{1}
+\.
+
 -- clean up
 DROP TABLE forcetest;
 DROP TABLE vistest;
@@ -478,6 +496,7 @@ DROP TABLE instead_of_insert_tbl;
 DROP VIEW instead_of_insert_tbl_view;
 DROP VIEW instead_of_insert_tbl_view_2;
 DROP FUNCTION fun_instead_of_insert_tbl();
+DROP TABLE check_ign_err;
 
 --
 -- COPY FROM ... DEFAULT

base-commit: 36f40ce2dc66f1a36d6a12f7a0352e1c5bf1063e
-- 
2.25.1

---

From: Damir Belyalov <[email-masked]>
Date: Mon, 27 Mar 2023 17:28:09 +0300

Hi!

I made the specified changes and my patch turned out the same as yours. The
performance measurements were the same too.

The only thing left to do is how not to add IGNORE_DATATYPE_ERRORS as a
keyword. See how this is done for parameters such as FORCE_NOT_NULL,
FORCE_NULL, FORCE_QUOTE. They are not in kwlist.h and are not as keywords
in gram.y.

Regards,
Damir Belyalov
Postgres Professional

---

From: torikoshia <[email-masked]>
Date: Tue, 28 Mar 2023 21:16:13 +0900

On 2023-03-27 23:28, Damir Belyalov wrote:
> Hi!
> 
> I made the specified changes and my patch turned out the same as
> yours. The performance measurements were the same too.

Thanks for your review and measurements.

> The only thing left to do is how not to add IGNORE_DATATYPE_ERRORS as
> a keyword. See how this is done for parameters such as FORCE_NOT_NULL,
> FORCE_NULL, FORCE_QUOTE. They are not in kwlist.h and are not as
> keywords in gram.y.

I might misunderstand something, but I believe the v5 patch uses 
copy_generic_opt_list and it does not add IGNORE_DATATYPE_ERRORS as a 
keyword.
It modifies neither kwlist.h nor gram.y.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION