From: torikoshia <torikoshia@oss.nttdata.com>
To: Yugo NAGATA <nagata@sraoss.co.jp>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Pgsql Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add reject_limit option to file_fdw
Date: Tue, 19 Nov 2024 21:40:30 +0900
Message-ID: <a7d70e35229e2075b371cd1e81222244@oss.nttdata.com> (raw)
In-Reply-To: <20241113231706.09e5b5ea9640289312835be8@sraoss.co.jp>
References: <bab68a9fc502b12693f0755b6f35f327@oss.nttdata.com>
<fd14c3a0-a036-4bce-b5f8-4af83259eae1@oss.nttdata.com>
<54747389a562688eab7077b144daff6a@oss.nttdata.com>
<2b3ad6d4-e05f-4188-8cfe-1fd9d949a9c9@oss.nttdata.com>
<192ede1e4c2b34b853e5ac46f8dee50b@oss.nttdata.com>
<d7a03664-bef5-49b0-998f-3ea5356b53c9@oss.nttdata.com>
<f02ec7b6c348b9fbeeb69657733da9ba@oss.nttdata.com>
<20241112145114.ba288defc684699bf8646942@sraoss.co.jp>
<2f03bf95556c4c41d6b1659e74b346c7@oss.nttdata.com>
<20241113231706.09e5b5ea9640289312835be8@sraoss.co.jp>
On 2024-11-13 23:17, Yugo NAGATA wrote:
> On Wed, 13 Nov 2024 21:48:10 +0900
> torikoshia <torikoshia@oss.nttdata.com> wrote:
>
>> On 2024-11-12 14:51, Yugo Nagata wrote:
>>
>> Thanks for your review!
>>
>> > On Tue, 12 Nov 2024 10:16:50 +0900
>> > torikoshia <torikoshia@oss.nttdata.com> wrote:
>> >
>> >> On 2024-11-12 01:49, Fujii Masao wrote:
>> >> > On 2024/11/11 21:45, torikoshia wrote:
>> >> >>> Thanks for adding the comment. It clearly states that REJECT_LIMIT
>> >> >>> can be
>> >> >>> a single-quoted string. However, it might also be helpful to mention
>> >> >>> that
>> >> >>> it can be provided as an int64 in the COPY command option. How about
>> >> >>> updating it like this?
>> >> >>>
>> >> >>> ------------------------------------
>> >> >>> REJECT_LIMIT can be specified in two ways: as an int64 for the COPY
>> >> >>> command
>> >> >>> option or as a single-quoted string for the foreign table option
>> >> >>> using
>> >> >>> file_fdw. Therefore this function needs to handle both formats.
>> >> >>> ------------------------------------
>> >> >>
>> >> >> Thanks! it seems better.
>> >> >>
>> >> >>
>> >> >> Attached v3 patch.
>> >> >
>> >> > Thanks for updating the patch! It looks like you forgot to attach it,
>> >> > though.
>> >>
>> >> Oops, thanks for pointing it out.
>> >> Here it is.
>> >
>> > I have one minor comment.
>> >
>> > + ereport(ERROR,
>> > + (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>> > + errmsg("skipped more than REJECT_LIMIT (%lld) rows due to data
>> > type incompatibility",
>> > + (long long) cstate->opts.reject_limit)));
>> >
>> > Do we have to keep this message consistent with ones of COPY command?
>> > With foreign data wrappers, it seems common that the option name is
>> > passed in
>> > lowercase, so is it better to use "skipped more than reject_limit ..."
>> > rather
>> > than using uppercase?
>>
>> Agreed.
>> Attached v4 patch.
>
>
> Thank you for updating the patch.
>
> However, I noticed that file_fdw could output the following error
> using uppercase
> when an invalid value is set to the reject_limit option,
>
> ALTER FOREIGN TABLE agg_bad OPTIONS (ADD reject_limit '-1');
> ERROR: REJECT_LIMIT (-1) must be greater than zero
>
> as well as if reject_limit is set when on_error != 'ignore'.
>
> ALTER FOREIGN TABLE agg_bad OPTIONS (ADD reject_limit '1'); -- ERROR
> ERROR: COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE
>
> Also, in the regression tests, I found similar behaviours on existing
> options,
> for example;
>
> CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format
> 'xml'); -- ERROR
> ERROR: COPY format "xml" not recognized
> CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format
> 'text', quote ':'); -- ERROR
> ERROR: COPY QUOTE requires CSV mode
> CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format
> 'text', escape ':'); -- ERROR
> ...
>
> These messages may be unexpected for some users because the
> documentation of
> fild_fdw does not explicitly describe that file_fdw uses COPY
> internally.
> (I can find several wordings like "as COPY", though.)
> However, since the current file_fdw already has such messages, I came
> to think
> making the messages on "reject_limit" consistent with COPY is
> reasonable.
> I mean, the previous version of the message seems fine.
Agreed. Thanks for your investigation.
> As another comment, do we need to add validator test for on_error and
> reject_limit as similar to other options?
That might be better.
Added some queries which had wrong options to cause errors.
I was unsure whether to add it to "validator test" section or "on_error,
log_verbosity and reject_limit tests" section, but I chose "validator
test" as I believe it makes things clearer.
Added 0002 patch since some of the tests are not related to reject_limit
but just on_error.
--
Regards,
--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.
Attachments:download all (2 files, .tar.gz)v5-0001-file_fdw-Add-reject_limit-option-to-file_fdw.patch (text/x-diff, 7.1K) download
From 661937869d696bc640ff54c97631c27671502e60 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Tue, 19 Nov 2024 21:06:37 +0900
Subject: [PATCH v5 1/2] file_fdw: Add reject_limit option to file_fdw
Commit 4ac2a9bece introduced REJECT_LIMIT option for COPY. This patch
extends support for this option to file_fdw.
As well as REJECT_LIMIT option for COPY, this option limits the
maximum number of erroneous rows that can be skipped.
If more rows encounter data type conversion errors than allowed by
reject_limit, the access to the file_fdw foreign table will fail
with an error, even when on_error = 'ignore'.
Based on the synopsis of the CREATE/ALTER FOREIGN TABLE commands, the
value for the foreign table's option must be single-quoted.
To accommodate this, the patch modifies defGetCopyRejectLimitOption
to accept not only int64 but also strings.
---
contrib/file_fdw/data/agg.bad | 1 +
contrib/file_fdw/expected/file_fdw.out | 18 ++++++++++++++++--
contrib/file_fdw/file_fdw.c | 8 ++++++++
contrib/file_fdw/sql/file_fdw.sql | 7 ++++++-
doc/src/sgml/file-fdw.sgml | 12 ++++++++++++
src/backend/commands/copy.c | 16 +++++++++++++++-
6 files changed, 58 insertions(+), 4 deletions(-)
diff --git a/contrib/file_fdw/data/agg.bad b/contrib/file_fdw/data/agg.badindex 3415b15007..04279ce55b 100644--- a/contrib/file_fdw/data/agg.bad+++ b/contrib/file_fdw/data/agg.bad@@ -2,3 +2,4 @@
100;@99.097@
0;@aaa@
42;@324.78@
+1;@bbb@diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.outindex 593fdc782e..4f63c047ec 100644--- a/contrib/file_fdw/expected/file_fdw.out+++ b/contrib/file_fdw/expected/file_fdw.out@@ -90,6 +90,8 @@ ERROR: COPY delimiter cannot be newline or carriage return
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null '
'); -- ERROR
ERROR: COPY null representation cannot use newline or carriage return
+CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (reject_limit '1'); -- ERROR+ERROR: COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE
CREATE FOREIGN TABLE tbl () SERVER file_server; -- ERROR
ERROR: either filename or program is required for file_fdw foreign tables
\set filename :abs_srcdir '/data/agg.data'
@@ -206,10 +208,10 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
SELECT * FROM agg_bad; -- ERROR
ERROR: invalid input syntax for type real: "aaa"
CONTEXT: COPY agg_bad, line 3, column b: "aaa"
--- on_error and log_verbosity tests+-- on_error, log_verbosity and reject_limit tests
ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
SELECT * FROM agg_bad;
-NOTICE: 1 row was skipped due to data type incompatibility+NOTICE: 2 rows were skipped due to data type incompatibility
a | b
-----+--------
100 | 99.097
@@ -224,6 +226,18 @@ SELECT * FROM agg_bad;
42 | 324.78
(2 rows)
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD reject_limit '1'); -- ERROR+SELECT * FROM agg_bad;+ERROR: skipped more than REJECT_LIMIT (1) rows due to data type incompatibility+CONTEXT: COPY agg_bad, line 5, column b: "bbb"+ALTER FOREIGN TABLE agg_bad OPTIONS (SET reject_limit '2');+SELECT * FROM agg_bad;+ a | b +-----+--------+ 100 | 99.097+ 42 | 324.78+(2 rows)+
ANALYZE agg_bad;
-- misc query tests
\t on
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.cindex 043204c3e7..9e2896f32a 100644--- a/contrib/file_fdw/file_fdw.c+++ b/contrib/file_fdw/file_fdw.c@@ -77,6 +77,7 @@ static const struct FileFdwOption valid_options[] = {
{"encoding", ForeignTableRelationId},
{"on_error", ForeignTableRelationId},
{"log_verbosity", ForeignTableRelationId},
+ {"reject_limit", ForeignTableRelationId},
{"force_not_null", AttributeRelationId},
{"force_null", AttributeRelationId},
@@ -788,6 +789,13 @@ retry:
*/
ResetPerTupleExprContext(estate);
+ if (cstate->opts.reject_limit > 0 &&+ cstate->num_errors > cstate->opts.reject_limit)+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),+ errmsg("skipped more than REJECT_LIMIT (%lld) rows due to data type incompatibility",+ (long long) cstate->opts.reject_limit)));+
/* Repeat NextCopyFrom() until no soft error occurs */
goto retry;
}
diff --git a/contrib/file_fdw/sql/file_fdw.sql b/contrib/file_fdw/sql/file_fdw.sqlindex edd77c5cd2..4805ca8c04 100644--- a/contrib/file_fdw/sql/file_fdw.sql+++ b/contrib/file_fdw/sql/file_fdw.sql@@ -77,6 +77,7 @@ CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', delimiter
'); -- ERROR
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null '
'); -- ERROR
+CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (reject_limit '1'); -- ERROR
CREATE FOREIGN TABLE tbl () SERVER file_server; -- ERROR
\set filename :abs_srcdir '/data/agg.data'
@@ -150,11 +151,15 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
-- error context report tests
SELECT * FROM agg_bad; -- ERROR
--- on_error and log_verbosity tests+-- on_error, log_verbosity and reject_limit tests
ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
SELECT * FROM agg_bad;
ALTER FOREIGN TABLE agg_bad OPTIONS (ADD log_verbosity 'silent');
SELECT * FROM agg_bad;
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD reject_limit '1'); -- ERROR+SELECT * FROM agg_bad;+ALTER FOREIGN TABLE agg_bad OPTIONS (SET reject_limit '2');+SELECT * FROM agg_bad;
ANALYZE agg_bad;
-- misc query tests
diff --git a/doc/src/sgml/file-fdw.sgml b/doc/src/sgml/file-fdw.sgmlindex bb3579b077..882d9a76d2 100644--- a/doc/src/sgml/file-fdw.sgml+++ b/doc/src/sgml/file-fdw.sgml@@ -138,6 +138,18 @@
</listitem>
</varlistentry>
+ <varlistentry>+ <term><literal>reject_limit</literal></term>++ <listitem>+ <para>+ Specifies the maximum number of errors tolerated while converting a column's+ input value to its data type, the same as <command>COPY</command>'s+ <literal>REJECT_LIMIT</literal> option.+ </para>+ </listitem>+ </varlistentry>+
<varlistentry>
<term><literal>log_verbosity</literal></term>
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.cindex 3485ba8663..2d98ecf3f4 100644--- a/src/backend/commands/copy.c+++ b/src/backend/commands/copy.c@@ -420,11 +420,25 @@ defGetCopyOnErrorChoice(DefElem *def, ParseState *pstate, bool is_from)
/*
* Extract REJECT_LIMIT value from a DefElem.
+ *+ * REJECT_LIMIT can be specified in two ways: as an int64 for the COPY command+ * option or as a single-quoted string for the foreign table option using+ * file_fdw. Therefore this function needs to handle both formats.
*/
static int64
defGetCopyRejectLimitOption(DefElem *def)
{
- int64 reject_limit = defGetInt64(def);+ int64 reject_limit;++ if (def->arg == NULL)+ ereport(ERROR,+ (errcode(ERRCODE_SYNTAX_ERROR),+ errmsg("%s requires a numeric value",+ def->defname)));+ else if (nodeTag(def->arg) == T_String)+ reject_limit = pg_strtoint64(strVal(def->arg));+ else+ reject_limit = defGetInt64(def);
if (reject_limit <= 0)
ereport(ERROR,
--
2.39.2
v5-0002-Add-validator-tests-for-on_error-options.patch (text/x-diff, 2.1K) download
From ab3c38571761507a9e1b3073ffa9c2fdbc50c7de Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Tue, 19 Nov 2024 21:12:52 +0900
Subject: [PATCH v5 2/2] Add validator tests for on_error options
---
contrib/file_fdw/expected/file_fdw.out | 4 ++++
contrib/file_fdw/sql/file_fdw.sql | 2 ++
2 files changed, 6 insertions(+)
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.outindex 4f63c047ec..f383aec6d8 100644--- a/contrib/file_fdw/expected/file_fdw.out+++ b/contrib/file_fdw/expected/file_fdw.out@@ -90,6 +90,10 @@ ERROR: COPY delimiter cannot be newline or carriage return
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null '
'); -- ERROR
ERROR: COPY null representation cannot use newline or carriage return
+CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (on_error 'aaa'); -- ERROR+ERROR: COPY ON_ERROR "aaa" not recognized+CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', on_error 'ignore'); -- ERROR+ERROR: only ON_ERROR STOP is allowed in BINARY mode
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (reject_limit '1'); -- ERROR
ERROR: COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE
CREATE FOREIGN TABLE tbl () SERVER file_server; -- ERROR
diff --git a/contrib/file_fdw/sql/file_fdw.sql b/contrib/file_fdw/sql/file_fdw.sqlindex 4805ca8c04..1a76aebde6 100644--- a/contrib/file_fdw/sql/file_fdw.sql+++ b/contrib/file_fdw/sql/file_fdw.sql@@ -77,6 +77,8 @@ CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', delimiter
'); -- ERROR
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null '
'); -- ERROR
+CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (on_error 'aaa'); -- ERROR+CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', on_error 'ignore'); -- ERROR
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (reject_limit '1'); -- ERROR
CREATE FOREIGN TABLE tbl () SERVER file_server; -- ERROR
--
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, nagata@sraoss.co.jp, masao.fujii@oss.nttdata.com
Subject: Re: Add reject_limit option to file_fdw
In-Reply-To: <a7d70e35229e2075b371cd1e81222244@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