MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Tue, 01 Jul 2025 23:54:15 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: jian he <jian.universality@gmail.com>
Cc: 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>
Subject: Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
In-Reply-To: 
 <CACJufxGEHmijmP-QqvrmqU6cxmhgpdjY7ewQBQ=E9NmdyEcqmw@mail.gmail.com>
References: 
 <CAKFQuwawy1e6YR4S=j+y7pXqg_Dw1WBVrgvf=BP3d1_aSfe_+Q@mail.gmail.com>
 <CACJufxGsGqU0BnHw9+Vk3KhBSA3951fZs+JQnM387CiJD3Qn+g@mail.gmail.com>
 <CALdSSPh13qiKmhqwj=bR_3seZJVkP9E6BDbLfHXQ_DbaxQL4FA@mail.gmail.com>
 <CACJufxEq9aQX6ddkHeEX-RqjTZfDF02BLWGVS28ixRQQxoz0LA@mail.gmail.com>
 <CALdSSPjiR8cqPrJOjoGPvTnDufuWYj94-1whCGtGox=_QeZQEA@mail.gmail.com>
 <CACJufxEKq25CvQj4rYxcvHC_0MZQxzfwEKVWo9i-bKKxbVw3BA@mail.gmail.com>
 <CALdSSPgcuW1omVFUpnL6H63H+2qvtvEkD+4H3jUh1iEaNn_nEg@mail.gmail.com>
 <CACJufxFTvtMy_aYFQPtcBwVwJ-2=rJRVE0SBWv6efv86J5DyAQ@mail.gmail.com>
 <CALdSSPhxprTyzzr4tk-QU-GUroBKYNuJJ2GxWnExbB=bjQjJ7w@mail.gmail.com>
 <CACJufxHpTC-kqmW0LKNd_ZgnPJkhiAebPQkX2YE-CVxBP4LDfg@mail.gmail.com>
 <CALdSSPjpi0w6US75_st_-zOdUfr5q2vXmh08x0pL4FGJGYZvsQ@mail.gmail.com>
 <CACJufxE420KH=SLQSMEmi8QfEhXgVL8kGkuZJTviZ0o=TEVQgw@mail.gmail.com>
 <90dc6e9d-9348-485a-b27c-7b1637f06c2e@uni-muenster.de>
 <CACJufxFej2xUCERoWU+Eb+R+L_xzMNq4ZERNtL=SPF8O0T0ygw@mail.gmail.com>
 <cb26ae01-74d5-47b8-b257-c5e396339b2c@uni-muenster.de>
 <CACJufxHj0PejJC8BMq1cCiFC5uFdnazqrHh_VEm=s65-cNSnwQ@mail.gmail.com>
 <CALDaNm0Q3hA9w76gvB=goOY4ktKb-wjrbZbOcrqSwvxdm0yrcw@mail.gmail.com>
 <CACJufxEueK_MFebYqp73e+P+ykbD4+QbONptMg=iaaxUEGU7EQ@mail.gmail.com>
 <CALDaNm0FUtPjH9wz5dgwOBNtsAXEZj=0-TYGsyQXchQy-hXGrw@mail.gmail.com>
 <CACJufxFpm2Gzx4AD9qKtiQSiiiunX02wNTxu0JoFm7nEKF2KUw@mail.gmail.com>
 <CAD21AoBjmEYjZT9A6P-vAuwboiiYtXaMc12pX2ySmh3RXi=v8w@mail.gmail.com>
 <CACJufxF6_YwAboiCaVYLRtNpO4kGbXqkXzH_7W=pUvrNXK8WuQ@mail.gmail.com>
 <CAD21AoBWwEzpp3Z6tp-O-AQGftK-kuj6vRva2L5daWoWBtbnRg@mail.gmail.com>
 <CACJufxGEHmijmP-QqvrmqU6cxmhgpdjY7ewQBQ=E9NmdyEcqmw@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <f5ebfa4b318e97c41c0919304b94c77f@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/f5ebfa4b318e97c41c0919304b94c77f%40oss.nttdata.com>
Precedence: bulk

Hi,

Thanks for updating the patch and I've read 
v17-0001-COPY-on_error-set_null.patch and here are some comments.

> +COPY x from stdin (on_error set_null, reject_limit 2);
> +ERROR:  COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE

I understand that REJECT_LIMIT is out of scope for this patch, but 
personally, I feel that supporting REJECT_LIMIT with ON_ERROR SET_NULL 
would be a natural extension.
- Both IGNORE and SET_NULL share the common behavior of allowing COPY to 
continue despite soft errors.
- Since REJECT_LIMIT defines the threshold for how many soft errors can 
be tolerated before COPY fails, it seems consistent to allow it with 
SET_NULL as well.


+       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?


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


-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA Japan Corporation to SRA OSS K.K.