Re: Add reject_limit option to file_fdw

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.