On Sun, Jan 14, 2024 at 10:35 PM Masahiko Sawada <[email protected]> wrote: > Thank you for updating the patch. Here are two comments: > > --- > + if (cstate->opts.save_error_to != COPY_SAVE_ERROR_TO_UNSPECIFIED && > + cstate->num_errors > 0) > + ereport(WARNING, > + errmsg("%zd rows were skipped due to data type > incompatibility", > + cstate->num_errors)); > + > /* Done, clean up */ > error_context_stack = errcallback.previous; > > If a malformed input is not the last data, the context message seems odd: > > postgres(1:1769258)=# create table test (a int); > CREATE TABLE > postgres(1:1769258)=# copy test from stdin (save_error_to none); > Enter data to be copied followed by a newline. > End with a backslash and a period on a line by itself, or an EOF signal. > >> a > >> 1 > >> > 2024-01-15 05:05:53.980 JST [1769258] WARNING: 1 rows were skipped > due to data type incompatibility > 2024-01-15 05:05:53.980 JST [1769258] CONTEXT: COPY test, line 3: "" > COPY 1 > > I think it's better to report the WARNING after resetting the > error_context_stack. Or is a WARNING really appropriate here? The > v15-0001-Make-COPY-FROM-more-error-tolerant.patch[1] uses NOTICE but > the v1-0001-Add-new-COPY-option-SAVE_ERROR_TO.patch[2] changes it to > WARNING without explanation.
Thank you for noticing this. I think NOTICE is more appropriate here.
There is nothing to "worry" about: the user asked to ignore the errors
and we did. And yes, it doesn't make sense to use the last line as
the context. Fixed.
> ---
> +-- test missing data: should fail
> +COPY check_ign_err FROM STDIN WITH (save_error_to none);
> +1 {1}
> +\.
>
> We might want to cover the extra data cases too.
Agreed, the relevant test is added.
------
Regards,
Alexander Korotkov
0001-Add-new-COPY-option-SAVE_ERROR_TO-v4.patch
Description: Binary data
