Re: Add reject_limit option to file_fdw

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