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.