Re: Add reject_limit option to file_fdw

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.