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

From: jian he <jian(dot)universality(at)gmail(dot)com>
To: Matheus Alcantara <matheusssilv97(at)gmail(dot)com>
Cc: torikoshia <torikoshia(at)oss(dot)nttdata(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>, Kirill Reshke <reshkekirill(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
Date: 2026-02-09 03:59:46
Message-ID: CACJufxGUP09S7a2akQ4GD62-Rf2bcV52cqnw4qqBOXcn9bmxCA@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, Feb 6, 2026 at 8:58 PM Matheus Alcantara
<matheusssilv97(at)gmail(dot)com> 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.

> 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.

inspired by your change, I further simplified the error handling code.

--
jian
https://www.enterprisedb.com/

Attachment Content-Type Size
v23-0001-COPY-on_error-set_null.patch text/x-patch 23.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Hayato Kuroda (Fujitsu) 2026-02-09 05:15:17 RE: [WIP] Pipelined Recovery
Previous Message jian he 2026-02-09 03:48:09 Re: Emitting JSON to file using COPY TO