Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row

Hi,

Thanks for updating the patch and I've read
v17-0001-COPY-on_error-set_null.patch and here are some comments.

> +COPY x from stdin (on_error set_null, reject_limit 2);
> +ERROR: COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE

I understand that REJECT_LIMIT is out of scope for this patch, but
personally, I feel that supporting REJECT_LIMIT with ON_ERROR SET_NULL
would be a natural extension.
- Both IGNORE and SET_NULL share the common behavior of allowing COPY to
continue despite soft errors.
- Since REJECT_LIMIT defines the threshold for how many soft errors can
be tolerated before COPY fails, it seems consistent to allow it with
SET_NULL as well.

+ if (current_row_erroneous)
+ cstate->num_errors++;

Is there any reason this error counting isn't placed inside the "if
(cstate->opts.on_error == COPY_ON_ERROR_SET_NULL)" block?
As far as I can tell, current_row_erroneous is only modified within that
block, so it might make sense to keep this logic together for clarity.

These may be very minor, but I noticed a few inconsistencies in casing
and wording:

+ * If ON_ERROR is specified with IGNORE, skip rows with
soft errors.
+ * If ON_ERROR is specified with set_null, try to
replace with null.

IGNORE is in uppercase, but set_null is lowercase.

+ * we use it to count number of rows
(not fields!) that
+ * successfully applied on_error
set_null.

The sentence should begin with a capital: "We use it..."
Also, I felt it's unclear what "we use it" means. Does it necessary?

+COPY x to stdout (on_error set_null);
+ERROR: COPY ON_ERROR cannot be used with COPY TO
+LINE 1: COPY x to stdout (on_error set_null);

COPY is uppercase, but to is lowercase.

+COPY x from stdin (format BINARY, on_error set_null);
+ERROR: only ON_ERROR STOP is allowed in BINARY mode
+COPY x from stdin (on_error set_null, reject_limit 2);
+ERROR: COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE
...
+COPY t_on_error_null FROM STDIN WITH (on_error set_null);
+ERROR: domain d_int_not_null does not allow null values
+CONTEXT: COPY t_on_error_null, line 1, column a: null input

It might be better to consider standardizing casing across all COPY
statements (e.g., COPY ... TO, COPY ... FROM STDIN) for consistency.

--
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA Japan Corporation to SRA OSS K.K.