From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: Pgsql Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add reject_limit option to file_fdw
Date: Fri, 8 Nov 2024 01:44:43 +0900
Message-ID: <2b3ad6d4-e05f-4188-8cfe-1fd9d949a9c9@oss.nttdata.com> (raw)
In-Reply-To: <54747389a562688eab7077b144daff6a@oss.nttdata.com>
References: <bab68a9fc502b12693f0755b6f35f327@oss.nttdata.com>
<fd14c3a0-a036-4bce-b5f8-4af83259eae1@oss.nttdata.com>
<54747389a562688eab7077b144daff6a@oss.nttdata.com>
On 2024/11/05 22:30, torikoshia wrote:
>> Thanks for the patch! Could you add it to the next CommitFest?
>
> Added an entry for this patch:
> https://commitfest.postgresql.org/50/5331/
Thanks!
>> +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.
As for the agg.bad2 file that your latest patch added:
Instead of adding a new bad input file, what do you think about simply appending
"1;@bbb@" to the existing agg.bad file and adjusting sql/file_fdw.sql as follows?
This approach might keep things simpler.
-------------------
-\set filename :abs_srcdir '/data/agg.bad2'
-ALTER FOREIGN TABLE agg_bad OPTIONS (SET filename :'filename', ADD reject_limit '1');
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD reject_limit '1');
SELECT * FROM agg_bad;
ALTER FOREIGN TABLE agg_bad OPTIONS (SET reject_limit '2');
-------------------
>> + <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.
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.
------------------------------------
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, torikoshia@oss.nttdata.com
Subject: Re: Add reject_limit option to file_fdw
In-Reply-To: <2b3ad6d4-e05f-4188-8cfe-1fd9d949a9c9@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox