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

On Tue, Jul 1, 2025 at 10:54 PM torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> wrote:
>
> Hi,
>
> Thanks for updating the patch and I've read
> v17-0001-COPY-on_error-set_null.patch and here are some comments.
>
> + 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?
>

hi.
I changed this comment, also heavily refactored CopyFromTextLikeOneRow based on
v17-0001-COPY-on_error-set_null.patch.
Now it looks way more intuitive, IMHO.

CopyFromTextLikeOneRow
else if (!InputFunctionCallSafe(&in_functions[m],
string,
typioparams[m],
att->atttypmod,
(Node *) cstate->escontext,
&values[m]))
{
if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)
////code for on_errr ignore
else if (cstate->opts.on_error == COPY_ON_ERROR_SET_NULL)
////code for on_errr set_null

if (cstate->opts.log_verbosity == COPY_LOG_VERBOSITY_VERBOSE)
//code for verbose message for on_error ignore or on_error set_null

if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)
////code for on_errr ignore loop control
else if (cstate->opts.on_error == COPY_ON_ERROR_SET_NULL)
////code for on_errr set_null loop control
}

> +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.
>
I followed near code conventions, changing the casing here seems not necessary.