Horton

Thread

Commits

  1. file_fdw: Add regression tests for ON_ERROR and other options.

  2. file_fdw: Add REJECT_LIMIT option to file_fdw.

  1. Add reject_limit option to file_fdw

    torikoshia <torikoshia@oss.nttdata.com> — 2024-10-17T13:45:46Z

    Hi,

4ac2a9bec introduced reject_limit option to the COPY command, and I was 
wondering if it might be beneficial to add the same option to file_fdw.

Although there may be fewer practical use cases compared to COPY, it 
could still be useful in situations where the file being read via 
file_fdw is subject to modifications and there is a need to tolerate a 
limited number of errors.

What do you think?

I've attached a patch.
Based on the synopsis of the CREATE/ALTER FOREIGN TABLE commands, the 
value for the foreign table's option must be single-quoted. I’m not 
entirely sure if this is the correct approach, but in order to 
accommodate this, the patch modifies the value of reject_limit option to 
accept not only numeric values but also strings.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  2. Re: Add reject_limit option to file_fdw

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-10-17T16:51:59Z

    
On 2024/10/17 22:45, torikoshia wrote:
> Hi,
> 
> 4ac2a9bec introduced reject_limit option to the COPY command, and I was wondering if it might be beneficial to add the same option to file_fdw.
> 
> Although there may be fewer practical use cases compared to COPY, it could still be useful in situations where the file being read via file_fdw is subject to modifications and there is a need to tolerate a limited number of errors.

Agreed.


> 
> What do you think?
> 
> I've attached a patch.

Thanks for the patch! Could you add it to the next CommitFest?


+ALTER FOREIGN TABLE agg_bad OPTIONS (reject_limit '1');
+SELECT * FROM agg_bad;
+  a  |   b
+-----+--------
+ 100 | 99.097
+  42 | 324.78
+(2 rows)

Wouldn't it be better to include a test where a SELECT query fails, even with
on_error set to "ignore," because the number of errors exceeds reject_limit?


+			if (cstate->opts.reject_limit > 0 && \

The trailing backslash isn't needed here.


+  <varlistentry>
+   <term><literal>reject_limit</literal></term>

This entry should be placed right after the on_error option,
following the same order as in the COPY command documentation.


> Based on the synopsis of the CREATE/ALTER FOREIGN TABLE commands, the value for the foreign table's option must be single-quoted. I’m not entirely sure if this is the correct approach, but in order to accommodate this, the patch modifies the value of reject_limit option to accept not only numeric values but also strings.
> 

I don't have a better approach for this, so I'm okay with your solution.
Just one note: it would be helpful to explain and comment
why defGetCopyRejectLimitOption() accepts and parses both int64 and
string values.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  3. Re: Add reject_limit option to file_fdw

    torikoshia <torikoshia@oss.nttdata.com> — 2024-11-05T13:30:28Z

    On 2024-10-18 01:51, Fujii Masao wrote:
Thanks for your review and sorry for my late reply.

> On 2024/10/17 22:45, torikoshia wrote:
>> Hi,
>> 
>> 4ac2a9bec introduced reject_limit option to the COPY command, and I 
>> was wondering if it might be beneficial to add the same option to 
>> file_fdw.
>> 
>> Although there may be fewer practical use cases compared to COPY, it 
>> could still be useful in situations where the file being read via 
>> file_fdw is subject to modifications and there is a need to tolerate a 
>> limited number of errors.
> 
> Agreed.
> 
> 
>> 
>> What do you think?
>> 
>> I've attached a patch.
> 
> Thanks for the patch! Could you add it to the next CommitFest?

Added an entry for this patch:
https://commitfest.postgresql.org/50/5331/

> +ALTER FOREIGN TABLE agg_bad OPTIONS (reject_limit '1');
> +SELECT * FROM agg_bad;
> +  a  |   b
> +-----+--------
> + 100 | 99.097
> +  42 | 324.78
> +(2 rows)
> 
> Wouldn't it be better to include a test where a SELECT query fails, 
> even with
> on_error set to "ignore," because the number of errors exceeds 
> reject_limit?

Agreed.

> 
> +			if (cstate->opts.reject_limit > 0 && \
> 
> The trailing backslash isn't needed here.
Agreed.

> 
> 
> +  <varlistentry>
> +   <term><literal>reject_limit</literal></term>
> 
> This entry should be placed right after the on_error option,
> following the same order as in the COPY command documentation.
> 
> 
>> Based on the synopsis of the CREATE/ALTER FOREIGN TABLE commands, the 
>> value for the foreign table's option must be single-quoted. I’m not 
>> entirely sure if this is the correct approach, but in order to 
>> accommodate this, the patch modifies the value of reject_limit option 
>> to accept not only numeric values but also strings.
>> 
> 
> I don't have a better approach for this, so I'm okay with your 
> solution.
> Just one note: it would be helpful to explain and comment
> why defGetCopyRejectLimitOption() accepts and parses both int64 and
> string values.

Added a comment for this.


-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

  4. Re: Add reject_limit option to file_fdw

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-11-07T16:44:43Z

    
On 2024/11/05 22:30, torikoshia wrote:
>> Thanks for the patch! Could you add it to the next CommitFest?
> 
> Added an entry for this patch:
> https://commitfest.postgresql.org/50/5331/

Thanks!


>> +ALTER FOREIGN TABLE agg_bad OPTIONS (reject_limit '1');
>> +SELECT * FROM agg_bad;
>> +  a  |   b
>> +-----+--------
>> + 100 | 99.097
>> +  42 | 324.78
>> +(2 rows)
>>
>> Wouldn't it be better to include a test where a SELECT query fails, even with
>> on_error set to "ignore," because the number of errors exceeds reject_limit?
> 
> Agreed.

As for the agg.bad2 file that your latest patch added:

Instead of adding a new bad input file, what do you think about simply appending
"1;@bbb@" to the existing agg.bad file and adjusting sql/file_fdw.sql as follows?
This approach might keep things simpler.

-------------------
-\set filename :abs_srcdir '/data/agg.bad2'
-ALTER FOREIGN TABLE agg_bad OPTIONS (SET filename :'filename', ADD reject_limit '1');
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD reject_limit '1');
  SELECT * FROM agg_bad;
  ALTER FOREIGN TABLE agg_bad OPTIONS (SET reject_limit '2');
-------------------

>> +  <varlistentry>
>> +   <term><literal>reject_limit</literal></term>
>>
>> This entry should be placed right after the on_error option,
>> following the same order as in the COPY command documentation.
>>
>>
>>> Based on the synopsis of the CREATE/ALTER FOREIGN TABLE commands, the value for the foreign table's option must be single-quoted. I’m not entirely sure if this is the correct approach, but in order to accommodate this, the patch modifies the value of reject_limit option to accept not only numeric values but also strings.
>>>
>>
>> I don't have a better approach for this, so I'm okay with your solution.
>> Just one note: it would be helpful to explain and comment
>> why defGetCopyRejectLimitOption() accepts and parses both int64 and
>> string values.
> 
> Added a comment for this.

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

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  5. Re: Add reject_limit option to file_fdw

    torikoshia <torikoshia@oss.nttdata.com> — 2024-11-11T12:45:39Z

    On 2024-11-08 01:44, Fujii Masao wrote:
Thanks for your review!

> On 2024/11/05 22:30, torikoshia wrote:
>>> Thanks for the patch! Could you add it to the next CommitFest?
>> 
>> Added an entry for this patch:
>> https://commitfest.postgresql.org/50/5331/
> 
> Thanks!
> 
> 
>>> +ALTER FOREIGN TABLE agg_bad OPTIONS (reject_limit '1');
>>> +SELECT * FROM agg_bad;
>>> +  a  |   b
>>> +-----+--------
>>> + 100 | 99.097
>>> +  42 | 324.78
>>> +(2 rows)
>>> 
>>> Wouldn't it be better to include a test where a SELECT query fails, 
>>> even with
>>> on_error set to "ignore," because the number of errors exceeds 
>>> reject_limit?
>> 
>> Agreed.
> 
> As for the agg.bad2 file that your latest patch added:
> 
> Instead of adding a new bad input file, what do you think about simply 
> appending
> "1;@bbb@" to the existing agg.bad file and adjusting sql/file_fdw.sql
> as follows?
> This approach might keep things simpler.

That seems better. Agreed.

> -------------------
> -\set filename :abs_srcdir '/data/agg.bad2'
> -ALTER FOREIGN TABLE agg_bad OPTIONS (SET filename :'filename', ADD
> reject_limit '1');
> +ALTER FOREIGN TABLE agg_bad OPTIONS (ADD reject_limit '1');
>  SELECT * FROM agg_bad;
>  ALTER FOREIGN TABLE agg_bad OPTIONS (SET reject_limit '2');
> -------------------
> 
>>> +  <varlistentry>
>>> +   <term><literal>reject_limit</literal></term>
>>> 
>>> This entry should be placed right after the on_error option,
>>> following the same order as in the COPY command documentation.
>>> 
>>> 
>>>> Based on the synopsis of the CREATE/ALTER FOREIGN TABLE commands, 
>>>> the value for the foreign table's option must be single-quoted. I’m 
>>>> not entirely sure if this is the correct approach, but in order to 
>>>> accommodate this, the patch modifies the value of reject_limit 
>>>> option to accept not only numeric values but also strings.
>>>> 
>>> 
>>> I don't have a better approach for this, so I'm okay with your 
>>> solution.
>>> Just one note: it would be helpful to explain and comment
>>> why defGetCopyRejectLimitOption() accepts and parses both int64 and
>>> string values.
>> 
>> Added a comment for this.
> 
> 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.

-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

  6. Re: Add reject_limit option to file_fdw

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-11-11T16:49:34Z

    
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.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  7. Re: Add reject_limit option to file_fdw

    torikoshia <torikoshia@oss.nttdata.com> — 2024-11-12T01:16:50Z

    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.


-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

  8. Re: Add reject_limit option to file_fdw

    Yugo Nagata <nagata@sraoss.co.jp> — 2024-11-12T05:51:14Z

    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?

Regards,
Yugo Nagata

-- 
Yugo Nagata <nagata@sraoss.co.jp>

  9. Re: Add reject_limit option to file_fdw

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-12T06:23:03Z

    On Tue, 12 Nov 2024 at 06:17, 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.
>
>
> --
> Regards,
>
> --
> Atsushi Torikoshi
> Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

Hi!

A little question from me.

This is your doc for reject_limit:

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

This is how it looks on the current HEAD for copy.

<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, when <literal>ON_ERROR</literal> is
      set to <literal>ignore</literal>.
      If the input causes more errors than the specified value, the
<command>COPY</command>
      command fails, even with <literal>ON_ERROR</literal> set to
<literal>ignore</literal>.
      This clause must be used with
<literal>ON_ERROR</literal>=<literal>ignore</literal>
      and <replaceable class="parameter">maxerror</replaceable> must
be positive <type>bigint</type>.
      If not specified, <literal>ON_ERROR</literal>=<literal>ignore</literal>
      allows an unlimited number of errors, meaning <command>COPY</command> will
      skip all erroneous data.
     </para>
    </listitem>
   </varlistentry>

There is a difference. Should we add REJECT_LIMIT vs ON_ERROR
clarification for file_fdw too? or maybe we put a reference for COPY
doc.

-- 
Best regards,
Kirill Reshke

  10. Re: Add reject_limit option to file_fdw

    torikoshia <torikoshia@oss.nttdata.com> — 2024-11-13T12:47:21Z

    On 2024-11-12 15:23, Kirill Reshke wrote:

Thanks for your review!

> On Tue, 12 Nov 2024 at 06:17, 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.
>> 
>> 
>> --
>> Regards,
>> 
>> --
>> Atsushi Torikoshi
>> Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.
> 
> Hi!
> 
> A little question from me.
> 
> This is your doc for reject_limit:
> 
> +  <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>
> +
> 
> This is how it looks on the current HEAD for copy.
> 
> <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, when 
> <literal>ON_ERROR</literal> is
>       set to <literal>ignore</literal>.
>       If the input causes more errors than the specified value, the
> <command>COPY</command>
>       command fails, even with <literal>ON_ERROR</literal> set to
> <literal>ignore</literal>.
>       This clause must be used with
> <literal>ON_ERROR</literal>=<literal>ignore</literal>
>       and <replaceable class="parameter">maxerror</replaceable> must
> be positive <type>bigint</type>.
>       If not specified, 
> <literal>ON_ERROR</literal>=<literal>ignore</literal>
>       allows an unlimited number of errors, meaning 
> <command>COPY</command> will
>       skip all erroneous data.
>      </para>
>     </listitem>
>    </varlistentry>
> 
> There is a difference. Should we add REJECT_LIMIT vs ON_ERROR
> clarification for file_fdw too? or maybe we put a reference for COPY
> doc.

As you may know, some options for file_fdw are the same as those for 
COPY. While the manual provides detailed descriptions of these options 
in the COPY section, the explanations in file_fdw are shorter and less 
detailed.

I intended to follow this approach, but do you think it should be 
changed?

-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

  11. Re: Add reject_limit option to file_fdw

    torikoshia <torikoshia@oss.nttdata.com> — 2024-11-13T12:48:10Z

    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.

-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

  12. Re: Add reject_limit option to file_fdw

    Yugo Nagata <nagata@sraoss.co.jp> — 2024-11-13T14:17:06Z

    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.


As another comment, do we need to add validator test for on_error and
reject_limit as similar to other options?


Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

  13. Re: Add reject_limit option to file_fdw

    torikoshia <torikoshia@oss.nttdata.com> — 2024-11-19T12:40:30Z

    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.

  14. Re: Add reject_limit option to file_fdw

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-11-20T15:43:46Z

    
On 2024/11/19 21:40, torikoshia wrote:
>> 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.

I've pushed the 0001 patch. Thanks!


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

How about adding validator tests for log_verbosity and reject_limit=0,
similar to the tests in the copy2.sql regression test? I've added those
two tests, and the latest version of the patch is attached.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  15. Re: Add reject_limit option to file_fdw

    torikoshia <torikoshia@oss.nttdata.com> — 2024-11-21T06:43:22Z

    On 2024-11-21 00:43, Fujii Masao wrote:
> On 2024/11/19 21:40, torikoshia wrote:
>>> 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.
> 
> I've pushed the 0001 patch. Thanks!

Thanks a lot!

>>> 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.
> 
> How about adding validator tests for log_verbosity and reject_limit=0,
> similar to the tests in the copy2.sql regression test? I've added those
> two tests, and the latest version of the patch is attached.

Thanks for the update! Agreed.
I think the validator tests related to the on_error option you added are 
sufficient.

As for the other file_fdw options, I'm a bit concerned whether it would 
be better to add validator tests for cases like when 'encoding' is 
'unsupported', but that should be discussed in another thread.

> Regards,

-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

  16. Re: Add reject_limit option to file_fdw

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-11-27T14:45:23Z

    
On 2024/11/21 15:43, torikoshia wrote:
> Thanks for the update! Agreed.
> I think the validator tests related to the on_error option you added are sufficient.

I've pushed the patch. Thanks!

> As for the other file_fdw options, I'm a bit concerned whether it would be better to add validator tests for cases like when 'encoding' is 'unsupported', but that should be discussed in another thread.

I'm not sure how valuable it is to add that test, but yes,
it would be better to start a new thread for discussion if needed.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION