On 09/02/26 00:59, jian he wrote:
Thanks, overall the patch looks good to me. I'm attaching a diff with
just some small tweaks on documentation and error messages. Please see
and check if it's make sense.
In the function CopyFrom, we have:
if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
cstate->escontext->error_occurred)
{
cstate->escontext->error_occurred = false;
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
cstate->num_errors);
That means PROGRESS_COPY_TUPLES_SKIPPED applied for COPY_ON_ERROR_IGNORE only.
So
<para>
Number of tuples skipped because they contain malformed data.
This counter only advances when
<literal>ignore</literal> is specified to the
<literal>ON_ERROR</literal>
option.
</para></entry>
should be ok.
Ok, agree.
I'm wondering if we should have an else if block on
CopyFromTextLikeOneRow() when cstate->cur_attval is NULL to handle
COPY_ON_ERROR_SET_NULL when log_verbosity is set to
COPY_LOG_VERBOSITY_VERBOSE
if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)
ereport(NOTICE,
errmsg("skipping row due to data type incompatibility at line %" PRIu64 " for
column \"%s\": null input",
cstate->cur_lineno,
cstate->cur_attname));
+ else if (cstate->opts.on_error == COPY_ON_ERROR_SET_NULL)
+ ereport(NOTICE,
+ errmsg("setting to null due to data type incompatibility at line %" PRIu64 " for
column \"%s\": null input",
+ cstate->cur_lineno,
+ cstate->cur_attname));
CopyFromTextLikeOneRow, we have:
cstate->cur_attname = NameStr(att->attname);
cstate->cur_attval = string;
even if "string" is NULL (two InputFunctionCallSafe function call with
"str" value as NULL), it will fail at
```
else if (string == NULL)
ereport(ERROR,
errcode(ERRCODE_NOT_NULL_VIOLATION),
errmsg("null value in column \"%s\"
violates not-null constraint of domain %s",
cstate->cur_attname,
format_type_be(typioparams[m])),
errdatatype(typioparams[m]));
```
so i think condition like:
if (cstate->opts.log_verbosity == COPY_LOG_VERBOSITY_VERBOSE &&
cstate->cur_attval == NULL &&
cstate->opts.on_error == COPY_ON_ERROR_SET_NULL)
is not reachable.
therefore I didn't add the ELSE IF block.
Ok, make sense. I've tested and it seems correct.
inspired by your change, I further simplified the error handling code.
Thanks for the new version. It looks good to me. I don't have any
other comments.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
