Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps  (TLS1.3) tls
 TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1tATnh-00FyDd-Kd
	for pgsql-hackers@arkaria.postgresql.org; Mon, 11 Nov 2024 12:45:48 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.94.2)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1tATnf-00E0S8-3L
	for pgsql-hackers@arkaria.postgresql.org; Mon, 11 Nov 2024 12:45:47 +0000
Received: from makus.postgresql.org ([2001:4800:3e1:1::229])
	by malur.postgresql.org with esmtps  (TLS1.3) tls
 TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <torikoshia@oss.nttdata.com>)
	id 1tATne-00E0Rz-Ox
	for pgsql-hackers@lists.postgresql.org; Mon, 11 Nov 2024 12:45:47 +0000
Received: from oss.nttdata.com ([49.212.34.109])
	by makus.postgresql.org with esmtps  (TLS1.2) tls
 TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <torikoshia@oss.nttdata.com>)
	id 1tATnc-001JCL-1j
	for pgsql-hackers@postgresql.org; Mon, 11 Nov 2024 12:45:46 +0000
Received: from oss.nttdata.com (localhost [127.0.0.1])
	by oss.nttdata.com (Postfix) with ESMTPA id BB7A9618D5;
	Mon, 11 Nov 2024 21:45:39 +0900 (JST)
X-Virus-Status: Clean
X-Virus-Scanned: clamav-milter 0.103.11 at oss.nttdata.com
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Mon, 11 Nov 2024 21:45:39 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Pgsql Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add reject_limit option to file_fdw
In-Reply-To: <2b3ad6d4-e05f-4188-8cfe-1fd9d949a9c9@oss.nttdata.com>
References: <bab68a9fc502b12693f0755b6f35f327@oss.nttdata.com>
 <fd14c3a0-a036-4bce-b5f8-4af83259eae1@oss.nttdata.com>
 <54747389a562688eab7077b144daff6a@oss.nttdata.com>
 <2b3ad6d4-e05f-4188-8cfe-1fd9d949a9c9@oss.nttdata.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <192ede1e4c2b34b853e5ac46f8dee50b@oss.nttdata.com>
X-Sender: torikoshia@oss.nttdata.com
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: 
 <https://www.postgresql.org/message-id/192ede1e4c2b34b853e5ac46f8dee50b%40oss.nttdata.com>
Precedence: bulk

On 2024-11-08 01:44, Fujii Masao wrote:
Thanks for your review!

> 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.

That seems better. Agreed.

> -------------------
> -\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.
> ------------------------------------

Thanks! it seems better.


Attached v3 patch.

-- 
Regards,

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