On 2024-04-01 11:31, Masahiko Sawada wrote:
On Fri, Mar 29, 2024 at 11:54 AM torikoshia
<torikos...@oss.nttdata.com> wrote:
On 2024-03-28 21:54, Masahiko Sawada wrote:
> On Thu, Mar 28, 2024 at 9:38 PM torikoshia <torikos...@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.m...@gmail.com>
>> > wrote:
>> >>
>> >> On Thu, Jan 18, 2024 at 4:59 PM Alexander Korotkov
>> >> <aekorot...@gmail.com> wrote:
>> >> >
>> >> > On Thu, Jan 18, 2024 at 4:16 AM torikoshia <torikos...@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.m...@gmail.com>
>> >> > > > wrote:
>> >> > > >> On Thu, Jan 18, 2024 at 6:38 AM Tom Lane <t...@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.
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.
The rest looks good to me.
Alexander, what do you think about this change as you're the committer
of this feature?
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
From 1b4bec3c2223246ec59ffb9eb7de2f1de27315f7 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@oss.nttdata.com>
Date: Fri, 29 Mar 2024 11:36:12 +0900
Subject: [PATCH v1] Disallow ON_ERROR option without value
Currently ON_ERROR option of COPY allows to omit its value,
but the syntax synopsis in the documentation requires it.
Since it seems non-boolean parameters usually require its value
and it's not obvious what happens when value of ON_ERROR is
omitted, this patch disallows ON_ERROR without its value.
---
src/backend/commands/copy.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 28cf8b040a..2719bf28b7 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -392,7 +392,7 @@ defGetCopyHeaderChoice(DefElem *def, bool is_from)
static CopyOnErrorChoice
defGetCopyOnErrorChoice(DefElem *def, ParseState *pstate, bool is_from)
{
- char *sval;
+ char *sval = defGetString(def);
if (!is_from)
ereport(ERROR,
@@ -400,16 +400,9 @@ defGetCopyOnErrorChoice(DefElem *def, ParseState *pstate, bool is_from)
errmsg("COPY ON_ERROR cannot be used with COPY TO"),
parser_errposition(pstate, def->location)));
- /*
- * If no parameter value given, assume the default value.
- */
- if (def->arg == NULL)
- return COPY_ON_ERROR_STOP;
-
/*
* Allow "stop", or "ignore" values.
*/
- sval = defGetString(def);
if (pg_strcasecmp(sval, "stop") == 0)
return COPY_ON_ERROR_STOP;
if (pg_strcasecmp(sval, "ignore") == 0)
base-commit: 0075d78947e3800c5a807f48fd901f16db91101b
--
2.39.2
From 840152c20d47220f106d5fe14af4a86cec99987e Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@oss.nttdata.com>
Date: Tue, 2 Apr 2024 19:11:01 +0900
Subject: [PATCH v1] doc: Fix COPY ON_ERROR option syntax synopsis.
Since ON_ERROR value doesn't require quotations, this patch removes them.
Oversight in b725b7eec43.
---
doc/src/sgml/ref/copy.sgml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 33ce7c4ea6..1ce19668d8 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -43,7 +43,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
FORCE_QUOTE { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * }
FORCE_NOT_NULL { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * }
FORCE_NULL { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * }
- ON_ERROR '<replaceable class="parameter">error_action</replaceable>'
+ ON_ERROR <replaceable class="parameter">error_action</replaceable>
ENCODING '<replaceable class="parameter">encoding_name</replaceable>'
LOG_VERBOSITY <replaceable class="parameter">mode</replaceable>
</synopsis>
base-commit: 0075d78947e3800c5a807f48fd901f16db91101b
--
2.39.2
