On Tue, Mar 12, 2024 at 12:54:29PM +0530, Bharath Rupireddy wrote: > +1. But, do you want to add them now or later as a separate > patch/discussion altogether?
The attached 0003 is what I had in mind: - Simplification of the LOG generated with quotes applied around the column name, don't see much need to add the relation name, either, for consistency and because the knowledge is known in the query. - A few more tests. - Some doc changes. >> Wouldn't it be better to squash the patches together, by the way? > > I guess not. They are two different things IMV. Well, 0001 is sitting doing nothing because the COPY code does not make use of it internally. -- Michael
From c45474726e084faf876a319485995ce84eef8293 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> Date: Mon, 11 Mar 2024 11:37:49 +0000 Subject: [PATCH v6 1/3] Add LOG_VERBOSITY option to COPY command This commit adds a new option LOG_VERBOSITY to set the verbosity of logged messages by COPY command. A value of 'verbose' can be used to emit more informative messages by the command, while the value of 'default (which is the default) can be used to not log any additional messages. More values such as 'terse', 'row_details' etc. can be added based on the need to the LOG_VERBOSITY option. An upcoming commit for emitting more info on soft errors by COPY FROM command with ON_ERROR 'ignore' uses this. Author: Bharath Rupireddy Reviewed-by: Michael Paquier, Masahiko Sawada Reviewed-by: Atsushi Torikoshi Discussion: https://www.postgresql.org/message-id/CALj2ACXNA0focNeriYRvQQaCGc4CsTuOnFbzF9LqTKNWxuJdhA%40mail.gmail.com --- src/include/commands/copy.h | 10 ++++++++ src/backend/commands/copy.c | 38 +++++++++++++++++++++++++++++ src/bin/psql/tab-complete.c | 6 ++++- src/test/regress/expected/copy2.out | 8 ++++++ src/test/regress/sql/copy2.sql | 2 ++ doc/src/sgml/ref/copy.sgml | 14 +++++++++++ src/tools/pgindent/typedefs.list | 1 + 7 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index b3da3cb0be..99d183fa4d 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -40,6 +40,15 @@ typedef enum CopyOnErrorChoice COPY_ON_ERROR_IGNORE, /* ignore errors */ } CopyOnErrorChoice; +/* + * Represents verbosity of logged messages by COPY command. + */ +typedef enum CopyLogVerbosityChoice +{ + COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages, default */ + COPY_LOG_VERBOSITY_VERBOSE, /* logs additional messages */ +} CopyLogVerbosityChoice; + /* * A struct to hold COPY options, in a parsed form. All of these are related * to formatting, except for 'freeze', which doesn't really belong here, but @@ -73,6 +82,7 @@ typedef struct CopyFormatOptions bool *force_null_flags; /* per-column CSV FN flags */ bool convert_selectively; /* do selective binary conversion? */ CopyOnErrorChoice on_error; /* what to do when error happened */ + CopyLogVerbosityChoice log_verbosity; /* verbosity of logged messages */ List *convert_select; /* list of column names (can be NIL) */ } CopyFormatOptions; diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 056b6733c8..23eb8c9c79 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -428,6 +428,36 @@ defGetCopyOnErrorChoice(DefElem *def, ParseState *pstate, bool is_from) return COPY_ON_ERROR_STOP; /* keep compiler quiet */ } +/* + * Extract a CopyLogVerbosityChoice value from a DefElem. + */ +static CopyLogVerbosityChoice +defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate) +{ + char *sval; + + /* + * If no parameter value given, assume the default value. + */ + if (def->arg == NULL) + return COPY_LOG_VERBOSITY_DEFAULT; + + /* + * Allow "default", or "verbose" values. + */ + sval = defGetString(def); + if (pg_strcasecmp(sval, "default") == 0) + return COPY_LOG_VERBOSITY_DEFAULT; + if (pg_strcasecmp(sval, "verbose") == 0) + return COPY_LOG_VERBOSITY_VERBOSE; + + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("COPY LOG_VERBOSITY \"%s\" not recognized", sval), + parser_errposition(pstate, def->location))); + return COPY_LOG_VERBOSITY_DEFAULT; /* keep compiler quiet */ +} + /* * Process the statement option list for COPY. * @@ -454,6 +484,7 @@ ProcessCopyOptions(ParseState *pstate, bool freeze_specified = false; bool header_specified = false; bool on_error_specified = false; + bool log_verbosity_specified = false; ListCell *option; /* Support external use for option sanity checking */ @@ -613,6 +644,13 @@ ProcessCopyOptions(ParseState *pstate, on_error_specified = true; opts_out->on_error = defGetCopyOnErrorChoice(defel, pstate, is_from); } + else if (strcmp(defel->defname, "log_verbosity") == 0) + { + if (log_verbosity_specified) + errorConflictingDefElem(defel, pstate); + log_verbosity_specified = true; + opts_out->log_verbosity = defGetCopyLogVerbosityChoice(defel, pstate); + } else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 73133ce735..9305800340 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2901,7 +2901,7 @@ psql_completion(const char *text, int start, int end) COMPLETE_WITH("FORMAT", "FREEZE", "DELIMITER", "NULL", "HEADER", "QUOTE", "ESCAPE", "FORCE_QUOTE", "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING", "DEFAULT", - "ON_ERROR"); + "ON_ERROR", "LOG_VERBOSITY"); /* Complete COPY <sth> FROM|TO filename WITH (FORMAT */ else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "FORMAT")) @@ -2911,6 +2911,10 @@ psql_completion(const char *text, int start, int end) else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "ON_ERROR")) COMPLETE_WITH("stop", "ignore"); + /* Complete COPY <sth> FROM filename WITH (LOG_VERBOSITY */ + else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "LOG_VERBOSITY")) + COMPLETE_WITH("default", "verbose"); + /* Complete COPY <sth> FROM <sth> WITH (<options>) */ else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny, "WITH", MatchAny)) COMPLETE_WITH("WHERE"); diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index 25c401ce34..62406ef827 100644 --- a/src/test/regress/expected/copy2.out +++ b/src/test/regress/expected/copy2.out @@ -81,6 +81,10 @@ COPY x from stdin (on_error ignore, on_error ignore); ERROR: conflicting or redundant options LINE 1: COPY x from stdin (on_error ignore, on_error ignore); ^ +COPY x from stdin (log_verbosity 'default', log_verbosity 'verbose'); +ERROR: conflicting or redundant options +LINE 1: COPY x from stdin (log_verbosity 'default', log_verbosity 'v... + ^ -- incorrect options COPY x to stdin (format BINARY, delimiter ','); ERROR: cannot specify DELIMITER in BINARY mode @@ -108,6 +112,10 @@ COPY x to stdin (format BINARY, on_error unsupported); ERROR: COPY ON_ERROR cannot be used with COPY TO LINE 1: COPY x to stdin (format BINARY, on_error unsupported); ^ +COPY x to stdout (log_verbosity 'unsupported'); +ERROR: COPY LOG_VERBOSITY "unsupported" not recognized +LINE 1: COPY x to stdout (log_verbosity 'unsupported'); + ^ -- too many columns in column list: should fail COPY x (a, b, c, d, e, d, c) from stdin; ERROR: column "d" specified more than once diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql index b5e549e856..5116157cc9 100644 --- a/src/test/regress/sql/copy2.sql +++ b/src/test/regress/sql/copy2.sql @@ -67,6 +67,7 @@ COPY x from stdin (force_null (a), force_null (b)); COPY x from stdin (convert_selectively (a), convert_selectively (b)); COPY x from stdin (encoding 'sql_ascii', encoding 'sql_ascii'); COPY x from stdin (on_error ignore, on_error ignore); +COPY x from stdin (log_verbosity 'default', log_verbosity 'verbose'); -- incorrect options COPY x to stdin (format BINARY, delimiter ','); @@ -80,6 +81,7 @@ COPY x to stdin (format CSV, force_not_null(a)); COPY x to stdout (format TEXT, force_null(a)); COPY x to stdin (format CSV, force_null(a)); COPY x to stdin (format BINARY, on_error unsupported); +COPY x to stdout (log_verbosity 'unsupported'); -- too many columns in column list: should fail COPY x (a, b, c, d, e, d, c) from stdin; diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 55764fc1f2..67ba6212fe 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -45,6 +45,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable FORCE_NULL { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * } ON_ERROR '<replaceable class="parameter">error_action</replaceable>' ENCODING '<replaceable class="parameter">encoding_name</replaceable>' + LOG_VERBOSITY [ <replaceable class="parameter">mode</replaceable> ] </synopsis> </refsynopsisdiv> @@ -415,6 +416,19 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable </listitem> </varlistentry> + <varlistentry> + <term><literal>LOG_VERBOSITY</literal></term> + <listitem> + <para> + Sets the verbosity of logged messages by <command>COPY</command> command. + A <replaceable class="parameter">mode</replaceable> value of + <literal>verbose</literal> can be used to emit more informative messages + by the command, while value of <literal>default</literal> (which is the + default) can be used to not log any additional messages. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>WHERE</literal></term> <listitem> diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index aa7a25b8f8..549378c8ad 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -479,6 +479,7 @@ CopyFromState CopyFromStateData CopyHeaderChoice CopyInsertMethod +CopyLogVerbosityChoice CopyMultiInsertBuffer CopyMultiInsertInfo CopyOnErrorChoice -- 2.43.0
From 1f087fb0cd740e927c6a475996430e2bb12de845 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> Date: Mon, 11 Mar 2024 11:53:44 +0000 Subject: [PATCH v6 2/3] Add detailed info when COPY skips soft errors This commit emits individual info like line number and column name when COPY skips soft errors. Because, the summary containing the total rows skipped isn't enough for the users to know what exactly are the malformed rows in the input data. Author: Bharath Rupireddy Reviewed-by: Michael Paquier, Masahiko Sawada Reviewed-by: Atsushi Torikoshi Discussion: https://www.postgresql.org/message-id/CALj2ACUk700cYhx1ATRQyRw-fBM%2BaRo6auRAitKGff7XNmYfqQ%40mail.gmail.com --- src/backend/commands/copyfromparse.c | 10 ++++++++++ src/test/regress/expected/copy2.out | 7 ++++++- src/test/regress/sql/copy2.sql | 4 +++- doc/src/sgml/ref/copy.sgml | 12 +++++++++--- 4 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 5682d5d054..a7ad6c17c8 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -967,7 +967,17 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext, (Node *) cstate->escontext, &values[m])) { + Assert(cstate->opts.on_error != COPY_ON_ERROR_STOP); + cstate->num_errors++; + + if (cstate->opts.log_verbosity == COPY_LOG_VERBOSITY_VERBOSE) + ereport(NOTICE, + errmsg("detected data type incompatibility at line number %llu for column %s; COPY %s", + (unsigned long long) cstate->cur_lineno, + cstate->cur_attname, + cstate->cur_relname)); + return true; } diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index 62406ef827..c6655000e4 100644 --- a/src/test/regress/expected/copy2.out +++ b/src/test/regress/expected/copy2.out @@ -737,7 +737,12 @@ CREATE TABLE check_ign_err (n int, m int[], k int); COPY check_ign_err FROM STDIN WITH (on_error stop); ERROR: invalid input syntax for type integer: "a" CONTEXT: COPY check_ign_err, line 2, column n: "a" -COPY check_ign_err FROM STDIN WITH (on_error ignore); +-- tests for options on_error and log_verbosity +COPY check_ign_err FROM STDIN WITH (on_error ignore, log_verbosity 'verbose'); +NOTICE: detected data type incompatibility at line number 2 for column n; COPY check_ign_err +NOTICE: detected data type incompatibility at line number 3 for column k; COPY check_ign_err +NOTICE: detected data type incompatibility at line number 4 for column m; COPY check_ign_err +NOTICE: detected data type incompatibility at line number 5 for column n; COPY check_ign_err NOTICE: 4 rows were skipped due to data type incompatibility SELECT * FROM check_ign_err; n | m | k diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql index 5116157cc9..b637a5b3bb 100644 --- a/src/test/regress/sql/copy2.sql +++ b/src/test/regress/sql/copy2.sql @@ -510,7 +510,9 @@ a {2} 2 5 {5} 5 \. -COPY check_ign_err FROM STDIN WITH (on_error ignore); + +-- tests for options on_error and log_verbosity +COPY check_ign_err FROM STDIN WITH (on_error ignore, log_verbosity 'verbose'); 1 {1} 1 a {2} 2 3 {3} 3333333333 diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 67ba6212fe..ed7fdc59fb 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -398,8 +398,12 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable when the <literal>FORMAT</literal> is <literal>text</literal> or <literal>csv</literal>. </para> <para> - A <literal>NOTICE</literal> message containing the ignored row count is emitted at the end - of the <command>COPY FROM</command> if at least one row was discarded. + A <literal>NOTICE</literal> message containing the ignored row count is + emitted at the end of the <command>COPY FROM</command> if at least one + row was discarded. When <literal>LOG_VERBOSITY</literal> option is set to + <literal>verbose</literal>, a <literal>NOTICE</literal> message + containing the line number and column name (whose input conversion has + failed) is emitted for each discarded row. </para> </listitem> </varlistentry> @@ -424,7 +428,9 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable A <replaceable class="parameter">mode</replaceable> value of <literal>verbose</literal> can be used to emit more informative messages by the command, while value of <literal>default</literal> (which is the - default) can be used to not log any additional messages. + default) can be used to not log any additional messages. As an example, + see its usage for <command>COPY FROM</command> command when + <literal>ON_ERROR</literal> option is set to <literal>ignore</literal>. </para> </listitem> </varlistentry> -- 2.43.0
From 96368b5af3c4186a065770cec96fcb0ea60dafd0 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 13 Mar 2024 08:45:20 +0900
Subject: [PATCH v6 3/3] Some docs and comments updates
---
src/backend/commands/copyfromparse.c | 6 ++----
src/test/regress/expected/copy2.out | 15 +++++++++------
src/test/regress/sql/copy2.sql | 3 +++
doc/src/sgml/ref/copy.sgml | 18 ++++++++++--------
4 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index a7ad6c17c8..44f615de28 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -973,11 +973,9 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
if (cstate->opts.log_verbosity == COPY_LOG_VERBOSITY_VERBOSE)
ereport(NOTICE,
- errmsg("detected data type incompatibility at line number %llu for column %s; COPY %s",
+ errmsg("data type incompatibility at line %llu for column \"%s\"",
(unsigned long long) cstate->cur_lineno,
- cstate->cur_attname,
- cstate->cur_relname));
-
+ cstate->cur_attname));
return true;
}
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index c6655000e4..af669fedbe 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -739,17 +739,20 @@ ERROR: invalid input syntax for type integer: "a"
CONTEXT: COPY check_ign_err, line 2, column n: "a"
-- tests for options on_error and log_verbosity
COPY check_ign_err FROM STDIN WITH (on_error ignore, log_verbosity 'verbose');
-NOTICE: detected data type incompatibility at line number 2 for column n; COPY check_ign_err
-NOTICE: detected data type incompatibility at line number 3 for column k; COPY check_ign_err
-NOTICE: detected data type incompatibility at line number 4 for column m; COPY check_ign_err
-NOTICE: detected data type incompatibility at line number 5 for column n; COPY check_ign_err
-NOTICE: 4 rows were skipped due to data type incompatibility
+NOTICE: data type incompatibility at line 2 for column "n"
+NOTICE: data type incompatibility at line 3 for column "k"
+NOTICE: data type incompatibility at line 4 for column "m"
+NOTICE: data type incompatibility at line 5 for column "n"
+NOTICE: data type incompatibility at line 7 for column "m"
+NOTICE: data type incompatibility at line 8 for column "k"
+NOTICE: 6 rows were skipped due to data type incompatibility
SELECT * FROM check_ign_err;
n | m | k
---+-----+---
1 | {1} | 1
5 | {5} | 5
-(2 rows)
+ 8 | {8} | 8
+(3 rows)
-- test datatype error that can't be handled as soft: should fail
CREATE TABLE hard_err(foo widget);
diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql
index b637a5b3bb..4fb736535d 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -519,6 +519,9 @@ a {2} 2
4 {a, 4} 4
5 {5} 5
+6 a
+7 {7} a
+8 {8} 8
\.
SELECT * FROM check_ign_err;
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index ed7fdc59fb..7d594d275e 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -402,8 +402,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
emitted at the end of the <command>COPY FROM</command> if at least one
row was discarded. When <literal>LOG_VERBOSITY</literal> option is set to
<literal>verbose</literal>, a <literal>NOTICE</literal> message
- containing the line number and column name (whose input conversion has
- failed) is emitted for each discarded row.
+ containing the line of the input file and the column name whose input
+ conversion has failed is emitted for each discarded row.
</para>
</listitem>
</varlistentry>
@@ -424,13 +424,15 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<term><literal>LOG_VERBOSITY</literal></term>
<listitem>
<para>
- Sets the verbosity of logged messages by <command>COPY</command> command.
+ Sets the verbosity of some of the messages logged by a
+ <command>COPY</command> command.
A <replaceable class="parameter">mode</replaceable> value of
- <literal>verbose</literal> can be used to emit more informative messages
- by the command, while value of <literal>default</literal> (which is the
- default) can be used to not log any additional messages. As an example,
- see its usage for <command>COPY FROM</command> command when
- <literal>ON_ERROR</literal> option is set to <literal>ignore</literal>.
+ <literal>verbose</literal> can be used to emit more informative messages.
+ <literal>default</literal> will not log any additional messages.
+ </para>
+ <para>
+ This is currently used in <command>COPY FROM</command> command when
+ <literal>ON_ERROR</literal> is set to <literal>ignore</literal>.
</para>
</listitem>
</varlistentry>
--
2.43.0
signature.asc
Description: PGP signature
