From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: torikoshia <torikoshia@oss.nttdata.com>
To: zhjwpku@gmail.com
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Add new COPY option REJECT_LIMIT
Date: Tue, 23 Jul 2024 13:35:40 +0900
Message-ID: <644a094c-2c0c-4b5f-b151-3677c7de4ee7@oss.nttdata.com> (raw)
In-Reply-To: <f9feda0ecd4e2e09620a6f9ca8f4aac1@oss.nttdata.com>
References: <63f99327aa6b404cc951217fa3e61fe4@oss.nttdata.com>
<fab7018f-6e00-4be4-bd1e-4a013096f6c9@oss.nttdata.com>
<de0551d0f9d9e9072e324e51ed5c426d@oss.nttdata.com>
<6e3dcdc6-8e18-4716-a23b-3bee316aeaa7@oss.nttdata.com>
<979c6d306343f3d665d14eab2cd1b7bc@oss.nttdata.com>
<920a29a36032befbeb355cbdfbbe0f63@oss.nttdata.com>
<a873643c-e7d1-456c-b16b-6162ab698036@oss.nttdata.com>
<54bea7dd-94b1-41f1-a4e9-c5c53cc82ce7@oss.nttdata.com>
<f9feda0ecd4e2e09620a6f9ca8f4aac1@oss.nttdata.com>
On 2024/07/22 21:37, torikoshia wrote:
> On Fri, Jul 19, 2024 at 11:48 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> Thanks for the comment.
>
>> In patch 0002, the ratio is calculated by the already skipped/processed
>> rows, but what if a user wants to copy 1000 rows, and he/she can tolerate
>> 10 error rows, so he/she might set *reject_limit 0.01*, but one bad row in the
>> first 100 rows will fail the entire command, this might surprise the user.
>
> Since the ratio is calculated after all data is processed, the case "one bad row in the first 100 rows will fail the entire command" doesn't happen:
Yes, but is this the desired behavior when using the ratio threshold?
I was thinking that COPY should fail once the error ratio (errors vs.
total rows in the input file) reaches the threshold. Since it's
difficult to know the total number of rows in the input file,
implementing this seems not easy, though.
> Updated the patch.
Thanks for updating the patch!
+ This option must be used with <literal>ON_ERROR</literal> to be set to
+ other than <literal>stop</literal>.
Regarding the ON_ERROR option, now it only has two values.
Instead of saying "other than stop," we should simply use "ignore"
for clarity and intuition?
+ When specified <literal>INFINITY</literal>, <command>COPY</command> ignores all
+ the errors. This is a synonym for <literal>ON_ERROR</literal> <literal>ignore</literal>.
For the INFINITY value, the description is a bit unclear.
As I understand it, INFINITY is the default for REJECT_LIMIT.
So, if ON_ERROR=ignore is set without specifying REJECT_LIMIT,
COPY will ignore all errors, similar to when REJECT_LIMIT=INFINITY is used.
In line with my previous suggestion, if we support only REJECT_LIMIT
without ON_ERROR, having INFINITY makes sense. However,
with ON_ERROR supported, REJECT_LIMIT=INFINITY seems redundant.
Users can just set ON_ERROR=ignore to ignore all errors,
making INFINITY unnecessary for REJECT_LIMIT. This is open for
discussion, but I believe it's better to remove INFINITY from
the REJECT_LIMIT options.
+ else if (strcmp(defel->defname, "reject_limit") == 0)
+ {
+ if (reject_limit_specified)
+ errorConflictingDefElem(defel, pstate);
+ if (!opts_out->on_error)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("REJECT_LIMIT requires ON_ERROR to be set to other than stop")));
Using "ignore" instead of "other than stop" in the error message
is clearer and more intuitive.
Checking if ON_ERROR and REJECT_LIMIT are specified should be
done after the foreach() processing of the options. Otherwise,
if REJECT_LIMIT is specified before ON_ERROR, unexpected errors can occur.
---------------
=# copy test from '...' WITH (REJECT_LIMIT 7, ON_ERROR 'ignore');
ERROR: REJECT_LIMIT requires ON_ERROR to be set to other than stop
---------------
+ ereport(ERROR,
+ (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+ errmsg("exceeded the number specified by REJECT_LIMIT \"%lld\"",
+ (long long) cstate->opts.reject_limits.num_err)));
The error message isn't clear about what exceeded REJECT_LIMIT.
How about saying "skipped more than REJECT_LIMIT rows" or something instead?
+/*
+ * A struct to hold reject_limit options, in a parsed form.
+ * More values to be added in another patch.
+ */
The latter comment doesn't seem necessary or helpful.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, torikoshia@oss.nttdata.com, zhjwpku@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Add new COPY option REJECT_LIMIT
In-Reply-To: <644a094c-2c0c-4b5f-b151-3677c7de4ee7@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox