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.