Re: Add reject_limit option to file_fdw

On Tue, 12 Nov 2024 10:16:50 +0900
torikoshia <torikoshia(at)oss(dot)nttdata(dot)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?

Regards,
Yugo Nagata

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>