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 1t8JeE-003PG5-Dv for pgsql-hackers@arkaria.postgresql.org; Tue, 05 Nov 2024 13:31:05 +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 1t8JeB-00DW5O-Q8 for pgsql-hackers@arkaria.postgresql.org; Tue, 05 Nov 2024 13:31:04 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1t8Jdm-00DTOk-Cu for pgsql-hackers@lists.postgresql.org; Tue, 05 Nov 2024 13:30:39 +0000 Received: from oss.nttdata.com ([49.212.34.109]) by magus.postgresql.org with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1t8Jdh-000KFZ-AT for pgsql-hackers@postgresql.org; Tue, 05 Nov 2024 13:30:38 +0000 Received: from oss.nttdata.com (localhost [127.0.0.1]) by oss.nttdata.com (Postfix) with ESMTPA id 3C794619B7; Tue, 5 Nov 2024 22:30:28 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at oss.nttdata.com MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=_0d6ec320693688e4b4d8b579d45aa934" Content-Transfer-Encoding: 8bit Date: Tue, 05 Nov 2024 22:30:28 +0900 From: torikoshia To: Fujii Masao Cc: Pgsql Hackers Subject: Re: Add reject_limit option to file_fdw In-Reply-To: References: User-Agent: Roundcube Webmail/1.4.11 Message-ID: <54747389a562688eab7077b144daff6a@oss.nttdata.com> X-Sender: torikoshia@oss.nttdata.com List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --=_0d6ec320693688e4b4d8b579d45aa934 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=UTF-8; format=flowed On 2024-10-18 01:51, Fujii Masao wrote: Thanks for your review and sorry for my late reply. > 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? Added an entry for this patch: https://commitfest.postgresql.org/50/5331/ > +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. > > + if (cstate->opts.reject_limit > 0 && \ > > The trailing backslash isn't needed here. Agreed. > > > + > + 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. -- Regards, -- Atsushi Torikoshi Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K. --=_0d6ec320693688e4b4d8b579d45aa934 Content-Transfer-Encoding: base64 Content-Type: text/x-diff; name=v2-0001-file_fdw-Add-reject_limit-option-to-file_fdw.patch Content-Disposition: attachment; filename=v2-0001-file_fdw-Add-reject_limit-option-to-file_fdw.patch; size=6278 RnJvbSAwMmI4MWYzNzZkOWIzMTZhMDZkNjhjN2JmNzE0YzY3MjkxMDAxNjJjIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBdHN1c2hpIFRvcmlrb3NoaSA8dG9yaWtvc2hpYUBvc3MubnR0 ZGF0YS5jb20+CkRhdGU6IFR1ZSwgNSBOb3YgMjAyNCAyMTo1NzoxNiArMDkwMApTdWJqZWN0OiBb UEFUQ0ggdjJdIGZpbGVfZmR3OiBBZGQgcmVqZWN0X2xpbWl0IG9wdGlvbiB0byBmaWxlX2ZkdwoK Q29tbWl0IDRhYzJhOWJlY2UgaW50cm9kdWNlZCBSRUpFQ1RfTElNSVQgb3B0aW9uIGZvciBDT1BZ LiBUaGlzIHBhdGNoCmV4dGVuZHMgc3VwcG9ydCBmb3IgdGhpcyBvcHRpb24gdG8gZmlsZV9mZHcu CgpBcyB3ZWxsIGFzIFJFSkVDVF9MSU1JVCBvcHRpb24gZm9yIENPUFksIHRoaXMgb3B0aW9uIGxp bWl0cyB0aGUKbWF4aW11bSBudW1iZXIgb2YgZXJyb25lb3VzIHJvd3MgdGhhdCBjYW4gYmUgc2tp cHBlZC4KSWYgbW9yZSByb3dzIGVuY291bnRlciBkYXRhIHR5cGUgY29udmVyc2lvbiBlcnJvcnMg dGhhbiBhbGxvd2VkIGJ5CnJlamVjdF9saW1pdCwgdGhlIGFjY2VzcyB0byB0aGUgZmlsZV9mZHcg Zm9yZWlnbiB0YWJsZSB3aWxsIGZhaWwKd2l0aCBhbiBlcnJvciwgZXZlbiB3aGVuIG9uX2Vycm9y ID0gJ2lnbm9yZScuCgpCYXNlZCBvbiB0aGUgc3lub3BzaXMgb2YgdGhlIENSRUFURS9BTFRFUiBG T1JFSUdOIFRBQkxFIGNvbW1hbmRzLCB0aGUKdmFsdWUgZm9yIHRoZSBmb3JlaWduIHRhYmxlJ3Mg b3B0aW9uIG11c3QgYmUgc2luZ2xlLXF1b3RlZC4KVG8gYWNjb21tb2RhdGUgdGhpcywgdGhlIHBh dGNoIG1vZGlmaWVzIGRlZkdldENvcHlSZWplY3RMaW1pdE9wdGlvbgp0byBhY2NlcHQgbm90IG9u bHkgaW50NjQgYnV0IGFsc28gc3RyaW5ncy4KCi0tLQogY29udHJpYi9maWxlX2Zkdy9kYXRhL2Fn Zy5iYWQyICAgICAgICAgfCAgNSArKysrKwogY29udHJpYi9maWxlX2Zkdy9leHBlY3RlZC9maWxl X2Zkdy5vdXQgfCAxNSArKysrKysrKysrKysrKy0KIGNvbnRyaWIvZmlsZV9mZHcvZmlsZV9mZHcu YyAgICAgICAgICAgIHwgIDggKysrKysrKysKIGNvbnRyaWIvZmlsZV9mZHcvc3FsL2ZpbGVfZmR3 LnNxbCAgICAgIHwgIDcgKysrKysrLQogZG9jL3NyYy9zZ21sL2ZpbGUtZmR3LnNnbWwgICAgICAg ICAgICAgfCAxMiArKysrKysrKysrKysKIHNyYy9iYWNrZW5kL2NvbW1hbmRzL2NvcHkuYyAgICAg ICAgICAgIHwgMTUgKysrKysrKysrKysrKystCiA2IGZpbGVzIGNoYW5nZWQsIDU5IGluc2VydGlv bnMoKyksIDMgZGVsZXRpb25zKC0pCiBjcmVhdGUgbW9kZSAxMDA2NDQgY29udHJpYi9maWxlX2Zk dy9kYXRhL2FnZy5iYWQyCgpkaWZmIC0tZ2l0IGEvY29udHJpYi9maWxlX2Zkdy9kYXRhL2FnZy5i YWQyIGIvY29udHJpYi9maWxlX2Zkdy9kYXRhL2FnZy5iYWQyCm5ldyBmaWxlIG1vZGUgMTAwNjQ0 CmluZGV4IDAwMDAwMDAwMDAuLjA0Mjc5Y2U1NWIKLS0tIC9kZXYvbnVsbAorKysgYi9jb250cmli L2ZpbGVfZmR3L2RhdGEvYWdnLmJhZDIKQEAgLTAsMCArMSw1IEBACis1NjtANy44QAorMTAwO0A5 OS4wOTdACiswO0BhYWFACis0MjtAMzI0Ljc4QAorMTtAYmJiQApkaWZmIC0tZ2l0IGEvY29udHJp Yi9maWxlX2Zkdy9leHBlY3RlZC9maWxlX2Zkdy5vdXQgYi9jb250cmliL2ZpbGVfZmR3L2V4cGVj dGVkL2ZpbGVfZmR3Lm91dAppbmRleCA1OTNmZGM3ODJlLi5kYmZhZTUyYmFmIDEwMDY0NAotLS0g YS9jb250cmliL2ZpbGVfZmR3L2V4cGVjdGVkL2ZpbGVfZmR3Lm91dAorKysgYi9jb250cmliL2Zp bGVfZmR3L2V4cGVjdGVkL2ZpbGVfZmR3Lm91dApAQCAtMjA2LDcgKzIwNiw3IEBAIFNFTEVDVCAq IEZST00gYWdnX2NzdiBjIEpPSU4gYWdnX3RleHQgdCBPTiAodC5hID0gYy5hKSBPUkRFUiBCWSBj LmE7CiBTRUxFQ1QgKiBGUk9NIGFnZ19iYWQ7ICAgICAgICAgICAgICAgLS0gRVJST1IKIEVSUk9S OiAgaW52YWxpZCBpbnB1dCBzeW50YXggZm9yIHR5cGUgcmVhbDogImFhYSIKIENPTlRFWFQ6ICBD T1BZIGFnZ19iYWQsIGxpbmUgMywgY29sdW1uIGI6ICJhYWEiCi0tLSBvbl9lcnJvciBhbmQgbG9n X3ZlcmJvc2l0eSB0ZXN0cworLS0gb25fZXJyb3IsIGxvZ192ZXJib3NpdHkgYW5kIHJlamVjdF9s aW1pdCB0ZXN0cwogQUxURVIgRk9SRUlHTiBUQUJMRSBhZ2dfYmFkIE9QVElPTlMgKEFERCBvbl9l cnJvciAnaWdub3JlJyk7CiBTRUxFQ1QgKiBGUk9NIGFnZ19iYWQ7CiBOT1RJQ0U6ICAxIHJvdyB3 YXMgc2tpcHBlZCBkdWUgdG8gZGF0YSB0eXBlIGluY29tcGF0aWJpbGl0eQpAQCAtMjI0LDYgKzIy NCwxOSBAQCBTRUxFQ1QgKiBGUk9NIGFnZ19iYWQ7CiAgIDQyIHwgMzI0Ljc4CiAoMiByb3dzKQog Citcc2V0IGZpbGVuYW1lIDphYnNfc3JjZGlyICcvZGF0YS9hZ2cuYmFkMicKK0FMVEVSIEZPUkVJ R04gVEFCTEUgYWdnX2JhZCBPUFRJT05TIChTRVQgZmlsZW5hbWUgOidmaWxlbmFtZScsIEFERCBy ZWplY3RfbGltaXQgJzEnKTsKK1NFTEVDVCAqIEZST00gYWdnX2JhZDsKK0VSUk9SOiAgc2tpcHBl ZCBtb3JlIHRoYW4gUkVKRUNUX0xJTUlUICgxKSByb3dzIGR1ZSB0byBkYXRhIHR5cGUgaW5jb21w YXRpYmlsaXR5CitDT05URVhUOiAgQ09QWSBhZ2dfYmFkLCBsaW5lIDUsIGNvbHVtbiBiOiAiYmJi IgorQUxURVIgRk9SRUlHTiBUQUJMRSBhZ2dfYmFkIE9QVElPTlMgKFNFVCByZWplY3RfbGltaXQg JzInKTsKK1NFTEVDVCAqIEZST00gYWdnX2JhZDsKKyAgYSAgfCAgIGIgICAgCistLS0tLSstLS0t LS0tLQorIDEwMCB8IDk5LjA5NworICA0MiB8IDMyNC43OAorKDIgcm93cykKKwogQU5BTFlaRSBh Z2dfYmFkOwogLS0gbWlzYyBxdWVyeSB0ZXN0cwogXHQgb24KZGlmZiAtLWdpdCBhL2NvbnRyaWIv ZmlsZV9mZHcvZmlsZV9mZHcuYyBiL2NvbnRyaWIvZmlsZV9mZHcvZmlsZV9mZHcuYwppbmRleCAw NDMyMDRjM2U3Li45ZTI4OTZmMzJhIDEwMDY0NAotLS0gYS9jb250cmliL2ZpbGVfZmR3L2ZpbGVf ZmR3LmMKKysrIGIvY29udHJpYi9maWxlX2Zkdy9maWxlX2Zkdy5jCkBAIC03Nyw2ICs3Nyw3IEBA IHN0YXRpYyBjb25zdCBzdHJ1Y3QgRmlsZUZkd09wdGlvbiB2YWxpZF9vcHRpb25zW10gPSB7CiAJ eyJlbmNvZGluZyIsIEZvcmVpZ25UYWJsZVJlbGF0aW9uSWR9LAogCXsib25fZXJyb3IiLCBGb3Jl aWduVGFibGVSZWxhdGlvbklkfSwKIAl7ImxvZ192ZXJib3NpdHkiLCBGb3JlaWduVGFibGVSZWxh dGlvbklkfSwKKwl7InJlamVjdF9saW1pdCIsIEZvcmVpZ25UYWJsZVJlbGF0aW9uSWR9LAogCXsi Zm9yY2Vfbm90X251bGwiLCBBdHRyaWJ1dGVSZWxhdGlvbklkfSwKIAl7ImZvcmNlX251bGwiLCBB dHRyaWJ1dGVSZWxhdGlvbklkfSwKIApAQCAtNzg4LDYgKzc4OSwxMyBAQCByZXRyeToKIAkJCSAq LwogCQkJUmVzZXRQZXJUdXBsZUV4cHJDb250ZXh0KGVzdGF0ZSk7CiAKKwkJCWlmIChjc3RhdGUt Pm9wdHMucmVqZWN0X2xpbWl0ID4gMCAmJgorCQkJCWNzdGF0ZS0+bnVtX2Vycm9ycyA+IGNzdGF0 ZS0+b3B0cy5yZWplY3RfbGltaXQpCisJCQkJZXJlcG9ydChFUlJPUiwKKwkJCQkJCShlcnJjb2Rl KEVSUkNPREVfSU5WQUxJRF9URVhUX1JFUFJFU0VOVEFUSU9OKSwKKwkJCQkJCSBlcnJtc2coInNr aXBwZWQgbW9yZSB0aGFuIFJFSkVDVF9MSU1JVCAoJWxsZCkgcm93cyBkdWUgdG8gZGF0YSB0eXBl IGluY29tcGF0aWJpbGl0eSIsCisJCQkJCQkJCShsb25nIGxvbmcpIGNzdGF0ZS0+b3B0cy5yZWpl Y3RfbGltaXQpKSk7CisKIAkJCS8qIFJlcGVhdCBOZXh0Q29weUZyb20oKSB1bnRpbCBubyBzb2Z0 IGVycm9yIG9jY3VycyAqLwogCQkJZ290byByZXRyeTsKIAkJfQpkaWZmIC0tZ2l0IGEvY29udHJp Yi9maWxlX2Zkdy9zcWwvZmlsZV9mZHcuc3FsIGIvY29udHJpYi9maWxlX2Zkdy9zcWwvZmlsZV9m ZHcuc3FsCmluZGV4IGVkZDc3YzVjZDIuLjAyMDhlMDk1YWUgMTAwNjQ0Ci0tLSBhL2NvbnRyaWIv ZmlsZV9mZHcvc3FsL2ZpbGVfZmR3LnNxbAorKysgYi9jb250cmliL2ZpbGVfZmR3L3NxbC9maWxl X2Zkdy5zcWwKQEAgLTE1MCwxMSArMTUwLDE2IEBAIFNFTEVDVCAqIEZST00gYWdnX2NzdiBjIEpP SU4gYWdnX3RleHQgdCBPTiAodC5hID0gYy5hKSBPUkRFUiBCWSBjLmE7CiAtLSBlcnJvciBjb250 ZXh0IHJlcG9ydCB0ZXN0cwogU0VMRUNUICogRlJPTSBhZ2dfYmFkOyAgICAgICAgICAgICAgIC0t IEVSUk9SCiAKLS0tIG9uX2Vycm9yIGFuZCBsb2dfdmVyYm9zaXR5IHRlc3RzCistLSBvbl9lcnJv ciwgbG9nX3ZlcmJvc2l0eSBhbmQgcmVqZWN0X2xpbWl0IHRlc3RzCiBBTFRFUiBGT1JFSUdOIFRB QkxFIGFnZ19iYWQgT1BUSU9OUyAoQUREIG9uX2Vycm9yICdpZ25vcmUnKTsKIFNFTEVDVCAqIEZS T00gYWdnX2JhZDsKIEFMVEVSIEZPUkVJR04gVEFCTEUgYWdnX2JhZCBPUFRJT05TIChBREQgbG9n X3ZlcmJvc2l0eSAnc2lsZW50Jyk7CiBTRUxFQ1QgKiBGUk9NIGFnZ19iYWQ7Citcc2V0IGZpbGVu YW1lIDphYnNfc3JjZGlyICcvZGF0YS9hZ2cuYmFkMicKK0FMVEVSIEZPUkVJR04gVEFCTEUgYWdn X2JhZCBPUFRJT05TIChTRVQgZmlsZW5hbWUgOidmaWxlbmFtZScsIEFERCByZWplY3RfbGltaXQg JzEnKTsKK1NFTEVDVCAqIEZST00gYWdnX2JhZDsKK0FMVEVSIEZPUkVJR04gVEFCTEUgYWdnX2Jh ZCBPUFRJT05TIChTRVQgcmVqZWN0X2xpbWl0ICcyJyk7CitTRUxFQ1QgKiBGUk9NIGFnZ19iYWQ7 CiBBTkFMWVpFIGFnZ19iYWQ7CiAKIC0tIG1pc2MgcXVlcnkgdGVzdHMKZGlmZiAtLWdpdCBhL2Rv Yy9zcmMvc2dtbC9maWxlLWZkdy5zZ21sIGIvZG9jL3NyYy9zZ21sL2ZpbGUtZmR3LnNnbWwKaW5k ZXggYmIzNTc5YjA3Ny4uODgyZDlhNzZkMiAxMDA2NDQKLS0tIGEvZG9jL3NyYy9zZ21sL2ZpbGUt ZmR3LnNnbWwKKysrIGIvZG9jL3NyYy9zZ21sL2ZpbGUtZmR3LnNnbWwKQEAgLTEzOCw2ICsxMzgs MTggQEAKICAgIDwvbGlzdGl0ZW0+CiAgIDwvdmFybGlzdGVudHJ5PgogCisgIDx2YXJsaXN0ZW50 cnk+CisgICA8dGVybT48bGl0ZXJhbD5yZWplY3RfbGltaXQ8L2xpdGVyYWw+PC90ZXJtPgorCisg ICA8bGlzdGl0ZW0+CisgICAgPHBhcmE+CisgICAgIFNwZWNpZmllcyB0aGUgbWF4aW11bSBudW1i ZXIgb2YgZXJyb3JzIHRvbGVyYXRlZCB3aGlsZSBjb252ZXJ0aW5nIGEgY29sdW1uJ3MKKyAgICAg aW5wdXQgdmFsdWUgdG8gaXRzIGRhdGEgdHlwZSwgdGhlIHNhbWUgYXMgPGNvbW1hbmQ+Q09QWTwv Y29tbWFuZD4ncworICAgIDxsaXRlcmFsPlJFSkVDVF9MSU1JVDwvbGl0ZXJhbD4gb3B0aW9uLgor ICAgIDwvcGFyYT4KKyAgIDwvbGlzdGl0ZW0+CisgIDwvdmFybGlzdGVudHJ5PgorCiAgIDx2YXJs aXN0ZW50cnk+CiAgICA8dGVybT48bGl0ZXJhbD5sb2dfdmVyYm9zaXR5PC9saXRlcmFsPjwvdGVy bT4KIApkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvY29tbWFuZHMvY29weS5jIGIvc3JjL2JhY2tl bmQvY29tbWFuZHMvY29weS5jCmluZGV4IDM0ODViYTg2NjMuLmM4OGY1ODc0MmQgMTAwNjQ0Ci0t LSBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL2NvcHkuYworKysgYi9zcmMvYmFja2VuZC9jb21tYW5k cy9jb3B5LmMKQEAgLTQyMCwxMSArNDIwLDI0IEBAIGRlZkdldENvcHlPbkVycm9yQ2hvaWNlKERl ZkVsZW0gKmRlZiwgUGFyc2VTdGF0ZSAqcHN0YXRlLCBib29sIGlzX2Zyb20pCiAKIC8qCiAgKiBF eHRyYWN0IFJFSkVDVF9MSU1JVCB2YWx1ZSBmcm9tIGEgRGVmRWxlbS4KKyAqCisgKiBTaW5jZSBm b3JlaWduIHRhYmxlIG9wdGlvbnMgbXVzdCBiZSBzaW5nbGUtcXVvdGVkIHZhbHVlcywgdGhpcyBm dW5jdGlvbgorICogYWNjZXB0cyBib3RoIGludDY0IGFuZCBzdHJpbmcgdmFsdWUuCiAgKi8KIHN0 YXRpYyBpbnQ2NAogZGVmR2V0Q29weVJlamVjdExpbWl0T3B0aW9uKERlZkVsZW0gKmRlZikKIHsK LQlpbnQ2NAkJcmVqZWN0X2xpbWl0ID0gZGVmR2V0SW50NjQoZGVmKTsKKwlpbnQ2NAkJcmVqZWN0 X2xpbWl0OworCisJaWYgKGRlZi0+YXJnID09IE5VTEwpCisJCWVyZXBvcnQoRVJST1IsCisJCQkJ KGVycmNvZGUoRVJSQ09ERV9TWU5UQVhfRVJST1IpLAorCQkJCSBlcnJtc2coIiVzIHJlcXVpcmVz IGEgbnVtZXJpYyB2YWx1ZSIsCisJCQkJCQlkZWYtPmRlZm5hbWUpKSk7CisJZWxzZSBpZiAobm9k ZVRhZyhkZWYtPmFyZykgPT0gVF9TdHJpbmcpCisJCXJlamVjdF9saW1pdCA9IHBnX3N0cnRvaW50 NjQoc3RyVmFsKGRlZi0+YXJnKSk7CisJZWxzZQorCQlyZWplY3RfbGltaXQgPSBkZWZHZXRJbnQ2 NChkZWYpOwogCiAJaWYgKHJlamVjdF9saW1pdCA8PSAwKQogCQllcmVwb3J0KEVSUk9SLAotLSAK Mi4zOS4yCgo= --=_0d6ec320693688e4b4d8b579d45aa934--