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

From: Alexander Korotkov <[email-masked]>
Date: Wed, 17 Jan 2024 23:01:59 +0200

On Wed, Jan 17, 2024 at 7:38 AM torikoshia <[email-masked]> wrote:
>
> Hi,
>
> Thanks for applying!
>
> > +                               errmsg_plural("%zd row were skipped due
> > to data type incompatibility",
>
> Sorry, I just noticed it, but 'were' should be 'was' here?

Sure, the fix is pushed.

------
Regards,
Alexander Korotkov

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 17 Jan 2024 23:06:56 +0200

On Wed, Jan 17, 2024 at 9:49 AM Kyotaro Horiguchi
<[email-masked]> wrote:
> At Wed, 17 Jan 2024 14:38:54 +0900, torikoshia <[email-masked]> wrote in
> > Hi,
> >
> > Thanks for applying!
> >
> > > + errmsg_plural("%zd row were skipped due to data type
> > > incompatibility",
> >
> > >> BTW I'm thinking we should add a column to pg_stat_progress_copy that
> > >> counts soft errors. I'll suggest this in another thread.
> > > Please do!
> >
> > I've started it here:
> >
> > https://www.postgresql.org/message-id/[email-masked]
>
> Switching topics, this commit (9e2d870119) adds the following help message:
>
>
> >                       "COPY { %s [ ( %s [, ...] ) ] | ( %s ) }\n"
> >                       "    TO { '%s' | PROGRAM '%s' | STDOUT }\n"
> > ...
> >                       "    SAVE_ERROR_TO '%s'\n"
> > ...
> >                       _("location"),
>
> On the other hand, SAVE_ERROR_TO takes 'error' or 'none', which
> indicate "immediately error out" and 'just ignore the failure'
> respectively, but these options hardly seem to denote a 'location',
> and appear more like an 'action'. I somewhat suspect that this
> parameter name intially conceived with the assupmtion that it would
> take file names or similar parameters. I'm not sure if others will
> agree, but I think the parameter name might not be the best
> choice. For instance, considering the addition of the third value
> 'log', something like on_error_action (error, ignore, log) would be
> more intuitively understandable. What do you think?

Probably, but I'm not sure about that.  The name SAVE_ERROR_TO assumes
the next word will be location, not action.  With some stretch we can
assume 'error' to be location.  I think it would be even more stretchy
to think that SAVE_ERROR_TO is followed by action.  Probably, we can
replace SAVE_ERROR_TO with another name which could be naturally
followed by action, but I don't have something appropriate in mind.
However, I'm not native english speaker and certainly could miss
something.

------
Regards,
Alexander Korotkov

---

From: Tom Lane <[email-masked]>
Date: Wed, 17 Jan 2024 16:37:57 -0500

> to think that SAVE_ERROR_TO is followed by action.

The other problem with this terminology is that with 'none', what it
is doing is the exact opposite of "saving" the errors.  I agree we
need a better name.

Kyotaro-san's suggestion isn't bad, though I might shorten it to
error_action {error|ignore|log} (or perhaps "stop" instead of "error")?
You will need a separate parameter anyway to specify the destination
of "log", unless "none" became an illegal table name when I wasn't
looking.  I don't buy that one parameter that has some special values
while other values could be names will be a good design.  Moreover,
what if we want to support (say) log-to-file along with log-to-table?
Trying to distinguish a file name from a table name without any other
context seems impossible.

			regards, tom lane

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 18 Jan 2024 09:56:39 +0900

> is doing is the exact opposite of "saving" the errors.  I agree we

Agreed.

> context seems impossible.

I've been thinking we can add more values to this option to log errors
not only to the server logs but also to the error table (not sure
details but I imagined an error table is created for each table on
error), without an additional option for the destination name. The
values would be like error_action {error|ignore|save-logs|save-table}.

Regards,

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

---

From: jian he <[email-masked]>
Date: Thu, 18 Jan 2024 09:10:51 +0800

> values would be like error_action {error|ignore|save-logs|save-table}.

another idea:
on_error {error|ignore|other_future_option}
if not specified then by default ERROR.
You can also specify ERROR or IGNORE for now.

I agree, the parameter "error_action" is better than "location".

---

From: torikoshia <[email-masked]>
Date: Thu, 18 Jan 2024 11:15:47 +0900

> I agree, the parameter "error_action" is better than "location".

I'm not sure whether error_action or on_error is better, but either way 
"error_action error" and "on_error error" seems a bit odd to me.
I feel "stop" is better for both cases as Tom suggested.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 18 Jan 2024 09:59:20 +0200

> I feel "stop" is better for both cases as Tom suggested.

OK.  What about this?
on_error {stop|ignore|other_future_option}
where other_future_option might be compound like "file 'copy.log'" or
"table 'copy_log'".

------
Regards,
Alexander Korotkov

---

From: Pavel Stehule <[email-masked]>
Date: Thu, 18 Jan 2024 09:01:24 +0100

čt 18. 1. 2024 v 8:59 odesílatel Alexander Korotkov <[email-masked]>
napsal:

> > > On Thu, Jan 18, 2024 at 8:57 AM Masahiko Sawada <[email-masked]
> >
> > > wrote:

+1

it is consistent with psql

Regards

Pavel

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 18 Jan 2024 17:33:16 +0900

+1

Regards,

---

From: torikoshia <[email-masked]>
Date: Thu, 18 Jan 2024 21:09:13 +0900

> where other_future_option might be compound like "file 'copy.log'" or

Thanks, also +1 from me.

-- 
Regards,

---

From: jian he <[email-masked]>
Date: Thu, 18 Jan 2024 22:59:44 +0800

Hi.
patch refactored based on "on_error {stop|ignore}"
doc changes:

--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -43,7 +43,7 @@ COPY { <replaceable
class="parameter">table_name</replaceable> [ ( <replaceable
     FORCE_QUOTE { ( <replaceable
class="parameter">column_name</replaceable> [, ...] ) | * }
     FORCE_NOT_NULL { ( <replaceable
class="parameter">column_name</replaceable> [, ...] ) | * }
     FORCE_NULL { ( <replaceable
class="parameter">column_name</replaceable> [, ...] ) | * }
-    SAVE_ERROR_TO '<replaceable class="parameter">location</replaceable>'
+    ON_ERROR '<replaceable class="parameter">error_action</replaceable>'
     ENCODING '<replaceable class="parameter">encoding_name</replaceable>'
 </synopsis>
  </refsynopsisdiv>
@@ -375,20 +375,20 @@ COPY { <replaceable
class="parameter">table_name</replaceable> [ ( <replaceable
    </varlistentry>

    <varlistentry>
-    <term><literal>SAVE_ERROR_TO</literal></term>
+    <term><literal>ON_ERROR</literal></term>
     <listitem>
      <para>
-      Specifies to save error information to <replaceable class="parameter">
-      location</replaceable> when there is malformed data in the input.
-      Currently, only <literal>error</literal> (default) and
<literal>none</literal>
+      Specifies which <replaceable class="parameter">
+      error_action</replaceable> to perform when there is malformed
data in the input.
+      Currently, only <literal>stop</literal> (default) and
<literal>ignore</literal>
       values are supported.
-      If the <literal>error</literal> value is specified,
+      If the <literal>stop</literal> value is specified,
       <command>COPY</command> stops operation at the first error.
-      If the <literal>none</literal> value is specified,
+      If the <literal>ignore</literal> value is specified,
       <command>COPY</command> skips malformed data and continues copying data.
       The option is allowed only in <command>COPY FROM</command>.
-      The <literal>none</literal> value is allowed only when
-      not using <literal>binary</literal> format.
+      Only <literal>stop</literal> value is allowed only when
+      using <literal>binary</literal> format.
      </para>
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 85881ca0..c30baec1 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -43,7 +43,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
     FORCE_QUOTE { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * }
     FORCE_NOT_NULL { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * }
     FORCE_NULL { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * }
-    SAVE_ERROR_TO '<replaceable class="parameter">location</replaceable>'
+    ON_ERROR '<replaceable class="parameter">error_action</replaceable>'
     ENCODING '<replaceable class="parameter">encoding_name</replaceable>'
 </synopsis>
  </refsynopsisdiv>
@@ -375,20 +375,20 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
    </varlistentry>
 
    <varlistentry>
-    <term><literal>SAVE_ERROR_TO</literal></term>
+    <term><literal>ON_ERROR</literal></term>
     <listitem>
      <para>
-      Specifies to save error information to <replaceable class="parameter">
-      location</replaceable> when there is malformed data in the input.
-      Currently, only <literal>error</literal> (default) and <literal>none</literal>
+      Specifies which <replaceable class="parameter">
+      error_action</replaceable> to perform when there is malformed data in the input.
+      Currently, only <literal>stop</literal> (default) and <literal>ignore</literal>
       values are supported.
-      If the <literal>error</literal> value is specified,
+      If the <literal>stop</literal> value is specified,
       <command>COPY</command> stops operation at the first error.
-      If the <literal>none</literal> value is specified,
+      If the <literal>ignore</literal> value is specified,
       <command>COPY</command> skips malformed data and continues copying data.
       The option is allowed only in <command>COPY FROM</command>.
-      The <literal>none</literal> value is allowed only when
-      not using <literal>binary</literal> format.
+      Only <literal>stop</literal> value is allowed only when
+      using <literal>binary</literal> format.
      </para>
     </listitem>
    </varlistentry>
@@ -577,7 +577,7 @@ COPY <replaceable class="parameter">count</replaceable>
 
    <para>
     <command>COPY</command> stops operation at the first error when
-    <literal>SAVE_ERROR_TO</literal> is not specified. This
+    <literal>ON_ERROR</literal> is not specified. This
     should not lead to problems in the event of a <command>COPY
     TO</command>, but the target table will already have received
     earlier rows in a <command>COPY FROM</command>. These rows will not
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c36d7f1d..cc0786c6 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -395,39 +395,39 @@ defGetCopyHeaderChoice(DefElem *def, bool is_from)
 }
 
 /*
- * Extract a CopySaveErrorToChoice value from a DefElem.
+ * Extract a CopyOnErrorChoice value from a DefElem.
  */
-static CopySaveErrorToChoice
-defGetCopySaveErrorToChoice(DefElem *def, ParseState *pstate, bool is_from)
+static CopyOnErrorChoice
+defGetCopyOnErrorChoice(DefElem *def, ParseState *pstate, bool is_from)
 {
 	char	   *sval;
 
 	if (!is_from)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("COPY SAVE_ERROR_TO cannot be used with COPY TO"),
+				 errmsg("COPY ON_ERROR cannot be used with COPY TO"),
 				 parser_errposition(pstate, def->location)));
 
 	/*
 	 * If no parameter value given, assume the default value.
 	 */
 	if (def->arg == NULL)
-		return COPY_SAVE_ERROR_TO_ERROR;
+		return COPY_ON_ERROR_STOP;
 
 	/*
-	 * Allow "error", or "none" values.
+	 * Allow "stop", or "ignore" values.
 	 */
 	sval = defGetString(def);
-	if (pg_strcasecmp(sval, "error") == 0)
-		return COPY_SAVE_ERROR_TO_ERROR;
-	if (pg_strcasecmp(sval, "none") == 0)
-		return COPY_SAVE_ERROR_TO_NONE;
+	if (pg_strcasecmp(sval, "stop") == 0)
+		return COPY_ON_ERROR_STOP;
+	if (pg_strcasecmp(sval, "ignore") == 0)
+		return COPY_ON_ERROR_IGNORE;
 
 	ereport(ERROR,
 			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-			 errmsg("COPY save_error_to \"%s\" not recognized", sval),
+			 errmsg("COPY ON_ERROR \"%s\" not recognized", sval),
 			 parser_errposition(pstate, def->location)));
-	return COPY_SAVE_ERROR_TO_ERROR;	/* keep compiler quiet */
+	return COPY_ON_ERROR_STOP;	/* keep compiler quiet */
 }
 
 /*
@@ -455,7 +455,7 @@ ProcessCopyOptions(ParseState *pstate,
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
-	bool		save_error_to_specified = false;
+	bool		on_error_specified = false;
 	ListCell   *option;
 
 	/* Support external use for option sanity checking */
@@ -608,12 +608,12 @@ ProcessCopyOptions(ParseState *pstate,
 								defel->defname),
 						 parser_errposition(pstate, defel->location)));
 		}
-		else if (strcmp(defel->defname, "save_error_to") == 0)
+		else if (strcmp(defel->defname, "on_error") == 0)
 		{
-			if (save_error_to_specified)
+			if (on_error_specified)
 				errorConflictingDefElem(defel, pstate);
-			save_error_to_specified = true;
-			opts_out->save_error_to = defGetCopySaveErrorToChoice(defel, pstate, is_from);
+			on_error_specified = true;
+			opts_out->on_error = defGetCopyOnErrorChoice(defel, pstate, is_from);
 		}
 		else
 			ereport(ERROR,
@@ -642,10 +642,10 @@ ProcessCopyOptions(ParseState *pstate,
 				(errcode(ERRCODE_SYNTAX_ERROR),
 				 errmsg("cannot specify DEFAULT in BINARY mode")));
 
-	if (opts_out->binary && opts_out->save_error_to != COPY_SAVE_ERROR_TO_ERROR)
+	if (opts_out->binary && opts_out->on_error != COPY_ON_ERROR_STOP)
 		ereport(ERROR,
 				(errcode(ERRCODE_SYNTAX_ERROR),
-				 errmsg("cannot specify SAVE_ERROR_TO in BINARY mode")));
+				 errmsg("only ON_ERROR STOP is allowed in BINARY mode")));
 
 	/* Set defaults for omitted options */
 	if (!opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 50e245d5..c956cfa4 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -657,7 +657,7 @@ CopyFrom(CopyFromState cstate)
 	Assert(cstate->rel);
 	Assert(list_length(cstate->range_table) == 1);
 
-	if (cstate->opts.save_error_to != COPY_SAVE_ERROR_TO_ERROR)
+	if (cstate->opts.on_error != COPY_ON_ERROR_STOP)
 		Assert(cstate->escontext);
 
 	/*
@@ -996,14 +996,14 @@ CopyFrom(CopyFromState cstate)
 		if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
 			break;
 
-		if (cstate->opts.save_error_to != COPY_SAVE_ERROR_TO_ERROR &&
+		if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
 			cstate->escontext->error_occurred)
 		{
 			/*
-			 * Soft error occured, skip this tuple and save error information
-			 * according to SAVE_ERROR_TO.
+			 * Soft error occured, skip this tuple and deal with error information
+			 * according to ON_ERROR.
 			 */
-			if (cstate->opts.save_error_to == COPY_SAVE_ERROR_TO_NONE)
+			if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)
 
 				/*
 				 * Just make ErrorSaveContext ready for the next NextCopyFrom.
@@ -1307,7 +1307,7 @@ CopyFrom(CopyFromState cstate)
 	/* Done, clean up */
 	error_context_stack = errcallback.previous;
 
-	if (cstate->opts.save_error_to != COPY_SAVE_ERROR_TO_ERROR &&
+	if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
 		cstate->num_errors > 0)
 		ereport(NOTICE,
 				errmsg_plural("%llu row was skipped due to data type incompatibility",
@@ -1450,18 +1450,18 @@ BeginCopyFrom(ParseState *pstate,
 		}
 	}
 
-	/* Set up soft error handler for SAVE_ERROR_TO */
-	if (cstate->opts.save_error_to != COPY_SAVE_ERROR_TO_ERROR)
+	/* Set up soft error handler for ON_ERROR */
+	if (cstate->opts.on_error != COPY_ON_ERROR_STOP)
 	{
 		cstate->escontext = makeNode(ErrorSaveContext);
 		cstate->escontext->type = T_ErrorSaveContext;
 		cstate->escontext->error_occurred = false;
 
 		/*
-		 * Currently we only support COPY_SAVE_ERROR_TO_NONE. We'll add other
+		 * Currently we only support COPY_ON_ERROR_IGNORE. We'll add other
 		 * options later
 		 */
-		if (cstate->opts.save_error_to == COPY_SAVE_ERROR_TO_NONE)
+		if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)
 			cstate->escontext->details_wanted = false;
 	}
 	else
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 7207eb26..36214aab 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -956,7 +956,7 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 
 				values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
 			}
-			/* If SAVE_ERROR_TO is specified, skip rows with soft errors */
+			/* If ON_ERROR is specified with IGNORE, skip rows with soft errors */
 			else if (!InputFunctionCallSafe(&in_functions[m],
 											string,
 											typioparams[m],
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6bfdb5f0..ada711d0 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2899,15 +2899,15 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("FORMAT", "FREEZE", "DELIMITER", "NULL",
 					  "HEADER", "QUOTE", "ESCAPE", "FORCE_QUOTE",
 					  "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING", "DEFAULT",
-					  "SAVE_ERROR_TO");
+					  "ON_ERROR");
 
 	/* Complete COPY <sth> FROM|TO filename WITH (FORMAT */
 	else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "FORMAT"))
 		COMPLETE_WITH("binary", "csv", "text");
 
-	/* Complete COPY <sth> FROM filename WITH (SAVE_ERROR_TO */
-	else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "SAVE_ERROR_TO"))
-		COMPLETE_WITH("error", "none");
+	/* Complete COPY <sth> FROM filename WITH (ON_ERROR */
+	else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "ON_ERROR"))
+		COMPLETE_WITH("stop", "ignore");
 
 	/* Complete COPY <sth> FROM <sth> WITH (<options>) */
 	else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny, "WITH", MatchAny))
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 8972c618..78af1b0e 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -34,11 +34,11 @@ typedef enum CopyHeaderChoice
  * Represents where to save input processing errors.  More values to be added
  * in the future.
  */
-typedef enum CopySaveErrorToChoice
+typedef enum CopyOnErrorChoice
 {
-	COPY_SAVE_ERROR_TO_ERROR = 0,	/* immediately throw errors */
-	COPY_SAVE_ERROR_TO_NONE,	/* ignore errors */
-} CopySaveErrorToChoice;
+	COPY_ON_ERROR_STOP = 0,	/* immediately throw errors, default */
+	COPY_ON_ERROR_IGNORE,	/* ignore errors */
+} CopyOnErrorChoice;
 
 /*
  * A struct to hold COPY options, in a parsed form. All of these are related
@@ -72,7 +72,7 @@ typedef struct CopyFormatOptions
 	bool		force_null_all; /* FORCE_NULL *? */
 	bool	   *force_null_flags;	/* per-column CSV FN flags */
 	bool		convert_selectively;	/* do selective binary conversion? */
-	CopySaveErrorToChoice save_error_to;	/* where to save error information */
+	CopyOnErrorChoice on_error;	/* what to do when error happened */
 	List	   *convert_select; /* list of column names (can be NIL) */
 } CopyFormatOptions;
 
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 42cbcb2e..d982ae4f 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -77,21 +77,21 @@ COPY x from stdin (encoding 'sql_ascii', encoding 'sql_ascii');
 ERROR:  conflicting or redundant options
 LINE 1: COPY x from stdin (encoding 'sql_ascii', encoding 'sql_ascii...
                                                  ^
-COPY x from stdin (save_error_to none,save_error_to none);
+COPY x from stdin (ON_ERROR ignore, ON_ERROR ignore);
 ERROR:  conflicting or redundant options
-LINE 1: COPY x from stdin (save_error_to none,save_error_to none);
-                                              ^
+LINE 1: COPY x from stdin (ON_ERROR ignore, ON_ERROR ignore);
+                                            ^
 -- incorrect options
 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 from stdin (format BINARY, save_error_to none);
-ERROR:  cannot specify SAVE_ERROR_TO in BINARY mode
-COPY x to stdin (save_error_to none);
-ERROR:  COPY SAVE_ERROR_TO cannot be used with COPY TO
-LINE 1: COPY x to stdin (save_error_to none);
-                         ^
+COPY x from stdin (format BINARY, ON_ERROR ignore);
+ERROR:  only ON_ERROR STOP is allowed in BINARY mode
+COPY x from stdin (ON_ERROR unsupported);
+ERROR:  COPY ON_ERROR "unsupported" not recognized
+LINE 1: COPY x from stdin (ON_ERROR unsupported);
+                           ^
 COPY x to stdin (format TEXT, force_quote(a));
 ERROR:  COPY FORCE_QUOTE requires CSV mode
 COPY x from stdin (format CSV, force_quote(a));
@@ -104,9 +104,9 @@ COPY x to stdout (format TEXT, force_null(a));
 ERROR:  COPY FORCE_NULL requires CSV mode
 COPY x to stdin (format CSV, force_null(a));
 ERROR:  COPY FORCE_NULL cannot be used with COPY TO
-COPY x to stdin (format BINARY, save_error_to unsupported);
-ERROR:  COPY SAVE_ERROR_TO cannot be used with COPY TO
-LINE 1: COPY x to stdin (format BINARY, save_error_to unsupported);
+COPY x to stdin (format BINARY, ON_ERROR unsupported);
+ERROR:  COPY ON_ERROR cannot be used with COPY TO
+LINE 1: COPY x to stdin (format BINARY, ON_ERROR unsupported);
                                         ^
 -- too many columns in column list: should fail
 COPY x (a, b, c, d, e, d, c) from stdin;
@@ -724,12 +724,12 @@ SELECT * FROM instead_of_insert_tbl;
 (2 rows)
 
 COMMIT;
--- tests for SAVE_ERROR_TO option
+-- tests for ON_ERROR option
 CREATE TABLE check_ign_err (n int, m int[], k int);
-COPY check_ign_err FROM STDIN WITH (save_error_to error);
+COPY check_ign_err FROM STDIN WITH (ON_ERROR stop);
 ERROR:  invalid input syntax for type integer: "a"
 CONTEXT:  COPY check_ign_err, line 2, column n: "a"
-COPY check_ign_err FROM STDIN WITH (save_error_to none);
+COPY check_ign_err FROM STDIN WITH (ON_ERROR ignore);
 NOTICE:  4 rows were skipped due to data type incompatibility
 SELECT * FROM check_ign_err;
  n |  m  | k 
@@ -740,15 +740,15 @@ SELECT * FROM check_ign_err;
 
 -- test datatype error that can't be handled as soft: should fail
 CREATE TABLE hard_err(foo widget);
-COPY hard_err FROM STDIN WITH (save_error_to none);
+COPY hard_err FROM STDIN WITH (ON_ERROR ignore);
 ERROR:  invalid input syntax for type widget: "1"
 CONTEXT:  COPY hard_err, line 1, column foo: "1"
 -- test missing data: should fail
-COPY check_ign_err FROM STDIN WITH (save_error_to none);
+COPY check_ign_err FROM STDIN WITH (ON_ERROR ignore);
 ERROR:  missing data for column "k"
 CONTEXT:  COPY check_ign_err, line 1: "1	{1}"
 -- test extra data: should fail
-COPY check_ign_err FROM STDIN WITH (save_error_to none);
+COPY check_ign_err FROM STDIN WITH (ON_ERROR ignore);
 ERROR:  extra data after last expected column
 CONTEXT:  COPY check_ign_err, line 1: "1	{1}	3	abc"
 -- clean up
diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql
index c48d5563..73b2e688 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -66,20 +66,20 @@ COPY x from stdin (force_not_null (a), force_not_null (b));
 COPY x from stdin (force_null (a), force_null (b));
 COPY x from stdin (convert_selectively (a), convert_selectively (b));
 COPY x from stdin (encoding 'sql_ascii', encoding 'sql_ascii');
-COPY x from stdin (save_error_to none,save_error_to none);
+COPY x from stdin (ON_ERROR ignore, ON_ERROR ignore);
 
 -- incorrect options
 COPY x to stdin (format BINARY, delimiter ',');
 COPY x to stdin (format BINARY, null 'x');
-COPY x from stdin (format BINARY, save_error_to none);
-COPY x to stdin (save_error_to none);
+COPY x from stdin (format BINARY, ON_ERROR ignore);
+COPY x from stdin (ON_ERROR unsupported);
 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));
 COPY x to stdin (format CSV, force_not_null(a));
 COPY x to stdout (format TEXT, force_null(a));
 COPY x to stdin (format CSV, force_null(a));
-COPY x to stdin (format BINARY, save_error_to unsupported);
+COPY x to stdin (format BINARY, ON_ERROR unsupported);
 
 -- too many columns in column list: should fail
 COPY x (a, b, c, d, e, d, c) from stdin;
@@ -498,9 +498,9 @@ test1
 SELECT * FROM instead_of_insert_tbl;
 COMMIT;
 
--- tests for SAVE_ERROR_TO option
+-- tests for ON_ERROR option
 CREATE TABLE check_ign_err (n int, m int[], k int);
-COPY check_ign_err FROM STDIN WITH (save_error_to error);
+COPY check_ign_err FROM STDIN WITH (ON_ERROR stop);
 1	{1}	1
 a	{2}	2
 3	{3}	3333333333
@@ -508,7 +508,7 @@ a	{2}	2
 
 5	{5}	5
 \.
-COPY check_ign_err FROM STDIN WITH (save_error_to none);
+COPY check_ign_err FROM STDIN WITH (ON_ERROR ignore);
 1	{1}	1
 a	{2}	2
 3	{3}	3333333333
@@ -520,17 +520,17 @@ SELECT * FROM check_ign_err;
 
 -- test datatype error that can't be handled as soft: should fail
 CREATE TABLE hard_err(foo widget);
-COPY hard_err FROM STDIN WITH (save_error_to none);
+COPY hard_err FROM STDIN WITH (ON_ERROR ignore);
 1
 \.
 
 -- test missing data: should fail
-COPY check_ign_err FROM STDIN WITH (save_error_to none);
+COPY check_ign_err FROM STDIN WITH (ON_ERROR ignore);
 1	{1}
 \.
 
 -- test extra data: should fail
-COPY check_ign_err FROM STDIN WITH (save_error_to none);
+COPY check_ign_err FROM STDIN WITH (ON_ERROR ignore);
 1	{1}	3	abc
 \.
 
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 29fd1cae..456461f8 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -478,6 +478,7 @@ CopyHeaderChoice
 CopyInsertMethod
 CopyMultiInsertBuffer
 CopyMultiInsertInfo
+CopyOnErrorChoice
 CopySource
 CopyStmt
 CopyToState
@@ -4041,4 +4042,3 @@ manifest_writer
 rfile
 ws_options
 ws_file_info
-CopySaveErrorToChoice

---

From: torikoshia <[email-masked]>
Date: Fri, 19 Jan 2024 21:37:38 +0900

On 2024-01-18 23:59, jian he wrote:
> -    SAVE_ERROR_TO '<replaceable 
> class="parameter">location</replaceable>'
> +    ON_ERROR '<replaceable 
> class="parameter">error_action</replaceable>'
>      ENCODING '<replaceable 
> class="parameter">encoding_name</replaceable>'
>  </synopsis>
>   </refsynopsisdiv>

Thanks for making the patch!

> +      using <literal>binary</literal> format.

The second 'only' may be unnecessary.

> +                       /* If ON_ERROR is specified with IGNORE, skip 

This is correct now, but considering future works which add other 
options like "file 'copy.log'" and
"table 'copy_log'", it may be better not to limit the case to 'IGNORE'.
How about something like this?

   If ON_ERROR is specified and the value is not STOP, skip rows with 
soft errors

>  COPY x from stdin (format CSV, force_quote(a));

In the existing test for copy2.sql, the COPY options are written in 
lower case(e.g. 'format')　and option value(e.g. 'BINARY') are written in 
upper case.
It would be more consistent to align them.

-- 
Regards,

---

From: Alexander Korotkov <[email-masked]>
Date: Fri, 19 Jan 2024 15:27:18 +0200

Hi!

> Thanks for making the patch!

The patch is pushed!  The proposed changes are incorporated excluding this.

>    If ON_ERROR is specified and the value is not STOP, skip rows with

I think when we have more options, then we wouldn't just skip rows
with soft errors but rather save them.  So, I left this comment as is
for now.

------
Regards,
Alexander Korotkov

---

From: torikoshia <[email-masked]>
Date: Fri, 19 Jan 2024 23:26:25 +0900

> with soft errors but rather save them.  So, I left this comment as is

Agreed.
Thanks for the notification!

-- 
Regards,