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 ) id 1t95cr-007i4K-SI for pgsql-hackers@arkaria.postgresql.org; Thu, 07 Nov 2024 16:44:53 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1t95cp-00HRQC-6d for pgsql-hackers@arkaria.postgresql.org; Thu, 07 Nov 2024 16:44:51 +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 ) id 1t95co-00HRQ2-TI for pgsql-hackers@lists.postgresql.org; Thu, 07 Nov 2024 16:44:51 +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 ) id 1t95cm-000f6S-Cj for pgsql-hackers@postgresql.org; Thu, 07 Nov 2024 16:44:50 +0000 Received: from [192.168.11.3] (p4012170-ipxg13601funabasi.chiba.ocn.ne.jp [153.165.137.170]) by oss.nttdata.com (Postfix) with ESMTPSA id 5555A61901; Fri, 8 Nov 2024 01:44:44 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at oss.nttdata.com Message-ID: <2b3ad6d4-e05f-4188-8cfe-1fd9d949a9c9@oss.nttdata.com> Date: Fri, 8 Nov 2024 01:44:43 +0900 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: Add reject_limit option to file_fdw To: torikoshia Cc: Pgsql Hackers References:

<54747389a562688eab7077b144daff6a@oss.nttdata.com> Content-Language: en-US From: Fujii Masao In-Reply-To: <54747389a562688eab7077b144daff6a@oss.nttdata.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk 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'); ------------------- >> + >> + reject_limit >> >> 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