Message-ID: <976f11ca-ea15-47b4-a0cc-8828f12e35ec@gmail.com>
Date: Mon, 9 Feb 2026 12:36:55 -0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
To: jian he <jian.universality@gmail.com>
Cc: torikoshia <torikoshia@oss.nttdata.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>, vignesh C <vignesh21@gmail.com>,
 Jim Jones <jim.jones@uni-muenster.de>, Kirill Reshke
 <reshkekirill@gmail.com>, Fujii Masao <masao.fujii@oss.nttdata.com>,
 "David G. Johnston" <david.g.johnston@gmail.com>,
 Yugo NAGATA <nagata@sraoss.co.jp>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <CAKFQuwawy1e6YR4S=j+y7pXqg_Dw1WBVrgvf=BP3d1_aSfe_+Q@mail.gmail.com>
 <CACJufxF6_YwAboiCaVYLRtNpO4kGbXqkXzH_7W=pUvrNXK8WuQ@mail.gmail.com>
 <CAD21AoBWwEzpp3Z6tp-O-AQGftK-kuj6vRva2L5daWoWBtbnRg@mail.gmail.com>
 <CACJufxGEHmijmP-QqvrmqU6cxmhgpdjY7ewQBQ=E9NmdyEcqmw@mail.gmail.com>
 <f5ebfa4b318e97c41c0919304b94c77f@oss.nttdata.com>
 <CACJufxG=em0PHZvy1EAZ+vxPZ8UA68MfQ-Hji+h+WgnWNpqmVQ@mail.gmail.com>
 <CACJufxF0c3k5O8up9NOY-m02nyJ0f6N1tKxZwjCewTqvvFmbLw@mail.gmail.com>
 <CACJufxHr-LBV2pB9m64mA=1pgVMM4LvUn+-MzpeViWV=Ks_cyg@mail.gmail.com>
 <901967e5-e5dc-42c6-b2bf-fb3a49d7e787@gmail.com>
 <CACJufxEs_VBV39gYHnpLFOMcUaUD-ADAst_ePTCgmoDR8O=ekg@mail.gmail.com>
 <DFUHPCSP86G0.11VU7FGWRRXM2@gmail.com>
 <CACJufxGYPXQ_Jz1avF5eSh_XJRsxhPSUZ+=RzG3Hz4_XNAc32g@mail.gmail.com>
 <4c540fe3-495c-4bbf-8dcf-2c1e2b88bc3d@gmail.com>
 <CACJufxHFwQMw1As+QFk+fA7S8ZxRG2wOvHcvmsWuj2XJ+W6d_A@mail.gmail.com>
 <DG7WARDX1TY0.2VX3Q7QHSCXE9@gmail.com>
 <CACJufxGUP09S7a2akQ4GD62-Rf2bcV52cqnw4qqBOXcn9bmxCA@mail.gmail.com>
Content-Language: en-US
From: Matheus Alcantara <matheusssilv97@gmail.com>
In-Reply-To: <CACJufxGUP09S7a2akQ4GD62-Rf2bcV52cqnw4qqBOXcn9bmxCA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/976f11ca-ea15-47b4-a0cc-8828f12e35ec%40gmail.com>
Precedence: bulk

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