MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_23be9bef3a2929052e78723c6449f432"
Content-Transfer-Encoding: 8bit
Date: Fri, 29 Mar 2024 11:54:10 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, jian he
 <jian.universality@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Kyotaro
 Horiguchi <horikyota.ntt@gmail.com>, vignesh21@gmail.com,
 lena.ribackina@yandex.ru, dam.bel07@gmail.com, zhihuifan1213@163.com,
 daniel@yesql.se, pgsql-hackers@postgresql.org, andres@anarazel.de,
 anisimow.d@gmail.com, HukuToc@gmail.com, pgsql-hackers@lists.postgresql.org,
 a.lepikhov@postgrespro.ru
Subject: Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
In-Reply-To: <CAD21AoBpD+8HJtdiwFiNBfXAUfON9p3H7KwJgoCk6Y6F-MiqYg@mail.gmail.com>
References: <d73736e0614b4cf2330786b9c2ff93a5@oss.nttdata.com>
 <CAPpHfdvfpON-osWgDg-gsJGi9MnC4XdAWCdcH1vJcq9LOa97cw@mail.gmail.com>
 <762d7dd4d5aa9e5ecffec2ae6a255a28@oss.nttdata.com>
 <20240117.164859.2242646601795501168.horikyota.ntt@gmail.com>
 <CAPpHfduYDAmgccWM--A2md-zS-6zP_H41-xtUskV_daBfpAk_Q@mail.gmail.com>
 <2070915.1705527477@sss.pgh.pa.us>
 <CAD21AoD474GqjdoUkm8kU0e8xO__A-BHR1=_QyCAmAkcKoHY1w@mail.gmail.com>
 <CACJufxFyM-TPsaXPg6-b_Ps0=bnAi9xyhvVKT1UtuF-qyepEnA@mail.gmail.com>
 <350926525700756634e7c7b003fb1694@oss.nttdata.com>
 <CAPpHfds-WExDYWUvtBfb=Q9tjp4gcH4-noDgo6YCNhiR-DmJVw@mail.gmail.com>
 <CAD21AoAHeBTDtzMUkUkPDZknwjQz0XUc8taERsbiyh==75R+jw@mail.gmail.com>
 <CAD21AoCRL-yM2NZJkfxeJGhx6r_P+dLdn0H09dE9siMKUhzoTQ@mail.gmail.com>
 <a9770bf57646d90dedc3d54cf32634b2@oss.nttdata.com>
 <CAD21AoBpD+8HJtdiwFiNBfXAUfON9p3H7KwJgoCk6Y6F-MiqYg@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <b05c68160ee6bbc75fdee713947ff5bf@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/b05c68160ee6bbc75fdee713947ff5bf%40oss.nttdata.com>
Precedence: bulk

--=_23be9bef3a2929052e78723c6449f432
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=UTF-8;
 format=flowed

On 2024-03-28 21:54, Masahiko Sawada wrote:
> On Thu, Mar 28, 2024 at 9:38 PM torikoshia <torikoshia@oss.nttdata.com> 
> wrote:
>> 
>> On 2024-03-28 10:20, Masahiko Sawada wrote:
>> > Hi,
>> >
>> > On Thu, Jan 18, 2024 at 5:33 PM Masahiko Sawada <sawada.mshk@gmail.com>
>> > wrote:
>> >>
>> >> On Thu, Jan 18, 2024 at 4:59 PM Alexander Korotkov
>> >> <aekorotkov@gmail.com> wrote:
>> >> >
>> >> > On Thu, Jan 18, 2024 at 4:16 AM torikoshia <torikoshia@oss.nttdata.com> wrote:
>> >> > > On 2024-01-18 10:10, jian he wrote:
>> >> > > > On Thu, Jan 18, 2024 at 8:57 AM Masahiko Sawada <sawada.mshk@gmail.com>
>> >> > > > wrote:
>> >> > > >> On Thu, Jan 18, 2024 at 6:38 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >> > > >> > Kyotaro-san's suggestion isn't bad, though I might shorten it to
>> >> > > >> > error_action {error|ignore|log} (or perhaps "stop" instead of "error")?
>> >> > > >> > You will need a separate parameter anyway to specify the destination
>> >> > > >> > of "log", unless "none" became an illegal table name when I wasn't
>> >> > > >> > looking.  I don't buy that one parameter that has some special values
>> >> > > >> > while other values could be names will be a good design.  Moreover,
>> >> > > >> > what if we want to support (say) log-to-file along with log-to-table?
>> >> > > >> > Trying to distinguish a file name from a table name without any other
>> >> > > >> > context seems impossible.
>> >> > > >>
>> >> > > >> I've been thinking we can add more values to this option to log errors
>> >> > > >> not only to the server logs but also to the error table (not sure
>> >> > > >> details but I imagined an error table is created for each table on
>> >> > > >> error), without an additional option for the destination name. The
>> >> > > >> values would be like error_action {error|ignore|save-logs|save-table}.
>> >> > > >>
>> >> > > >
>> >> > > > another idea:
>> >> > > > on_error {error|ignore|other_future_option}
>> >> > > > if not specified then by default ERROR.
>> >> > > > You can also specify ERROR or IGNORE for now.
>> >> > > >
>> >> > > > I agree, the parameter "error_action" is better than "location".
>> >> > >
>> >> > > I'm not sure whether error_action or on_error is better, but either way
>> >> > > "error_action error" and "on_error error" seems a bit odd to me.
>> >> > > I feel "stop" is better for both cases as Tom suggested.
>> >> >
>> >> > OK.  What about this?
>> >> > on_error {stop|ignore|other_future_option}
>> >> > where other_future_option might be compound like "file 'copy.log'" or
>> >> > "table 'copy_log'".
>> >>
>> >> +1
>> >>
>> >
>> > I realized that ON_ERROR syntax synoposis in the documentation is not
>> > correct. The option doesn't require the value to be quoted and the
>> > value can be omitted. The attached patch fixes it.
>> >
>> > Regards,
>> 
>> Thanks!
>> 
>> Attached patch fixes the doc, but I'm wondering perhaps it might be
>> better to modify the codes to prohibit abbreviation of the value.
>> 
>> When seeing the query which abbreviates ON_ERROR value, I feel it's 
>> not
>> obvious what happens compared to other options which tolerates
>> abbreviation of the value such as FREEZE or HEADER.
>> 
>>    COPY t1 FROM stdin WITH (ON_ERROR);
>> 
>> What do you think?
> 
> Indeed. Looking at options of other commands such as VACUUM and
> EXPLAIN, I can see that we can omit a boolean value, but non-boolean
> parameters require its value. The HEADER option is not a pure boolean
> parameter but we can omit the value. It seems to be for backward
> compatibility; it used to be a boolean parameter. I agree that the
> above example would confuse users.
> 
> Regards,

Thanks for your comment!

Attached a patch which modifies the code to prohibit omission of its 
value.

I was a little unsure about adding a regression test for this, but I 
have not added it since other COPY option doesn't test the omission of 
its value.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation
--=_23be9bef3a2929052e78723c6449f432
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=v1-0001-Disallow-ON_ERROR-option-without-value.patch
Content-Disposition: attachment;
 filename=v1-0001-Disallow-ON_ERROR-option-without-value.patch;
 size=1646

RnJvbSAxYjRiZWMzYzIyMjMyNDZlYzU5ZmZiOWViN2RlMmYxZGUyNzMxNWY3IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBBdHN1c2hpIFRvcmlrb3NoaSA8dG9yaWtvc2hpYUBvc3MubnR0
ZGF0YS5jb20+CkRhdGU6IEZyaSwgMjkgTWFyIDIwMjQgMTE6MzY6MTIgKzA5MDAKU3ViamVjdDog
W1BBVENIIHYxXSBEaXNhbGxvdyBPTl9FUlJPUiBvcHRpb24gd2l0aG91dCB2YWx1ZQoKQ3VycmVu
dGx5IE9OX0VSUk9SIG9wdGlvbiBvZiBDT1BZIGFsbG93cyB0byBvbWl0IGl0cyB2YWx1ZSwKYnV0
IHRoZSBzeW50YXggc3lub3BzaXMgaW4gdGhlIGRvY3VtZW50YXRpb24gcmVxdWlyZXMgaXQuCgpT
aW5jZSBpdCBzZWVtcyBub24tYm9vbGVhbiBwYXJhbWV0ZXJzIHVzdWFsbHkgcmVxdWlyZSBpdHMg
dmFsdWUKYW5kIGl0J3Mgbm90IG9idmlvdXMgd2hhdCBoYXBwZW5zIHdoZW4gdmFsdWUgb2YgT05f
RVJST1IgaXMKb21pdHRlZCwgdGhpcyBwYXRjaCBkaXNhbGxvd3MgT05fRVJST1Igd2l0aG91dCBp
dHMgdmFsdWUuCi0tLQogc3JjL2JhY2tlbmQvY29tbWFuZHMvY29weS5jIHwgOSArLS0tLS0tLS0K
IDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigrKSwgOCBkZWxldGlvbnMoLSkKCmRpZmYgLS1n
aXQgYS9zcmMvYmFja2VuZC9jb21tYW5kcy9jb3B5LmMgYi9zcmMvYmFja2VuZC9jb21tYW5kcy9j
b3B5LmMKaW5kZXggMjhjZjhiMDQwYS4uMjcxOWJmMjhiNyAxMDA2NDQKLS0tIGEvc3JjL2JhY2tl
bmQvY29tbWFuZHMvY29weS5jCisrKyBiL3NyYy9iYWNrZW5kL2NvbW1hbmRzL2NvcHkuYwpAQCAt
MzkyLDcgKzM5Miw3IEBAIGRlZkdldENvcHlIZWFkZXJDaG9pY2UoRGVmRWxlbSAqZGVmLCBib29s
IGlzX2Zyb20pCiBzdGF0aWMgQ29weU9uRXJyb3JDaG9pY2UKIGRlZkdldENvcHlPbkVycm9yQ2hv
aWNlKERlZkVsZW0gKmRlZiwgUGFyc2VTdGF0ZSAqcHN0YXRlLCBib29sIGlzX2Zyb20pCiB7Ci0J
Y2hhcgkgICAqc3ZhbDsKKwljaGFyCSAgICpzdmFsID0gZGVmR2V0U3RyaW5nKGRlZik7CiAKIAlp
ZiAoIWlzX2Zyb20pCiAJCWVyZXBvcnQoRVJST1IsCkBAIC00MDAsMTYgKzQwMCw5IEBAIGRlZkdl
dENvcHlPbkVycm9yQ2hvaWNlKERlZkVsZW0gKmRlZiwgUGFyc2VTdGF0ZSAqcHN0YXRlLCBib29s
IGlzX2Zyb20pCiAJCQkJIGVycm1zZygiQ09QWSBPTl9FUlJPUiBjYW5ub3QgYmUgdXNlZCB3aXRo
IENPUFkgVE8iKSwKIAkJCQkgcGFyc2VyX2VycnBvc2l0aW9uKHBzdGF0ZSwgZGVmLT5sb2NhdGlv
bikpKTsKIAotCS8qCi0JICogSWYgbm8gcGFyYW1ldGVyIHZhbHVlIGdpdmVuLCBhc3N1bWUgdGhl
IGRlZmF1bHQgdmFsdWUuCi0JICovCi0JaWYgKGRlZi0+YXJnID09IE5VTEwpCi0JCXJldHVybiBD
T1BZX09OX0VSUk9SX1NUT1A7Ci0KIAkvKgogCSAqIEFsbG93ICJzdG9wIiwgb3IgImlnbm9yZSIg
dmFsdWVzLgogCSAqLwotCXN2YWwgPSBkZWZHZXRTdHJpbmcoZGVmKTsKIAlpZiAocGdfc3RyY2Fz
ZWNtcChzdmFsLCAic3RvcCIpID09IDApCiAJCXJldHVybiBDT1BZX09OX0VSUk9SX1NUT1A7CiAJ
aWYgKHBnX3N0cmNhc2VjbXAoc3ZhbCwgImlnbm9yZSIpID09IDApCgpiYXNlLWNvbW1pdDogMDA3
NWQ3ODk0N2UzODAwYzVhODA3ZjQ4ZmQ5MDFmMTZkYjkxMTAxYgotLSAKMi4zOS4yCgo=
--=_23be9bef3a2929052e78723c6449f432--