Re: Add reject_limit option to file_fdw

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.