public inbox for [email protected]
help / color / mirror / Atom feedFrom: torikoshia <[email protected]>
To: Masahiko Sawada <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: jian he <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
Date: Wed, 17 Apr 2024 16:28:29 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAD21AoD+QaDS8egMrpgmQrLuUPT_1Dz=g3PF-Y8EeVvGudtSKA@mail.gmail.com>
References: <[email protected]>
<CAPpHfdvfpON-osWgDg-gsJGi9MnC4XdAWCdcH1vJcq9LOa97cw@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAPpHfduYDAmgccWM--A2md-zS-6zP_H41-xtUskV_daBfpAk_Q@mail.gmail.com>
<[email protected]>
<CAD21AoD474GqjdoUkm8kU0e8xO__A-BHR1=_QyCAmAkcKoHY1w@mail.gmail.com>
<CACJufxFyM-TPsaXPg6-b_Ps0=bnAi9xyhvVKT1UtuF-qyepEnA@mail.gmail.com>
<[email protected]>
<CAPpHfds-WExDYWUvtBfb=Q9tjp4gcH4-noDgo6YCNhiR-DmJVw@mail.gmail.com>
<CAD21AoAHeBTDtzMUkUkPDZknwjQz0XUc8taERsbiyh==75R+jw@mail.gmail.com>
<CAD21AoCRL-yM2NZJkfxeJGhx6r_P+dLdn0H09dE9siMKUhzoTQ@mail.gmail.com>
<[email protected]>
<CAD21AoBpD+8HJtdiwFiNBfXAUfON9p3H7KwJgoCk6Y6F-MiqYg@mail.gmail.com>
<[email protected]>
<CAD21AoC=n4xR3+KQiqodnfT9chSB62XwZqmMff39H=x9DS4scQ@mail.gmail.com>
<[email protected]>
<CAD21AoD+QaDS8egMrpgmQrLuUPT_1Dz=g3PF-Y8EeVvGudtSKA@mail.gmail.com>
On 2024-04-16 13:16, Masahiko Sawada wrote:
> On Tue, Apr 2, 2024 at 7:34 PM torikoshia <[email protected]>
> wrote:
>>
>> On 2024-04-01 11:31, Masahiko Sawada wrote:
>> > On Fri, Mar 29, 2024 at 11:54 AM torikoshia
>> > <[email protected]> wrote:
>> >>
>> >> On 2024-03-28 21:54, Masahiko Sawada wrote:
>> >> > On Thu, Mar 28, 2024 at 9:38 PM torikoshia <[email protected]>
>> >> > wrote:
>> >> >>
>> >> >> On 2024-03-28 10:20, Masahiko Sawada wrote:
>> >> >> > Hi,
>> >> >> >
>> >> >> > On Thu, Jan 18, 2024 at 5:33 PM Masahiko Sawada <[email protected]>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Thu, Jan 18, 2024 at 4:59 PM Alexander Korotkov
>> >> >> >> <[email protected]> wrote:
>> >> >> >> >
>> >> >> >> > On Thu, Jan 18, 2024 at 4:16 AM torikoshia <[email protected]> wrote:
>> >> >> >> > > On 2024-01-18 10:10, jian he wrote:
>> >> >> >> > > > On Thu, Jan 18, 2024 at 8:57 AM Masahiko Sawada <[email protected]>
>> >> >> >> > > > wrote:
>> >> >> >> > > >> On Thu, Jan 18, 2024 at 6:38 AM Tom Lane <[email protected]> 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.
>> >
>> > Probably should we change the doc as well since ON_ERROR value doesn't
>> > necessarily need to be single-quoted?
>>
>> Agreed.
>> Since it seems this issue is independent from the omission of ON_ERROR
>> option value, attached a separate patch.
>>
>
> Thank you for the patches! These patches look good to me. I'll push
> them, barring any objections.
>
> Regards,
Thanks for your review and apply!
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
view thread (75+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox