Re: Add reject_limit option to file_fdw

On 2024-11-13 23:17, Yugo NAGATA wrote:
> On Wed, 13 Nov 2024 21:48:10 +0900
> torikoshia <torikoshia@oss.nttdata.com> wrote:
> 
>> 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.
> 
> 
> Thank you for updating the patch.
> 
> However, I noticed that file_fdw could output the following error
> using uppercase
> when an invalid value is set to the reject_limit option,
> 
>  ALTER FOREIGN TABLE agg_bad OPTIONS (ADD reject_limit '-1');
>  ERROR:  REJECT_LIMIT (-1) must be greater than zero
> 
> as well as if reject_limit is set when on_error != 'ignore'.
> 
>  ALTER FOREIGN TABLE agg_bad OPTIONS (ADD reject_limit '1'); -- ERROR
>  ERROR:  COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE
> 
> Also, in the regression tests, I found similar behaviours on existing 
> options,
> for example;
> 
>  CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format
> 'xml');  -- ERROR
>  ERROR:  COPY format "xml" not recognized
>  CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format
> 'text', quote ':');          -- ERROR
>  ERROR:  COPY QUOTE requires CSV mode
>  CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format
> 'text', escape ':');         -- ERROR
>  ...
> 
> These messages may be unexpected for some users because the 
> documentation of
> fild_fdw does not explicitly describe that file_fdw uses COPY 
> internally.
> (I can find several wordings like "as COPY", though.)
> However, since the current file_fdw already has such messages, I came 
> to think
> making the messages on "reject_limit" consistent with COPY is 
> reasonable.
> I mean, the previous version of the message seems fine.

Agreed. Thanks for your investigation.

> As another comment, do we need to add validator test for on_error and
> reject_limit as similar to other options?

That might be better.
Added some queries which had wrong options to cause errors.

I was unsure whether to add it to "validator test" section or "on_error, 
log_verbosity and reject_limit tests" section, but I chose "validator 
test" as I believe it makes things clearer.

Added 0002 patch since some of the tests are not related to reject_limit 
but just on_error.


-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.