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 1t1TjP-00GVAd-Qa
	for pgsql-hackers@arkaria.postgresql.org; Thu, 17 Oct 2024 16:52:11 +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 1t1TjN-007H3D-LU
	for pgsql-hackers@arkaria.postgresql.org; Thu, 17 Oct 2024 16:52:10 +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 <masao.fujii@oss.nttdata.com>)
	id 1t1TjN-007H35-6a
	for pgsql-hackers@lists.postgresql.org; Thu, 17 Oct 2024 16:52:09 +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 <masao.fujii@oss.nttdata.com>)
	id 1t1TjJ-001Oqk-2t
	for pgsql-hackers@postgresql.org; Thu, 17 Oct 2024 16:52:07 +0000
Received: from [192.168.11.3] (p4010149-ipxg13601funabasi.chiba.ocn.ne.jp [153.165.135.149])
	by oss.nttdata.com (Postfix) with ESMTPSA id 1BF276064A;
	Fri, 18 Oct 2024 01:52:00 +0900 (JST)
X-Virus-Status: Clean
X-Virus-Scanned: clamav-milter 0.103.11 at oss.nttdata.com
Message-ID: <fd14c3a0-a036-4bce-b5f8-4af83259eae1@oss.nttdata.com>
Date: Fri, 18 Oct 2024 01:51:59 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add reject_limit option to file_fdw
To: torikoshia <torikoshia@oss.nttdata.com>,
 Pgsql Hackers <pgsql-hackers@postgresql.org>
References: <bab68a9fc502b12693f0755b6f35f327@oss.nttdata.com>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <bab68a9fc502b12693f0755b6f35f327@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
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/fd14c3a0-a036-4bce-b5f8-4af83259eae1%40oss.nttdata.com>
Precedence: bulk



On 2024/10/17 22:45, torikoshia wrote:
> Hi,
> 
> 4ac2a9bec introduced reject_limit option to the COPY command, and I was wondering if it might be beneficial to add the same option to file_fdw.
> 
> Although there may be fewer practical use cases compared to COPY, it could still be useful in situations where the file being read via file_fdw is subject to modifications and there is a need to tolerate a limited number of errors.

Agreed.


> 
> What do you think?
> 
> I've attached a patch.

Thanks for the patch! Could you add it to the next CommitFest?


+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?


+			if (cstate->opts.reject_limit > 0 && \

The trailing backslash isn't needed here.


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

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION