From: torikoshia <torikoshia@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: zhjwpku@gmail.com
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Add new COPY option REJECT_LIMIT
Date: Tue, 24 Sep 2024 14:25:47 +0900
Message-ID: <056a5728cf733685cff7d9b5a8fbb0ad@oss.nttdata.com> (raw)
In-Reply-To: <5f807fcf3a36df7ba41464ab40b5c37d@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>
<644a094c-2c0c-4b5f-b151-3677c7de4ee7@oss.nttdata.com>
<5f807fcf3a36df7ba41464ab40b5c37d@oss.nttdata.com>
Updated the patch.
On 2024-07-23 23:06, torikoshia wrote:
> On Tue, Jul 23, 2024 at 1:35 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
>
> Thanks for your review.
>
>> 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.
>
> Users might expect like you.
> However, implementing it would need a row number counting feature as
> you pointed out, and it seems an overkill.
>
> Describing the difference between ratio and number in the manual might
> help, but
> it might be better to make REJECT_LIMIT support only the number of
> errors and leave it to the user to calculate the number from the
> ratio.
>
> I'd like to hear if anyone has an opinion on the need for supporting
> ratio.
Since there was no opinion about it, attached a patch only for
REJECT_LIMIT specifying number.
>> + 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?
>
> I'll Modify it.
> The reason for the roundabout wording was the expectation that
> on_error would support values other than these in the future, but as
> you point out, there are currently only two.
>
>
>> + 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.
>
> Agreed.
> Unless there are opposing opinions, I'm going to remove 'INFINITY' in
> the next patch.
As there are no opposing opinions, removed 'INFINITY' as well.
>> + 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.
>
> Agreed.
>
>
>> 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
>> ---------------
>
> Ugh, I'll modify it.
>
>> + 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?
>
> Agreed.
>
>> +/*
>> + * 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.
>
> Agreed.
>
> [1]
> https://www.postgresql.org/message-id/CAKFQuwYP91_G6tktYFTZq_CmkZ_%3DzuWjkz1%2B25Nd8bpsrDkx5Q%40mail...
On Tue, Jul 23, 2024 at 11:10 PM torikoshia <torikoshia@oss.nttdata.com>
wrote:
> On 2024-07-23 02:06, Kirill Reshke wrote:
>> If nothing is specified, then the maximum tolerable number of errors
>> is one, right? Should we state this explicitly in the documentation?
> REJECT_LIMIT now can be used wonly when on_error=ignore, I think the
> default(when only on_error=ignore is specified) is unlimited.
> Anyway, I'm going to add a description about the default.
Added the following description:
+ Just setting <literal>ON_ERROR</literal> to
<literal>ignore</literal>
+ tolerates unlimited number of errors.
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
Attachments:v4-0001-Add-new-COPY-option-REJECT_LIMIT-number.patch (text/x-diff, 8.6K) download
From 0e92a90bd9f944b7f14845e942147d11f81e3b6f Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Tue, 24 Sep 2024 14:11:22 +0900
Subject: [PATCH v4] Add new COPY option REJECT_LIMIT number
9e2d870 enabled the COPY FROM command to skip soft errors, but there
was no limitation and skipped all the soft errors.
In some cases, there would be a limitation how many errors are
tolerable. This patch introduces REJECT_LIMIT to specify how many
errors can be skipped.
---
doc/src/sgml/ref/copy.sgml | 20 ++++++++++++++
src/backend/commands/copy.c | 41 +++++++++++++++++++++++++++++
src/backend/commands/copyfrom.c | 5 ++++
src/include/commands/copy.h | 1 +
src/test/regress/expected/copy2.out | 10 +++++++
src/test/regress/sql/copy2.sql | 21 +++++++++++++++
6 files changed, 98 insertions(+)
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgmlindex 1518af8a04..3244384b1a 100644--- a/doc/src/sgml/ref/copy.sgml+++ b/doc/src/sgml/ref/copy.sgml@@ -44,6 +44,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
FORCE_NOT_NULL { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * }
FORCE_NULL { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * }
ON_ERROR <replaceable class="parameter">error_action</replaceable>
+ REJECT_LIMIT { <replaceable class="parameter">integer</replaceable> }
ENCODING '<replaceable class="parameter">encoding_name</replaceable>'
LOG_VERBOSITY <replaceable class="parameter">verbosity</replaceable>
</synopsis>
@@ -411,6 +412,25 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
</listitem>
</varlistentry>
+ <varlistentry>+ <term><literal>REJECT_LIMIT</literal></term>+ <listitem>+ <para>+ When a positive integer value is specified, <command>COPY</command> limits+ the maximum tolerable number of errors while converting a column's input+ value into its data type.+ If input data caused more errors than the specified value, entire+ <command>COPY</command> fails.+ Otherwise, <command>COPY</command> discards the input row and continues+ with the next one.+ This option must be used with <literal>ON_ERROR</literal> to be set to+ <literal>ignore</literal>.+ Just setting <literal>ON_ERROR</literal> to <literal>ignore</literal>+ tolerates unlimited number of errors.+ </para>+ </listitem>+ </varlistentry>+
<varlistentry>
<term><literal>ENCODING</literal></term>
<listitem>
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.cindex 3bb579a3a4..91e01dbc9f 100644--- a/src/backend/commands/copy.c+++ b/src/backend/commands/copy.c@@ -418,6 +418,30 @@ defGetCopyOnErrorChoice(DefElem *def, ParseState *pstate, bool is_from)
return COPY_ON_ERROR_STOP; /* keep compiler quiet */
}
+/*+ * Extract REJECT_LIMIT value from a DefElem.+ */+static int64+defGetCopyRejectLimitOptions(DefElem *def)+{+ int64 reject_limit;++ if (nodeTag(def->arg) == T_Integer)+ {+ reject_limit = defGetInt64(def);+ if (reject_limit <= 0)+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),+ errmsg("number for REJECT_LIMIT must be greater than zero")));+ }+ else+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),+ errmsg("value for REJECT_LIMIT must be positive integer")));++ return reject_limit;+}+
/*
* Extract a CopyLogVerbosityChoice value from a DefElem.
*/
@@ -470,6 +494,7 @@ ProcessCopyOptions(ParseState *pstate,
bool header_specified = false;
bool on_error_specified = false;
bool log_verbosity_specified = false;
+ bool reject_limit_specified = false;
ListCell *option;
/* Support external use for option sanity checking */
@@ -636,6 +661,13 @@ ProcessCopyOptions(ParseState *pstate,
log_verbosity_specified = true;
opts_out->log_verbosity = defGetCopyLogVerbosityChoice(defel, pstate);
}
+ else if (strcmp(defel->defname, "reject_limit") == 0)+ {+ if (reject_limit_specified)+ errorConflictingDefElem(defel, pstate);+ reject_limit_specified = true;+ opts_out->reject_limit = defGetCopyRejectLimitOptions(defel);+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -669,6 +701,15 @@ ProcessCopyOptions(ParseState *pstate,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("only ON_ERROR STOP is allowed in BINARY mode")));
+ if (opts_out->reject_limit && !opts_out->on_error)+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),+ /*- translator: first and second %s are the names of COPY+ * option, e.g. ON_ERROR, thrid is the value of the COPY option,+ * e.g. IGNORE */+ errmsg("COPY %s requires %s to be set to %s",+ "REJECT_LIMIT", "ON_ERROR", "IGNORE")));+
/* Set defaults for omitted options */
if (!opts_out->delim)
opts_out->delim = opts_out->csv_mode ? "," : "\t";
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.cindex 2d3462913e..c17fbf71bc 100644--- a/src/backend/commands/copyfrom.c+++ b/src/backend/commands/copyfrom.c@@ -1024,6 +1024,11 @@ CopyFrom(CopyFromState cstate)
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
++skipped);
+ if (cstate->opts.reject_limit && skipped > cstate->opts.reject_limit)+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),+ errmsg("skipped more than REJECT_LIMIT rows: \"%lld\",",+ (long long) cstate->opts.reject_limit)));
continue;
}
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.hindex 141fd48dc1..c3a7613778 100644--- a/src/include/commands/copy.h+++ b/src/include/commands/copy.h@@ -83,6 +83,7 @@ typedef struct CopyFormatOptions
bool convert_selectively; /* do selective binary conversion? */
CopyOnErrorChoice on_error; /* what to do when error happened */
CopyLogVerbosityChoice log_verbosity; /* verbosity of logged messages */
+ int64 reject_limit; /* maximum tolerable number of errors */
List *convert_select; /* list of column names (can be NIL) */
} CopyFormatOptions;
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.outindex 61a19cdc4c..f63228f5bc 100644--- a/src/test/regress/expected/copy2.out+++ b/src/test/regress/expected/copy2.out@@ -116,6 +116,10 @@ COPY x to stdout (log_verbosity unsupported);
ERROR: COPY LOG_VERBOSITY "unsupported" not recognized
LINE 1: COPY x to stdout (log_verbosity unsupported);
^
+COPY x from stdin with (reject_limit 1);+ERROR: COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE+COPY x from stdin with (on_error ignore, reject_limit 0);+ERROR: number for REJECT_LIMIT must be greater than zero
-- too many columns in column list: should fail
COPY x (a, b, c, d, e, d, c) from stdin;
ERROR: column "d" specified more than once
@@ -789,6 +793,12 @@ CONTEXT: COPY check_ign_err, line 1: "1 {1}"
COPY check_ign_err FROM STDIN WITH (on_error ignore);
ERROR: extra data after last expected column
CONTEXT: COPY check_ign_err, line 1: "1 {1} 3 abc"
+-- tests for reject_limit option+COPY check_ign_err FROM STDIN WITH (on_error ignore, reject_limit 3);+ERROR: skipped more than REJECT_LIMIT rows: "3",+CONTEXT: COPY check_ign_err, line 5, column n: ""+COPY check_ign_err FROM STDIN WITH (on_error ignore, reject_limit 4);+NOTICE: 4 rows were skipped due to data type incompatibility
-- clean up
DROP TABLE forcetest;
DROP TABLE vistest;
diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sqlindex 8b14962194..2d775d9c97 100644--- a/src/test/regress/sql/copy2.sql+++ b/src/test/regress/sql/copy2.sql@@ -82,6 +82,8 @@ COPY x to stdout (format TEXT, force_null(a));
COPY x to stdin (format CSV, force_null(a));
COPY x to stdin (format BINARY, on_error unsupported);
COPY x to stdout (log_verbosity unsupported);
+COPY x from stdin with (reject_limit 1);+COPY x from stdin with (on_error ignore, reject_limit 0);
-- too many columns in column list: should fail
COPY x (a, b, c, d, e, d, c) from stdin;
@@ -557,6 +559,25 @@ COPY check_ign_err FROM STDIN WITH (on_error ignore);
1 {1} 3 abc
\.
+-- tests for reject_limit option+COPY check_ign_err FROM STDIN WITH (on_error ignore, reject_limit 3);+6 {6} 6+a {7} 7+8 {8} 8888888888+9 {a, 9} 9++10 {10} 10+\.++COPY check_ign_err FROM STDIN WITH (on_error ignore, reject_limit 4);+6 {6} 6+a {7} 7+8 {8} 8888888888+9 {a, 9} 9++10 {10} 10+\.+
-- clean up
DROP TABLE forcetest;
DROP TABLE vistest;
base-commit: bbba59e69a56e1622e270f5e47b402c3a904cefc
--
2.39.2
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: torikoshia@oss.nttdata.com, masao.fujii@oss.nttdata.com, zhjwpku@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Add new COPY option REJECT_LIMIT
In-Reply-To: <056a5728cf733685cff7d9b5a8fbb0ad@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