Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row - Mailing list pgsql-hackers

On Fri, Mar 21, 2025 at 2:34 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Few comments:
> 1) I felt this is wrong:
> diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
> index 9a4d993e2bc..7980513a9bd 100644
> --- a/src/bin/psql/tab-complete.in.c
> +++ b/src/bin/psql/tab-complete.in.c
> @@ -3280,7 +3280,7 @@ match_previous_words(int pattern_id,
>                 COMPLETE_WITH("FORMAT", "FREEZE", "DELIMITER", "NULL",
>                                           "HEADER", "QUOTE", "ESCAPE",
> "FORCE_QUOTE",
>                                           "FORCE_NOT_NULL",
> "FORCE_NULL", "ENCODING", "DEFAULT",
> -                                         "ON_ERROR", "LOG_VERBOSITY");
> +                                         "ON_ERROR", "SET_TO_NULL",
> "LOG_VERBOSITY");
>
> as the following fails:
> postgres=# copy t_on_error_null from stdin WITH ( set_to_null );
> ERROR:  option "set_to_null" not recognized
> LINE 1: copy t_on_error_null from stdin WITH ( set_to_null );
>

- COMPLETE_WITH("stop", "ignore");
+ COMPLETE_WITH("stop", "ignore", "set_to_null");
yech. I think I fixed this.

> 2) Can you limit this to 80 chars if possible to improve the readability:
> +      <literal>stop</literal> means fail the command,
> +      <literal>ignore</literal> means discard the input row and
> continue with the next one, and
> +      <literal>set_to_null</literal> means replace columns containing
> invalid input values with
> +      <literal>NULL</literal> and move to the next field.
>
> 3) similarly here too:
> +      For <literal>ignore</literal> option,
> +      a <literal>NOTICE</literal> message containing the ignored row count is
> +      emitted at the end of the <command>COPY FROM</command> if at
> least one row was discarded.
> +      For <literal>set_to_null</literal> option,
> +      a <literal>NOTICE</literal> message indicating the number of
> rows where invalid input values were replaced with
> +      null is emitted at the end of the <command>COPY FROM</command>
> if at least one row was replaced.
>
sure.

> 4) Could you mention a brief one line in the commit message as to why
> "on_error null" cannot be used:
> Extent "on_error action", introduce new option:  on_error set_to_null.
> Current grammar makes us unable to use "on_error null", so we choose
> "on_error set_to_null".

by the following changes, we can change to (on_error null).
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3579,6 +3579,7 @@ copy_generic_opt_elem:

 copy_generic_opt_arg:
                        opt_boolean_or_string                   { $$ =
(Node *) makeString($1); }
+                       | NULL_P
         { $$ = (Node *) makeString("null"); }
                        | NumericOnly
 { $$ = (Node *) $1; }
                        | '*'
         { $$ = (Node *) makeNode(A_Star); }
                        | DEFAULT                       { $$ = (Node
*) makeString("default"); }

COPY x from stdin (format null);
ERROR:  syntax error at or near "null"
LINE 1: COPY x from stdin (format null);
                                  ^
will become

COPY x from stdin (format null);
ERROR:  COPY format "null" not recognized
LINE 1: COPY x from stdin (format null);
                           ^

it will cause NULL_P from reserved word to
non-reserved word in the COPY related command.


I am not sure this is what we want.
Anyway, I attached both two version
(ON_ERROR SET_TO_NULL) (ON_ERROR NULL).