public inbox for [email protected]
help / color / mirror / Atom feedRe: Add on_error and log_verbosity options to file_fdw
7+ messages / 4 participants
[nested] [flat]
* Re: Add on_error and log_verbosity options to file_fdw
@ 2024-09-30 15:36 Fujii Masao <[email protected]>
2024-10-02 00:27 ` Re: Add on_error and log_verbosity options to file_fdw Masahiko Sawada <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Fujii Masao @ 2024-09-30 15:36 UTC (permalink / raw)
To: torikoshia <[email protected]>; +Cc: Michael Paquier <[email protected]>; [email protected]; pgsql-hackers
On 2024/09/26 21:57, torikoshia wrote:
> Updated the patches.
Thanks for updating the patches! I’ve made some changes based on your work, which are attached.
Barring any objections, I'm thinking to push these patches.
For patches 0001 and 0003, I ran pgindent and updated the commit message.
Regarding patch 0002:
- I updated the regression test to run ANALYZE on the file_fdw foreign table
since the on_error option also affects the ANALYZE command. To ensure test
stability, the test now runs ANALYZE with log_verbosity = 'silent'.
- I removed the code that updated the count of skipped rows for
the pg_stat_progress_copy view. As far as I know, file_fdw doesn’t
currently support tracking pg_stat_progress_copy.tuples_processed.
Supporting only tuples_skipped seems inconsistent, so I suggest creating
a separate patch to extend file_fdw to track both tuples_processed and
tuples_skipped in this view.
- I refactored the for-loop handling on_error = 'ignore' in fileIterateForeignScan()
by replacing it with a goto statement for improved readability.
- I modified file_fdw to log a NOTICE message about skipped rows at the end of
ANALYZE if any rows are skipped due to the on_error = 'ignore' setting.
Regarding the "file contains XXX rows" message reported by the ANALYZE VERBOSE
command on the file_fdw foreign table, what number should be reflected in XXX,
especially when some rows are skipped due to on_error = 'ignore'?
Currently, the count only includes valid rows, excluding any skipped rows.
I haven't modified this code yet. Should we instead count all rows
(valid and erroneous) and report that total?
I noticed the code for reporting the number of skipped rows due to
on_error = 'ignore' appears in three places. I’m considering creating
a common function for this reporting to eliminate redundancy but haven’t
implemented it yet.
- I’ve updated the commit message and run pgindent.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From 6bcf56dc0556b3e9ded7200229c05c69e9c4fd6a Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email protected]>
Date: Wed, 25 Sep 2024 21:28:15 +0900
Subject: [PATCH v6 1/3] Add log_verbosity = 'silent' support to COPY command.
Previously, when the on_error option was set to ignore, the COPY command
would always log NOTICE messages for input rows discarded due to
data type incompatibility. Users had no way to suppress these messages.
This commit introduces a new log_verbosity setting, 'silent',
which prevents the COPY command from emitting NOTICE messages
when on_error = 'ignore' is used, even if rows are discarded.
This feature is particularly useful when processing malformed files
frequently, where a flood of NOTICE messages can be undesirable.
For example, when frequently loading malformed files via the COPY command
or querying foreign tables using file_fdw (with an upcoming patch to
add on_error support for file_fdw), users may prefer to suppress
these messages to reduce log noise and improve clarity.
Author: Atsushi Torikoshi
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
doc/src/sgml/ref/copy.sgml | 10 +++++++---
src/backend/commands/copy.c | 4 +++-
src/backend/commands/copyfrom.c | 3 ++-
src/bin/psql/tab-complete.c | 2 +-
src/include/commands/copy.h | 4 +++-
src/test/regress/expected/copy2.out | 4 +++-
src/test/regress/sql/copy2.sql | 4 ++++
7 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 1518af8a04..d87684a5be 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -407,6 +407,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<literal>verbose</literal>, a <literal>NOTICE</literal> message
containing the line of the input file and the column name whose input
conversion has failed is emitted for each discarded row.
+ When it is set to <literal>silent</literal>, no message is emitted
+ regarding ignored rows.
</para>
</listitem>
</varlistentry>
@@ -428,9 +430,11 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<listitem>
<para>
Specify the amount of messages emitted by a <command>COPY</command>
- command: <literal>default</literal> or <literal>verbose</literal>. If
- <literal>verbose</literal> is specified, additional messages are emitted
- during processing.
+ command: <literal>default</literal>, <literal>verbose</literal>, or
+ <literal>silent</literal>.
+ If <literal>verbose</literal> is specified, additional messages are
+ emitted during processing.
+ <literal>silent</literal> suppresses both verbose and default messages.
</para>
<para>
This is currently used in <command>COPY FROM</command> command when
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3bb579a3a4..03eb7a4eba 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -427,9 +427,11 @@ defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate)
char *sval;
/*
- * Allow "default", or "verbose" values.
+ * Allow "silent", "default", or "verbose" values.
*/
sval = defGetString(def);
+ if (pg_strcasecmp(sval, "silent") == 0)
+ return COPY_LOG_VERBOSITY_SILENT;
if (pg_strcasecmp(sval, "default") == 0)
return COPY_LOG_VERBOSITY_DEFAULT;
if (pg_strcasecmp(sval, "verbose") == 0)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 2d3462913e..47879994f7 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1320,7 +1320,8 @@ CopyFrom(CopyFromState cstate)
error_context_stack = errcallback.previous;
if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
- cstate->num_errors > 0)
+ cstate->num_errors > 0 &&
+ cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
ereport(NOTICE,
errmsg_plural("%llu row was skipped due to data type incompatibility",
"%llu rows were skipped due to data type incompatibility",
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a7ccde6d7d..6530b0f1ce 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2916,7 +2916,7 @@ psql_completion(const char *text, int start, int end)
/* 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_WITH("silent", "default", "verbose");
/* Complete COPY <sth> FROM <sth> WITH (<options>) */
else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny, "WITH", MatchAny))
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 141fd48dc1..6f64d97fdd 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -45,7 +45,9 @@ typedef enum CopyOnErrorChoice
*/
typedef enum CopyLogVerbosityChoice
{
- COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages, default */
+ COPY_LOG_VERBOSITY_SILENT = -1, /* logs none */
+ COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages. As this is
+ * the default, assign 0 */
COPY_LOG_VERBOSITY_VERBOSE, /* logs additional messages */
} CopyLogVerbosityChoice;
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 61a19cdc4c..4e752977b5 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -760,6 +760,7 @@ COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity verbose);
NOTICE: skipping row due to data type incompatibility at line 2 for column "l": null input
CONTEXT: COPY check_ign_err2
NOTICE: 1 row was skipped due to data type incompatibility
+COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity silent);
-- reset context choice
\set SHOW_CONTEXT errors
SELECT * FROM check_ign_err;
@@ -774,7 +775,8 @@ SELECT * FROM check_ign_err2;
n | m | k | l
---+-----+---+-------
1 | {1} | 1 | 'foo'
-(1 row)
+ 3 | {3} | 3 | 'bar'
+(2 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 8b14962194..fa6aa17344 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -533,6 +533,10 @@ COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity verbose);
1 {1} 1 'foo'
2 {2} 2 \N
\.
+COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity silent);
+3 {3} 3 'bar'
+4 {4} 4 \N
+\.
-- reset context choice
\set SHOW_CONTEXT errors
--
2.45.2
From 1fb0d7a1d4af8961edb56e8d05400a118e9a584c Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Mon, 30 Sep 2024 23:05:26 +0900
Subject: [PATCH v6 2/3] file_fdw: Add on_error and log_verbosity options to
file_fdw.
In v17, the on_error and log_verbosity options were introduced for
the COPY command. This commit extends support for these options
to file_fdw.
Setting on_error = 'ignore' for a file_fdw foreign table allows users
to query it without errors, even when the input file contains
malformed rows, by skipping the problematic rows.
Both on_error and log_verbosity options apply to SELECT and ANALYZE
operations on file_fdw foreign tables.
Author: Atsushi Torikoshi
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
contrib/file_fdw/expected/file_fdw.out | 19 +++++++
contrib/file_fdw/file_fdw.c | 72 +++++++++++++++++++++++---
contrib/file_fdw/sql/file_fdw.sql | 7 +++
doc/src/sgml/file-fdw.sgml | 23 ++++++++
4 files changed, 113 insertions(+), 8 deletions(-)
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 86c148a86b..593fdc782e 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -206,6 +206,25 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
SELECT * FROM agg_bad; -- ERROR
ERROR: invalid input syntax for type real: "aaa"
CONTEXT: COPY agg_bad, line 3, column b: "aaa"
+-- on_error and log_verbosity tests
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
+SELECT * FROM agg_bad;
+NOTICE: 1 row was skipped due to data type incompatibility
+ a | b
+-----+--------
+ 100 | 99.097
+ 42 | 324.78
+(2 rows)
+
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD log_verbosity 'silent');
+SELECT * FROM agg_bad;
+ a | b
+-----+--------
+ 100 | 99.097
+ 42 | 324.78
+(2 rows)
+
+ANALYZE agg_bad;
-- misc query tests
\t on
SELECT explain_filter('EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv');
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index d16821f8e1..1e28c20797 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -22,6 +22,7 @@
#include "catalog/pg_authid.h"
#include "catalog/pg_foreign_table.h"
#include "commands/copy.h"
+#include "commands/copyfrom_internal.h"
#include "commands/defrem.h"
#include "commands/explain.h"
#include "commands/vacuum.h"
@@ -74,6 +75,8 @@ static const struct FileFdwOption valid_options[] = {
{"null", ForeignTableRelationId},
{"default", ForeignTableRelationId},
{"encoding", ForeignTableRelationId},
+ {"on_error", ForeignTableRelationId},
+ {"log_verbosity", ForeignTableRelationId},
{"force_not_null", AttributeRelationId},
{"force_null", AttributeRelationId},
@@ -725,12 +728,12 @@ fileIterateForeignScan(ForeignScanState *node)
ExprContext *econtext;
MemoryContext oldcontext;
TupleTableSlot *slot = node->ss.ss_ScanTupleSlot;
- bool found;
+ CopyFromState cstate = festate->cstate;
ErrorContextCallback errcallback;
/* Set up callback to identify error line number. */
errcallback.callback = CopyFromErrorCallback;
- errcallback.arg = (void *) festate->cstate;
+ errcallback.arg = (void *) cstate;
errcallback.previous = error_context_stack;
error_context_stack = &errcallback;
@@ -751,10 +754,27 @@ fileIterateForeignScan(ForeignScanState *node)
* switch in case we are doing that.
*/
oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
- found = NextCopyFrom(festate->cstate, econtext,
- slot->tts_values, slot->tts_isnull);
- if (found)
+
+retry:
+ if (NextCopyFrom(cstate, econtext, slot->tts_values, slot->tts_isnull))
+ {
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->escontext->error_occurred)
+ {
+ /*
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
+ */
+ cstate->escontext->error_occurred = false;
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ goto retry;
+ }
+
ExecStoreVirtualTuple(slot);
+ }
/* Switch back to original memory context */
MemoryContextSwitchTo(oldcontext);
@@ -796,8 +816,19 @@ fileEndForeignScan(ForeignScanState *node)
FileFdwExecutionState *festate = (FileFdwExecutionState *) node->fdw_state;
/* if festate is NULL, we are in EXPLAIN; nothing to do */
- if (festate)
- EndCopyFrom(festate->cstate);
+ if (!festate)
+ return;
+
+ if (festate->cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ festate->cstate->num_errors > 0 &&
+ festate->cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
+ ereport(NOTICE,
+ errmsg_plural("%llu row was skipped due to data type incompatibility",
+ "%llu rows were skipped due to data type incompatibility",
+ (unsigned long long) festate->cstate->num_errors,
+ (unsigned long long) festate->cstate->num_errors));
+
+ EndCopyFrom(festate->cstate);
}
/*
@@ -1113,7 +1144,8 @@ estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
* which must have at least targrows entries.
* The actual number of rows selected is returned as the function result.
* We also count the total number of rows in the file and return it into
- * *totalrows. Note that *totaldeadrows is always set to 0.
+ * *totalrows. Rows skipped due to on_error = 'ignore' are not included
+ * in this count. Note that *totaldeadrows is always set to 0.
*
* Note that the returned list of rows is not always in order by physical
* position in the file. Therefore, correlation estimates derived later
@@ -1191,6 +1223,21 @@ file_acquire_sample_rows(Relation onerel, int elevel,
if (!found)
break;
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->escontext->error_occurred)
+ {
+ /*
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
+ */
+ cstate->escontext->error_occurred = false;
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ continue;
+ }
+
/*
* The first targrows sample rows are simply copied into the
* reservoir. Then we start replacing tuples in the sample until we
@@ -1236,6 +1283,15 @@ file_acquire_sample_rows(Relation onerel, int elevel,
/* Clean up. */
MemoryContextDelete(tupcontext);
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->num_errors > 0 &&
+ cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
+ ereport(NOTICE,
+ errmsg_plural("%llu row was skipped due to data type incompatibility",
+ "%llu rows were skipped due to data type incompatibility",
+ (unsigned long long) cstate->num_errors,
+ (unsigned long long) cstate->num_errors));
+
EndCopyFrom(cstate);
pfree(values);
diff --git a/contrib/file_fdw/sql/file_fdw.sql b/contrib/file_fdw/sql/file_fdw.sql
index f0548e14e1..edd77c5cd2 100644
--- a/contrib/file_fdw/sql/file_fdw.sql
+++ b/contrib/file_fdw/sql/file_fdw.sql
@@ -150,6 +150,13 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
-- error context report tests
SELECT * FROM agg_bad; -- ERROR
+-- on_error and log_verbosity tests
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
+SELECT * FROM agg_bad;
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD log_verbosity 'silent');
+SELECT * FROM agg_bad;
+ANALYZE agg_bad;
+
-- misc query tests
\t on
SELECT explain_filter('EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv');
diff --git a/doc/src/sgml/file-fdw.sgml b/doc/src/sgml/file-fdw.sgml
index f2f2af9a59..bb3579b077 100644
--- a/doc/src/sgml/file-fdw.sgml
+++ b/doc/src/sgml/file-fdw.sgml
@@ -126,6 +126,29 @@
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>on_error</literal></term>
+
+ <listitem>
+ <para>
+ Specifies how to behave when encountering an error converting a column's
+ input value into its data type,
+ the same as <command>COPY</command>'s <literal>ON_ERROR</literal> option.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>log_verbosity</literal></term>
+
+ <listitem>
+ <para>
+ Specifies the amount of messages emitted by <literal>file_fdw</literal>,
+ the same as <command>COPY</command>'s <literal>LOG_VERBOSITY</literal> option.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
<para>
--
2.45.2
From 68663c230bbfc54e8bd730258e3a1a420eb0a92e Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email protected]>
Date: Wed, 25 Sep 2024 21:30:26 +0900
Subject: [PATCH v6 3/3] Refactor CopyFrom() in copyfrom.c.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This commit simplifies CopyFrom() by removing the unnecessary local variable
'skipped', which tracked the number of rows skipped due to on_error = 'ignore'.
That count is already handled by cstate->num_errors, so the 'skipped' variable
was redundant.
Additionally, the condition on_error != COPY_ON_ERROR_STOP is removed.
Since on_error == COPY_ON_ERROR_IGNORE is already checked, and on_error
only has two values (ignore and stop), the additional check was redundant
and made the logic harder to read. Seemingly this was introduced
in preparation for a future patch, but the current checks don’t offer
clear value and have been removed to improve readability.
Author: Atsushi Torikoshi
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
src/backend/commands/copyfrom.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 47879994f7..9139a40785 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -657,7 +657,6 @@ CopyFrom(CopyFromState cstate)
CopyMultiInsertInfo multiInsertInfo = {0}; /* pacify compiler */
int64 processed = 0;
int64 excluded = 0;
- int64 skipped = 0;
bool has_before_insert_row_trig;
bool has_instead_insert_row_trig;
bool leafpart_use_multi_insert = false;
@@ -1004,26 +1003,22 @@ CopyFrom(CopyFromState cstate)
if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
break;
- if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
cstate->escontext->error_occurred)
{
/*
- * Soft error occurred, skip this tuple and deal with error
- * information according to ON_ERROR.
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
*/
- if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)
-
- /*
- * Just make ErrorSaveContext ready for the next NextCopyFrom.
- * Since we don't set details_wanted and error_data is not to
- * be filled, just resetting error_occurred is enough.
- */
- cstate->escontext->error_occurred = false;
+ cstate->escontext->error_occurred = false;
/* Report that this tuple was skipped by the ON_ERROR clause */
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
- ++skipped);
+ cstate->num_errors);
+ /* Repeat NextCopyFrom() until no soft error occurs */
continue;
}
--
2.45.2
Attachments:
[text/plain] v6-0001-Add-log_verbosity-silent-support-to-COPY-command.patch (6.7K, ../../[email protected]/2-v6-0001-Add-log_verbosity-silent-support-to-COPY-command.patch)
download | inline diff:
From 6bcf56dc0556b3e9ded7200229c05c69e9c4fd6a Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email protected]>
Date: Wed, 25 Sep 2024 21:28:15 +0900
Subject: [PATCH v6 1/3] Add log_verbosity = 'silent' support to COPY command.
Previously, when the on_error option was set to ignore, the COPY command
would always log NOTICE messages for input rows discarded due to
data type incompatibility. Users had no way to suppress these messages.
This commit introduces a new log_verbosity setting, 'silent',
which prevents the COPY command from emitting NOTICE messages
when on_error = 'ignore' is used, even if rows are discarded.
This feature is particularly useful when processing malformed files
frequently, where a flood of NOTICE messages can be undesirable.
For example, when frequently loading malformed files via the COPY command
or querying foreign tables using file_fdw (with an upcoming patch to
add on_error support for file_fdw), users may prefer to suppress
these messages to reduce log noise and improve clarity.
Author: Atsushi Torikoshi
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
doc/src/sgml/ref/copy.sgml | 10 +++++++---
src/backend/commands/copy.c | 4 +++-
src/backend/commands/copyfrom.c | 3 ++-
src/bin/psql/tab-complete.c | 2 +-
src/include/commands/copy.h | 4 +++-
src/test/regress/expected/copy2.out | 4 +++-
src/test/regress/sql/copy2.sql | 4 ++++
7 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 1518af8a04..d87684a5be 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -407,6 +407,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<literal>verbose</literal>, a <literal>NOTICE</literal> message
containing the line of the input file and the column name whose input
conversion has failed is emitted for each discarded row.
+ When it is set to <literal>silent</literal>, no message is emitted
+ regarding ignored rows.
</para>
</listitem>
</varlistentry>
@@ -428,9 +430,11 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<listitem>
<para>
Specify the amount of messages emitted by a <command>COPY</command>
- command: <literal>default</literal> or <literal>verbose</literal>. If
- <literal>verbose</literal> is specified, additional messages are emitted
- during processing.
+ command: <literal>default</literal>, <literal>verbose</literal>, or
+ <literal>silent</literal>.
+ If <literal>verbose</literal> is specified, additional messages are
+ emitted during processing.
+ <literal>silent</literal> suppresses both verbose and default messages.
</para>
<para>
This is currently used in <command>COPY FROM</command> command when
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3bb579a3a4..03eb7a4eba 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -427,9 +427,11 @@ defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate)
char *sval;
/*
- * Allow "default", or "verbose" values.
+ * Allow "silent", "default", or "verbose" values.
*/
sval = defGetString(def);
+ if (pg_strcasecmp(sval, "silent") == 0)
+ return COPY_LOG_VERBOSITY_SILENT;
if (pg_strcasecmp(sval, "default") == 0)
return COPY_LOG_VERBOSITY_DEFAULT;
if (pg_strcasecmp(sval, "verbose") == 0)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 2d3462913e..47879994f7 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1320,7 +1320,8 @@ CopyFrom(CopyFromState cstate)
error_context_stack = errcallback.previous;
if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
- cstate->num_errors > 0)
+ cstate->num_errors > 0 &&
+ cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
ereport(NOTICE,
errmsg_plural("%llu row was skipped due to data type incompatibility",
"%llu rows were skipped due to data type incompatibility",
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a7ccde6d7d..6530b0f1ce 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2916,7 +2916,7 @@ psql_completion(const char *text, int start, int end)
/* 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_WITH("silent", "default", "verbose");
/* Complete COPY <sth> FROM <sth> WITH (<options>) */
else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny, "WITH", MatchAny))
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 141fd48dc1..6f64d97fdd 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -45,7 +45,9 @@ typedef enum CopyOnErrorChoice
*/
typedef enum CopyLogVerbosityChoice
{
- COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages, default */
+ COPY_LOG_VERBOSITY_SILENT = -1, /* logs none */
+ COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages. As this is
+ * the default, assign 0 */
COPY_LOG_VERBOSITY_VERBOSE, /* logs additional messages */
} CopyLogVerbosityChoice;
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 61a19cdc4c..4e752977b5 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -760,6 +760,7 @@ COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity verbose);
NOTICE: skipping row due to data type incompatibility at line 2 for column "l": null input
CONTEXT: COPY check_ign_err2
NOTICE: 1 row was skipped due to data type incompatibility
+COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity silent);
-- reset context choice
\set SHOW_CONTEXT errors
SELECT * FROM check_ign_err;
@@ -774,7 +775,8 @@ SELECT * FROM check_ign_err2;
n | m | k | l
---+-----+---+-------
1 | {1} | 1 | 'foo'
-(1 row)
+ 3 | {3} | 3 | 'bar'
+(2 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 8b14962194..fa6aa17344 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -533,6 +533,10 @@ COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity verbose);
1 {1} 1 'foo'
2 {2} 2 \N
\.
+COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity silent);
+3 {3} 3 'bar'
+4 {4} 4 \N
+\.
-- reset context choice
\set SHOW_CONTEXT errors
--
2.45.2
[text/plain] v6-0002-file_fdw-Add-on_error-and-log_verbosity-options-t.patch (8.6K, ../../[email protected]/3-v6-0002-file_fdw-Add-on_error-and-log_verbosity-options-t.patch)
download | inline diff:
From 1fb0d7a1d4af8961edb56e8d05400a118e9a584c Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Mon, 30 Sep 2024 23:05:26 +0900
Subject: [PATCH v6 2/3] file_fdw: Add on_error and log_verbosity options to
file_fdw.
In v17, the on_error and log_verbosity options were introduced for
the COPY command. This commit extends support for these options
to file_fdw.
Setting on_error = 'ignore' for a file_fdw foreign table allows users
to query it without errors, even when the input file contains
malformed rows, by skipping the problematic rows.
Both on_error and log_verbosity options apply to SELECT and ANALYZE
operations on file_fdw foreign tables.
Author: Atsushi Torikoshi
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
contrib/file_fdw/expected/file_fdw.out | 19 +++++++
contrib/file_fdw/file_fdw.c | 72 +++++++++++++++++++++++---
contrib/file_fdw/sql/file_fdw.sql | 7 +++
doc/src/sgml/file-fdw.sgml | 23 ++++++++
4 files changed, 113 insertions(+), 8 deletions(-)
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 86c148a86b..593fdc782e 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -206,6 +206,25 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
SELECT * FROM agg_bad; -- ERROR
ERROR: invalid input syntax for type real: "aaa"
CONTEXT: COPY agg_bad, line 3, column b: "aaa"
+-- on_error and log_verbosity tests
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
+SELECT * FROM agg_bad;
+NOTICE: 1 row was skipped due to data type incompatibility
+ a | b
+-----+--------
+ 100 | 99.097
+ 42 | 324.78
+(2 rows)
+
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD log_verbosity 'silent');
+SELECT * FROM agg_bad;
+ a | b
+-----+--------
+ 100 | 99.097
+ 42 | 324.78
+(2 rows)
+
+ANALYZE agg_bad;
-- misc query tests
\t on
SELECT explain_filter('EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv');
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index d16821f8e1..1e28c20797 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -22,6 +22,7 @@
#include "catalog/pg_authid.h"
#include "catalog/pg_foreign_table.h"
#include "commands/copy.h"
+#include "commands/copyfrom_internal.h"
#include "commands/defrem.h"
#include "commands/explain.h"
#include "commands/vacuum.h"
@@ -74,6 +75,8 @@ static const struct FileFdwOption valid_options[] = {
{"null", ForeignTableRelationId},
{"default", ForeignTableRelationId},
{"encoding", ForeignTableRelationId},
+ {"on_error", ForeignTableRelationId},
+ {"log_verbosity", ForeignTableRelationId},
{"force_not_null", AttributeRelationId},
{"force_null", AttributeRelationId},
@@ -725,12 +728,12 @@ fileIterateForeignScan(ForeignScanState *node)
ExprContext *econtext;
MemoryContext oldcontext;
TupleTableSlot *slot = node->ss.ss_ScanTupleSlot;
- bool found;
+ CopyFromState cstate = festate->cstate;
ErrorContextCallback errcallback;
/* Set up callback to identify error line number. */
errcallback.callback = CopyFromErrorCallback;
- errcallback.arg = (void *) festate->cstate;
+ errcallback.arg = (void *) cstate;
errcallback.previous = error_context_stack;
error_context_stack = &errcallback;
@@ -751,10 +754,27 @@ fileIterateForeignScan(ForeignScanState *node)
* switch in case we are doing that.
*/
oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
- found = NextCopyFrom(festate->cstate, econtext,
- slot->tts_values, slot->tts_isnull);
- if (found)
+
+retry:
+ if (NextCopyFrom(cstate, econtext, slot->tts_values, slot->tts_isnull))
+ {
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->escontext->error_occurred)
+ {
+ /*
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
+ */
+ cstate->escontext->error_occurred = false;
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ goto retry;
+ }
+
ExecStoreVirtualTuple(slot);
+ }
/* Switch back to original memory context */
MemoryContextSwitchTo(oldcontext);
@@ -796,8 +816,19 @@ fileEndForeignScan(ForeignScanState *node)
FileFdwExecutionState *festate = (FileFdwExecutionState *) node->fdw_state;
/* if festate is NULL, we are in EXPLAIN; nothing to do */
- if (festate)
- EndCopyFrom(festate->cstate);
+ if (!festate)
+ return;
+
+ if (festate->cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ festate->cstate->num_errors > 0 &&
+ festate->cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
+ ereport(NOTICE,
+ errmsg_plural("%llu row was skipped due to data type incompatibility",
+ "%llu rows were skipped due to data type incompatibility",
+ (unsigned long long) festate->cstate->num_errors,
+ (unsigned long long) festate->cstate->num_errors));
+
+ EndCopyFrom(festate->cstate);
}
/*
@@ -1113,7 +1144,8 @@ estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
* which must have at least targrows entries.
* The actual number of rows selected is returned as the function result.
* We also count the total number of rows in the file and return it into
- * *totalrows. Note that *totaldeadrows is always set to 0.
+ * *totalrows. Rows skipped due to on_error = 'ignore' are not included
+ * in this count. Note that *totaldeadrows is always set to 0.
*
* Note that the returned list of rows is not always in order by physical
* position in the file. Therefore, correlation estimates derived later
@@ -1191,6 +1223,21 @@ file_acquire_sample_rows(Relation onerel, int elevel,
if (!found)
break;
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->escontext->error_occurred)
+ {
+ /*
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
+ */
+ cstate->escontext->error_occurred = false;
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ continue;
+ }
+
/*
* The first targrows sample rows are simply copied into the
* reservoir. Then we start replacing tuples in the sample until we
@@ -1236,6 +1283,15 @@ file_acquire_sample_rows(Relation onerel, int elevel,
/* Clean up. */
MemoryContextDelete(tupcontext);
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->num_errors > 0 &&
+ cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
+ ereport(NOTICE,
+ errmsg_plural("%llu row was skipped due to data type incompatibility",
+ "%llu rows were skipped due to data type incompatibility",
+ (unsigned long long) cstate->num_errors,
+ (unsigned long long) cstate->num_errors));
+
EndCopyFrom(cstate);
pfree(values);
diff --git a/contrib/file_fdw/sql/file_fdw.sql b/contrib/file_fdw/sql/file_fdw.sql
index f0548e14e1..edd77c5cd2 100644
--- a/contrib/file_fdw/sql/file_fdw.sql
+++ b/contrib/file_fdw/sql/file_fdw.sql
@@ -150,6 +150,13 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
-- error context report tests
SELECT * FROM agg_bad; -- ERROR
+-- on_error and log_verbosity tests
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
+SELECT * FROM agg_bad;
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD log_verbosity 'silent');
+SELECT * FROM agg_bad;
+ANALYZE agg_bad;
+
-- misc query tests
\t on
SELECT explain_filter('EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv');
diff --git a/doc/src/sgml/file-fdw.sgml b/doc/src/sgml/file-fdw.sgml
index f2f2af9a59..bb3579b077 100644
--- a/doc/src/sgml/file-fdw.sgml
+++ b/doc/src/sgml/file-fdw.sgml
@@ -126,6 +126,29 @@
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>on_error</literal></term>
+
+ <listitem>
+ <para>
+ Specifies how to behave when encountering an error converting a column's
+ input value into its data type,
+ the same as <command>COPY</command>'s <literal>ON_ERROR</literal> option.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>log_verbosity</literal></term>
+
+ <listitem>
+ <para>
+ Specifies the amount of messages emitted by <literal>file_fdw</literal>,
+ the same as <command>COPY</command>'s <literal>LOG_VERBOSITY</literal> option.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
<para>
--
2.45.2
[text/plain] v6-0003-Refactor-CopyFrom-in-copyfrom.c.patch (3.0K, ../../[email protected]/4-v6-0003-Refactor-CopyFrom-in-copyfrom.c.patch)
download | inline diff:
From 68663c230bbfc54e8bd730258e3a1a420eb0a92e Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email protected]>
Date: Wed, 25 Sep 2024 21:30:26 +0900
Subject: [PATCH v6 3/3] Refactor CopyFrom() in copyfrom.c.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This commit simplifies CopyFrom() by removing the unnecessary local variable
'skipped', which tracked the number of rows skipped due to on_error = 'ignore'.
That count is already handled by cstate->num_errors, so the 'skipped' variable
was redundant.
Additionally, the condition on_error != COPY_ON_ERROR_STOP is removed.
Since on_error == COPY_ON_ERROR_IGNORE is already checked, and on_error
only has two values (ignore and stop), the additional check was redundant
and made the logic harder to read. Seemingly this was introduced
in preparation for a future patch, but the current checks don’t offer
clear value and have been removed to improve readability.
Author: Atsushi Torikoshi
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
src/backend/commands/copyfrom.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 47879994f7..9139a40785 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -657,7 +657,6 @@ CopyFrom(CopyFromState cstate)
CopyMultiInsertInfo multiInsertInfo = {0}; /* pacify compiler */
int64 processed = 0;
int64 excluded = 0;
- int64 skipped = 0;
bool has_before_insert_row_trig;
bool has_instead_insert_row_trig;
bool leafpart_use_multi_insert = false;
@@ -1004,26 +1003,22 @@ CopyFrom(CopyFromState cstate)
if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
break;
- if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
cstate->escontext->error_occurred)
{
/*
- * Soft error occurred, skip this tuple and deal with error
- * information according to ON_ERROR.
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
*/
- if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)
-
- /*
- * Just make ErrorSaveContext ready for the next NextCopyFrom.
- * Since we don't set details_wanted and error_data is not to
- * be filled, just resetting error_occurred is enough.
- */
- cstate->escontext->error_occurred = false;
+ cstate->escontext->error_occurred = false;
/* Report that this tuple was skipped by the ON_ERROR clause */
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
- ++skipped);
+ cstate->num_errors);
+ /* Repeat NextCopyFrom() until no soft error occurs */
continue;
}
--
2.45.2
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add on_error and log_verbosity options to file_fdw
2024-09-30 15:36 Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
@ 2024-10-02 00:27 ` Masahiko Sawada <[email protected]>
2024-10-02 06:34 ` Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Masahiko Sawada @ 2024-10-02 00:27 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: torikoshia <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers
Hi,
On Mon, Sep 30, 2024 at 8:36 AM Fujii Masao <[email protected]> wrote:
>
>
>
> On 2024/09/26 21:57, torikoshia wrote:
> > Updated the patches.
>
> Thanks for updating the patches! I’ve made some changes based on your work, which are attached.
> Barring any objections, I'm thinking to push these patches.
>
> For patches 0001 and 0003, I ran pgindent and updated the commit message.
>
> Regarding patch 0002:
>
> - I updated the regression test to run ANALYZE on the file_fdw foreign table
> since the on_error option also affects the ANALYZE command. To ensure test
> stability, the test now runs ANALYZE with log_verbosity = 'silent'.
>
> - I removed the code that updated the count of skipped rows for
> the pg_stat_progress_copy view. As far as I know, file_fdw doesn’t
> currently support tracking pg_stat_progress_copy.tuples_processed.
> Supporting only tuples_skipped seems inconsistent, so I suggest creating
> a separate patch to extend file_fdw to track both tuples_processed and
> tuples_skipped in this view.
>
> - I refactored the for-loop handling on_error = 'ignore' in fileIterateForeignScan()
> by replacing it with a goto statement for improved readability.
>
> - I modified file_fdw to log a NOTICE message about skipped rows at the end of
> ANALYZE if any rows are skipped due to the on_error = 'ignore' setting.
>
> Regarding the "file contains XXX rows" message reported by the ANALYZE VERBOSE
> command on the file_fdw foreign table, what number should be reflected in XXX,
> especially when some rows are skipped due to on_error = 'ignore'?
> Currently, the count only includes valid rows, excluding any skipped rows.
> I haven't modified this code yet. Should we instead count all rows
> (valid and erroneous) and report that total?
>
> I noticed the code for reporting the number of skipped rows due to
> on_error = 'ignore' appears in three places. I’m considering creating
> a common function for this reporting to eliminate redundancy but haven’t
> implemented it yet.
>
> - I’ve updated the commit message and run pgindent.
Sorry for being late in joining the review of this patch. Both 0001
and 0003 look good to me. I have two comments on the 0002 patch:
- found = NextCopyFrom(festate->cstate, econtext,
- slot->tts_values,
slot->tts_isnull);
- if (found)
+
+retry:
+ if (NextCopyFrom(cstate, econtext, slot->tts_values, slot->tts_isnull))
+ {
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->escontext->error_occurred)
+ {
+ /*
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next
NextCopyFrom. Since we
+ * don't set details_wanted and error_data is
not to be filled,
+ * just resetting error_occurred is enough.
+ */
+ cstate->escontext->error_occurred = false;
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ goto retry;
+ }
+
ExecStoreVirtualTuple(slot);
+ }
I think that while scanning a file_fdw foreign table with
log_verbosity='silent' the query is not interruptible.
Also, we don't switch to the per-tuple memory context when retrying
due to a soft error. I'm not sure it's okay as in CopyFrom(), a
similar function for COPY command, we switch to the per-tuple memory
context every time before parsing an input line. Would it be
problematic if we switch to another memory context while parsing an
input line? In CopyFrom() we also call ResetPerTupleExprContext() and
ExecClearTuple() for every input, so we might want to consider calling
them for every input.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add on_error and log_verbosity options to file_fdw
2024-09-30 15:36 Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
2024-10-02 00:27 ` Re: Add on_error and log_verbosity options to file_fdw Masahiko Sawada <[email protected]>
@ 2024-10-02 06:34 ` Fujii Masao <[email protected]>
2024-10-03 04:23 ` Re: Add on_error and log_verbosity options to file_fdw Masahiko Sawada <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Fujii Masao @ 2024-10-02 06:34 UTC (permalink / raw)
To: Masahiko Sawada <[email protected]>; +Cc: torikoshia <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers
On 2024/10/02 9:27, Masahiko Sawada wrote:
> Sorry for being late in joining the review of this patch. Both 0001
> and 0003 look good to me. I have two comments on the 0002 patch:
Thanks for the review!
> I think that while scanning a file_fdw foreign table with
> log_verbosity='silent' the query is not interruptible.
You're right. I added CHECK_FOR_INTERRUPTS() in the retry loop.
> Also, we don't switch to the per-tuple memory context when retrying
> due to a soft error. I'm not sure it's okay as in CopyFrom(), a
> similar function for COPY command, we switch to the per-tuple memory
> context every time before parsing an input line. Would it be
> problematic if we switch to another memory context while parsing an
> input line? In CopyFrom() we also call ResetPerTupleExprContext() and
> ExecClearTuple() for every input, so we might want to consider calling
> them for every input.
Yes, I've updated the patch based on your comment.
Could you please review the latest version?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From 2c455d62aad84267e987b07a1287fd979abc0995 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email protected]>
Date: Wed, 25 Sep 2024 21:28:15 +0900
Subject: [PATCH v7 1/3] Add log_verbosity = 'silent' support to COPY command.
Previously, when the on_error option was set to ignore, the COPY command
would always log NOTICE messages for input rows discarded due to
data type incompatibility. Users had no way to suppress these messages.
This commit introduces a new log_verbosity setting, 'silent',
which prevents the COPY command from emitting NOTICE messages
when on_error = 'ignore' is used, even if rows are discarded.
This feature is particularly useful when processing malformed files
frequently, where a flood of NOTICE messages can be undesirable.
For example, when frequently loading malformed files via the COPY command
or querying foreign tables using file_fdw (with an upcoming patch to
add on_error support for file_fdw), users may prefer to suppress
these messages to reduce log noise and improve clarity.
Author: Atsushi Torikoshi
Reviewed-by: Masahiko Sawada, Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
doc/src/sgml/ref/copy.sgml | 10 +++++++---
src/backend/commands/copy.c | 4 +++-
src/backend/commands/copyfrom.c | 3 ++-
src/bin/psql/tab-complete.c | 2 +-
src/include/commands/copy.h | 4 +++-
src/test/regress/expected/copy2.out | 4 +++-
src/test/regress/sql/copy2.sql | 4 ++++
7 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index fdbd20bc50..58a14bc427 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -407,6 +407,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<literal>verbose</literal>, a <literal>NOTICE</literal> message
containing the line of the input file and the column name whose input
conversion has failed is emitted for each discarded row.
+ When it is set to <literal>silent</literal>, no message is emitted
+ regarding ignored rows.
</para>
</listitem>
</varlistentry>
@@ -428,9 +430,11 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<listitem>
<para>
Specify the amount of messages emitted by a <command>COPY</command>
- command: <literal>default</literal> or <literal>verbose</literal>. If
- <literal>verbose</literal> is specified, additional messages are emitted
- during processing.
+ command: <literal>default</literal>, <literal>verbose</literal>, or
+ <literal>silent</literal>.
+ If <literal>verbose</literal> is specified, additional messages are
+ emitted during processing.
+ <literal>silent</literal> suppresses both verbose and default messages.
</para>
<para>
This is currently used in <command>COPY FROM</command> command when
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3bb579a3a4..03eb7a4eba 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -427,9 +427,11 @@ defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate)
char *sval;
/*
- * Allow "default", or "verbose" values.
+ * Allow "silent", "default", or "verbose" values.
*/
sval = defGetString(def);
+ if (pg_strcasecmp(sval, "silent") == 0)
+ return COPY_LOG_VERBOSITY_SILENT;
if (pg_strcasecmp(sval, "default") == 0)
return COPY_LOG_VERBOSITY_DEFAULT;
if (pg_strcasecmp(sval, "verbose") == 0)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 2d3462913e..47879994f7 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1320,7 +1320,8 @@ CopyFrom(CopyFromState cstate)
error_context_stack = errcallback.previous;
if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
- cstate->num_errors > 0)
+ cstate->num_errors > 0 &&
+ cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
ereport(NOTICE,
errmsg_plural("%llu row was skipped due to data type incompatibility",
"%llu rows were skipped due to data type incompatibility",
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a7ccde6d7d..6530b0f1ce 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2916,7 +2916,7 @@ psql_completion(const char *text, int start, int end)
/* 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_WITH("silent", "default", "verbose");
/* Complete COPY <sth> FROM <sth> WITH (<options>) */
else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny, "WITH", MatchAny))
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 141fd48dc1..6f64d97fdd 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -45,7 +45,9 @@ typedef enum CopyOnErrorChoice
*/
typedef enum CopyLogVerbosityChoice
{
- COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages, default */
+ COPY_LOG_VERBOSITY_SILENT = -1, /* logs none */
+ COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages. As this is
+ * the default, assign 0 */
COPY_LOG_VERBOSITY_VERBOSE, /* logs additional messages */
} CopyLogVerbosityChoice;
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 61a19cdc4c..4e752977b5 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -760,6 +760,7 @@ COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity verbose);
NOTICE: skipping row due to data type incompatibility at line 2 for column "l": null input
CONTEXT: COPY check_ign_err2
NOTICE: 1 row was skipped due to data type incompatibility
+COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity silent);
-- reset context choice
\set SHOW_CONTEXT errors
SELECT * FROM check_ign_err;
@@ -774,7 +775,8 @@ SELECT * FROM check_ign_err2;
n | m | k | l
---+-----+---+-------
1 | {1} | 1 | 'foo'
-(1 row)
+ 3 | {3} | 3 | 'bar'
+(2 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 8b14962194..fa6aa17344 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -533,6 +533,10 @@ COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity verbose);
1 {1} 1 'foo'
2 {2} 2 \N
\.
+COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity silent);
+3 {3} 3 'bar'
+4 {4} 4 \N
+\.
-- reset context choice
\set SHOW_CONTEXT errors
--
2.45.2
From f4458a36698997da23f813f858e6680c3e1daefa Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Mon, 30 Sep 2024 23:05:26 +0900
Subject: [PATCH v7 2/3] file_fdw: Add on_error and log_verbosity options to
file_fdw.
In v17, the on_error and log_verbosity options were introduced for
the COPY command. This commit extends support for these options
to file_fdw.
Setting on_error = 'ignore' for a file_fdw foreign table allows users
to query it without errors, even when the input file contains
malformed rows, by skipping the problematic rows.
Both on_error and log_verbosity options apply to SELECT and ANALYZE
operations on file_fdw foreign tables.
Author: Atsushi Torikoshi
Reviewed-by: Masahiko Sawada, Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
contrib/file_fdw/expected/file_fdw.out | 19 +++++
contrib/file_fdw/file_fdw.c | 107 +++++++++++++++++++++----
contrib/file_fdw/sql/file_fdw.sql | 7 ++
doc/src/sgml/file-fdw.sgml | 23 ++++++
4 files changed, 140 insertions(+), 16 deletions(-)
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 86c148a86b..593fdc782e 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -206,6 +206,25 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
SELECT * FROM agg_bad; -- ERROR
ERROR: invalid input syntax for type real: "aaa"
CONTEXT: COPY agg_bad, line 3, column b: "aaa"
+-- on_error and log_verbosity tests
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
+SELECT * FROM agg_bad;
+NOTICE: 1 row was skipped due to data type incompatibility
+ a | b
+-----+--------
+ 100 | 99.097
+ 42 | 324.78
+(2 rows)
+
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD log_verbosity 'silent');
+SELECT * FROM agg_bad;
+ a | b
+-----+--------
+ 100 | 99.097
+ 42 | 324.78
+(2 rows)
+
+ANALYZE agg_bad;
-- misc query tests
\t on
SELECT explain_filter('EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv');
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index d16821f8e1..043204c3e7 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -22,6 +22,7 @@
#include "catalog/pg_authid.h"
#include "catalog/pg_foreign_table.h"
#include "commands/copy.h"
+#include "commands/copyfrom_internal.h"
#include "commands/defrem.h"
#include "commands/explain.h"
#include "commands/vacuum.h"
@@ -74,6 +75,8 @@ static const struct FileFdwOption valid_options[] = {
{"null", ForeignTableRelationId},
{"default", ForeignTableRelationId},
{"encoding", ForeignTableRelationId},
+ {"on_error", ForeignTableRelationId},
+ {"log_verbosity", ForeignTableRelationId},
{"force_not_null", AttributeRelationId},
{"force_null", AttributeRelationId},
@@ -723,38 +726,74 @@ fileIterateForeignScan(ForeignScanState *node)
FileFdwExecutionState *festate = (FileFdwExecutionState *) node->fdw_state;
EState *estate = CreateExecutorState();
ExprContext *econtext;
- MemoryContext oldcontext;
+ MemoryContext oldcontext = CurrentMemoryContext;
TupleTableSlot *slot = node->ss.ss_ScanTupleSlot;
- bool found;
+ CopyFromState cstate = festate->cstate;
ErrorContextCallback errcallback;
/* Set up callback to identify error line number. */
errcallback.callback = CopyFromErrorCallback;
- errcallback.arg = (void *) festate->cstate;
+ errcallback.arg = (void *) cstate;
errcallback.previous = error_context_stack;
error_context_stack = &errcallback;
/*
- * The protocol for loading a virtual tuple into a slot is first
- * ExecClearTuple, then fill the values/isnull arrays, then
- * ExecStoreVirtualTuple. If we don't find another row in the file, we
- * just skip the last step, leaving the slot empty as required.
- *
* We pass ExprContext because there might be a use of the DEFAULT option
* in COPY FROM, so we may need to evaluate default expressions.
*/
- ExecClearTuple(slot);
econtext = GetPerTupleExprContext(estate);
+retry:
+
/*
* DEFAULT expressions need to be evaluated in a per-tuple context, so
* switch in case we are doing that.
*/
- oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
- found = NextCopyFrom(festate->cstate, econtext,
- slot->tts_values, slot->tts_isnull);
- if (found)
+ MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
+ /*
+ * The protocol for loading a virtual tuple into a slot is first
+ * ExecClearTuple, then fill the values/isnull arrays, then
+ * ExecStoreVirtualTuple. If we don't find another row in the file, we
+ * just skip the last step, leaving the slot empty as required.
+ *
+ */
+ ExecClearTuple(slot);
+
+ if (NextCopyFrom(cstate, econtext, slot->tts_values, slot->tts_isnull))
+ {
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->escontext->error_occurred)
+ {
+ /*
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
+ */
+ cstate->escontext->error_occurred = false;
+
+ /* Switch back to original memory context */
+ MemoryContextSwitchTo(oldcontext);
+
+ /*
+ * Make sure we are interruptible while repeatedly calling
+ * NextCopyFrom() until no soft error occurs.
+ */
+ CHECK_FOR_INTERRUPTS();
+
+ /*
+ * Reset the per-tuple exprcontext, to clean-up after expression
+ * evaluations etc.
+ */
+ ResetPerTupleExprContext(estate);
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ goto retry;
+ }
+
ExecStoreVirtualTuple(slot);
+ }
/* Switch back to original memory context */
MemoryContextSwitchTo(oldcontext);
@@ -796,8 +835,19 @@ fileEndForeignScan(ForeignScanState *node)
FileFdwExecutionState *festate = (FileFdwExecutionState *) node->fdw_state;
/* if festate is NULL, we are in EXPLAIN; nothing to do */
- if (festate)
- EndCopyFrom(festate->cstate);
+ if (!festate)
+ return;
+
+ if (festate->cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ festate->cstate->num_errors > 0 &&
+ festate->cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
+ ereport(NOTICE,
+ errmsg_plural("%llu row was skipped due to data type incompatibility",
+ "%llu rows were skipped due to data type incompatibility",
+ (unsigned long long) festate->cstate->num_errors,
+ (unsigned long long) festate->cstate->num_errors));
+
+ EndCopyFrom(festate->cstate);
}
/*
@@ -1113,7 +1163,8 @@ estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
* which must have at least targrows entries.
* The actual number of rows selected is returned as the function result.
* We also count the total number of rows in the file and return it into
- * *totalrows. Note that *totaldeadrows is always set to 0.
+ * *totalrows. Rows skipped due to on_error = 'ignore' are not included
+ * in this count. Note that *totaldeadrows is always set to 0.
*
* Note that the returned list of rows is not always in order by physical
* position in the file. Therefore, correlation estimates derived later
@@ -1191,6 +1242,21 @@ file_acquire_sample_rows(Relation onerel, int elevel,
if (!found)
break;
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->escontext->error_occurred)
+ {
+ /*
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
+ */
+ cstate->escontext->error_occurred = false;
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ continue;
+ }
+
/*
* The first targrows sample rows are simply copied into the
* reservoir. Then we start replacing tuples in the sample until we
@@ -1236,6 +1302,15 @@ file_acquire_sample_rows(Relation onerel, int elevel,
/* Clean up. */
MemoryContextDelete(tupcontext);
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->num_errors > 0 &&
+ cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
+ ereport(NOTICE,
+ errmsg_plural("%llu row was skipped due to data type incompatibility",
+ "%llu rows were skipped due to data type incompatibility",
+ (unsigned long long) cstate->num_errors,
+ (unsigned long long) cstate->num_errors));
+
EndCopyFrom(cstate);
pfree(values);
diff --git a/contrib/file_fdw/sql/file_fdw.sql b/contrib/file_fdw/sql/file_fdw.sql
index f0548e14e1..edd77c5cd2 100644
--- a/contrib/file_fdw/sql/file_fdw.sql
+++ b/contrib/file_fdw/sql/file_fdw.sql
@@ -150,6 +150,13 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
-- error context report tests
SELECT * FROM agg_bad; -- ERROR
+-- on_error and log_verbosity tests
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
+SELECT * FROM agg_bad;
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD log_verbosity 'silent');
+SELECT * FROM agg_bad;
+ANALYZE agg_bad;
+
-- misc query tests
\t on
SELECT explain_filter('EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv');
diff --git a/doc/src/sgml/file-fdw.sgml b/doc/src/sgml/file-fdw.sgml
index f2f2af9a59..bb3579b077 100644
--- a/doc/src/sgml/file-fdw.sgml
+++ b/doc/src/sgml/file-fdw.sgml
@@ -126,6 +126,29 @@
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>on_error</literal></term>
+
+ <listitem>
+ <para>
+ Specifies how to behave when encountering an error converting a column's
+ input value into its data type,
+ the same as <command>COPY</command>'s <literal>ON_ERROR</literal> option.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>log_verbosity</literal></term>
+
+ <listitem>
+ <para>
+ Specifies the amount of messages emitted by <literal>file_fdw</literal>,
+ the same as <command>COPY</command>'s <literal>LOG_VERBOSITY</literal> option.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
<para>
--
2.45.2
From 6c132dc601d979ed8ffa4fadc0af9a8c73f88fce Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email protected]>
Date: Wed, 25 Sep 2024 21:30:26 +0900
Subject: [PATCH v7 3/3] Refactor CopyFrom() in copyfrom.c.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This commit simplifies CopyFrom() by removing the unnecessary local variable
'skipped', which tracked the number of rows skipped due to on_error = 'ignore'.
That count is already handled by cstate->num_errors, so the 'skipped' variable
was redundant.
Additionally, the condition on_error != COPY_ON_ERROR_STOP is removed.
Since on_error == COPY_ON_ERROR_IGNORE is already checked, and on_error
only has two values (ignore and stop), the additional check was redundant
and made the logic harder to read. Seemingly this was introduced
in preparation for a future patch, but the current checks don’t offer
clear value and have been removed to improve readability.
Author: Atsushi Torikoshi
Reviewed-by: Masahiko Sawada, Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
src/backend/commands/copyfrom.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 47879994f7..9139a40785 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -657,7 +657,6 @@ CopyFrom(CopyFromState cstate)
CopyMultiInsertInfo multiInsertInfo = {0}; /* pacify compiler */
int64 processed = 0;
int64 excluded = 0;
- int64 skipped = 0;
bool has_before_insert_row_trig;
bool has_instead_insert_row_trig;
bool leafpart_use_multi_insert = false;
@@ -1004,26 +1003,22 @@ CopyFrom(CopyFromState cstate)
if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
break;
- if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
cstate->escontext->error_occurred)
{
/*
- * Soft error occurred, skip this tuple and deal with error
- * information according to ON_ERROR.
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
*/
- if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)
-
- /*
- * Just make ErrorSaveContext ready for the next NextCopyFrom.
- * Since we don't set details_wanted and error_data is not to
- * be filled, just resetting error_occurred is enough.
- */
- cstate->escontext->error_occurred = false;
+ cstate->escontext->error_occurred = false;
/* Report that this tuple was skipped by the ON_ERROR clause */
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
- ++skipped);
+ cstate->num_errors);
+ /* Repeat NextCopyFrom() until no soft error occurs */
continue;
}
--
2.45.2
Attachments:
[text/plain] v7-0001-Add-log_verbosity-silent-support-to-COPY-command.patch (6.7K, ../../[email protected]/2-v7-0001-Add-log_verbosity-silent-support-to-COPY-command.patch)
download | inline diff:
From 2c455d62aad84267e987b07a1287fd979abc0995 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email protected]>
Date: Wed, 25 Sep 2024 21:28:15 +0900
Subject: [PATCH v7 1/3] Add log_verbosity = 'silent' support to COPY command.
Previously, when the on_error option was set to ignore, the COPY command
would always log NOTICE messages for input rows discarded due to
data type incompatibility. Users had no way to suppress these messages.
This commit introduces a new log_verbosity setting, 'silent',
which prevents the COPY command from emitting NOTICE messages
when on_error = 'ignore' is used, even if rows are discarded.
This feature is particularly useful when processing malformed files
frequently, where a flood of NOTICE messages can be undesirable.
For example, when frequently loading malformed files via the COPY command
or querying foreign tables using file_fdw (with an upcoming patch to
add on_error support for file_fdw), users may prefer to suppress
these messages to reduce log noise and improve clarity.
Author: Atsushi Torikoshi
Reviewed-by: Masahiko Sawada, Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
doc/src/sgml/ref/copy.sgml | 10 +++++++---
src/backend/commands/copy.c | 4 +++-
src/backend/commands/copyfrom.c | 3 ++-
src/bin/psql/tab-complete.c | 2 +-
src/include/commands/copy.h | 4 +++-
src/test/regress/expected/copy2.out | 4 +++-
src/test/regress/sql/copy2.sql | 4 ++++
7 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index fdbd20bc50..58a14bc427 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -407,6 +407,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<literal>verbose</literal>, a <literal>NOTICE</literal> message
containing the line of the input file and the column name whose input
conversion has failed is emitted for each discarded row.
+ When it is set to <literal>silent</literal>, no message is emitted
+ regarding ignored rows.
</para>
</listitem>
</varlistentry>
@@ -428,9 +430,11 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<listitem>
<para>
Specify the amount of messages emitted by a <command>COPY</command>
- command: <literal>default</literal> or <literal>verbose</literal>. If
- <literal>verbose</literal> is specified, additional messages are emitted
- during processing.
+ command: <literal>default</literal>, <literal>verbose</literal>, or
+ <literal>silent</literal>.
+ If <literal>verbose</literal> is specified, additional messages are
+ emitted during processing.
+ <literal>silent</literal> suppresses both verbose and default messages.
</para>
<para>
This is currently used in <command>COPY FROM</command> command when
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3bb579a3a4..03eb7a4eba 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -427,9 +427,11 @@ defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate)
char *sval;
/*
- * Allow "default", or "verbose" values.
+ * Allow "silent", "default", or "verbose" values.
*/
sval = defGetString(def);
+ if (pg_strcasecmp(sval, "silent") == 0)
+ return COPY_LOG_VERBOSITY_SILENT;
if (pg_strcasecmp(sval, "default") == 0)
return COPY_LOG_VERBOSITY_DEFAULT;
if (pg_strcasecmp(sval, "verbose") == 0)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 2d3462913e..47879994f7 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1320,7 +1320,8 @@ CopyFrom(CopyFromState cstate)
error_context_stack = errcallback.previous;
if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
- cstate->num_errors > 0)
+ cstate->num_errors > 0 &&
+ cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
ereport(NOTICE,
errmsg_plural("%llu row was skipped due to data type incompatibility",
"%llu rows were skipped due to data type incompatibility",
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a7ccde6d7d..6530b0f1ce 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2916,7 +2916,7 @@ psql_completion(const char *text, int start, int end)
/* 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_WITH("silent", "default", "verbose");
/* Complete COPY <sth> FROM <sth> WITH (<options>) */
else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny, "WITH", MatchAny))
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 141fd48dc1..6f64d97fdd 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -45,7 +45,9 @@ typedef enum CopyOnErrorChoice
*/
typedef enum CopyLogVerbosityChoice
{
- COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages, default */
+ COPY_LOG_VERBOSITY_SILENT = -1, /* logs none */
+ COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages. As this is
+ * the default, assign 0 */
COPY_LOG_VERBOSITY_VERBOSE, /* logs additional messages */
} CopyLogVerbosityChoice;
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 61a19cdc4c..4e752977b5 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -760,6 +760,7 @@ COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity verbose);
NOTICE: skipping row due to data type incompatibility at line 2 for column "l": null input
CONTEXT: COPY check_ign_err2
NOTICE: 1 row was skipped due to data type incompatibility
+COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity silent);
-- reset context choice
\set SHOW_CONTEXT errors
SELECT * FROM check_ign_err;
@@ -774,7 +775,8 @@ SELECT * FROM check_ign_err2;
n | m | k | l
---+-----+---+-------
1 | {1} | 1 | 'foo'
-(1 row)
+ 3 | {3} | 3 | 'bar'
+(2 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 8b14962194..fa6aa17344 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -533,6 +533,10 @@ COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity verbose);
1 {1} 1 'foo'
2 {2} 2 \N
\.
+COPY check_ign_err2 FROM STDIN WITH (on_error ignore, log_verbosity silent);
+3 {3} 3 'bar'
+4 {4} 4 \N
+\.
-- reset context choice
\set SHOW_CONTEXT errors
--
2.45.2
[text/plain] v7-0002-file_fdw-Add-on_error-and-log_verbosity-options-t.patch (10.0K, ../../[email protected]/3-v7-0002-file_fdw-Add-on_error-and-log_verbosity-options-t.patch)
download | inline diff:
From f4458a36698997da23f813f858e6680c3e1daefa Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
Date: Mon, 30 Sep 2024 23:05:26 +0900
Subject: [PATCH v7 2/3] file_fdw: Add on_error and log_verbosity options to
file_fdw.
In v17, the on_error and log_verbosity options were introduced for
the COPY command. This commit extends support for these options
to file_fdw.
Setting on_error = 'ignore' for a file_fdw foreign table allows users
to query it without errors, even when the input file contains
malformed rows, by skipping the problematic rows.
Both on_error and log_verbosity options apply to SELECT and ANALYZE
operations on file_fdw foreign tables.
Author: Atsushi Torikoshi
Reviewed-by: Masahiko Sawada, Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
contrib/file_fdw/expected/file_fdw.out | 19 +++++
contrib/file_fdw/file_fdw.c | 107 +++++++++++++++++++++----
contrib/file_fdw/sql/file_fdw.sql | 7 ++
doc/src/sgml/file-fdw.sgml | 23 ++++++
4 files changed, 140 insertions(+), 16 deletions(-)
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 86c148a86b..593fdc782e 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -206,6 +206,25 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
SELECT * FROM agg_bad; -- ERROR
ERROR: invalid input syntax for type real: "aaa"
CONTEXT: COPY agg_bad, line 3, column b: "aaa"
+-- on_error and log_verbosity tests
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
+SELECT * FROM agg_bad;
+NOTICE: 1 row was skipped due to data type incompatibility
+ a | b
+-----+--------
+ 100 | 99.097
+ 42 | 324.78
+(2 rows)
+
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD log_verbosity 'silent');
+SELECT * FROM agg_bad;
+ a | b
+-----+--------
+ 100 | 99.097
+ 42 | 324.78
+(2 rows)
+
+ANALYZE agg_bad;
-- misc query tests
\t on
SELECT explain_filter('EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv');
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index d16821f8e1..043204c3e7 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -22,6 +22,7 @@
#include "catalog/pg_authid.h"
#include "catalog/pg_foreign_table.h"
#include "commands/copy.h"
+#include "commands/copyfrom_internal.h"
#include "commands/defrem.h"
#include "commands/explain.h"
#include "commands/vacuum.h"
@@ -74,6 +75,8 @@ static const struct FileFdwOption valid_options[] = {
{"null", ForeignTableRelationId},
{"default", ForeignTableRelationId},
{"encoding", ForeignTableRelationId},
+ {"on_error", ForeignTableRelationId},
+ {"log_verbosity", ForeignTableRelationId},
{"force_not_null", AttributeRelationId},
{"force_null", AttributeRelationId},
@@ -723,38 +726,74 @@ fileIterateForeignScan(ForeignScanState *node)
FileFdwExecutionState *festate = (FileFdwExecutionState *) node->fdw_state;
EState *estate = CreateExecutorState();
ExprContext *econtext;
- MemoryContext oldcontext;
+ MemoryContext oldcontext = CurrentMemoryContext;
TupleTableSlot *slot = node->ss.ss_ScanTupleSlot;
- bool found;
+ CopyFromState cstate = festate->cstate;
ErrorContextCallback errcallback;
/* Set up callback to identify error line number. */
errcallback.callback = CopyFromErrorCallback;
- errcallback.arg = (void *) festate->cstate;
+ errcallback.arg = (void *) cstate;
errcallback.previous = error_context_stack;
error_context_stack = &errcallback;
/*
- * The protocol for loading a virtual tuple into a slot is first
- * ExecClearTuple, then fill the values/isnull arrays, then
- * ExecStoreVirtualTuple. If we don't find another row in the file, we
- * just skip the last step, leaving the slot empty as required.
- *
* We pass ExprContext because there might be a use of the DEFAULT option
* in COPY FROM, so we may need to evaluate default expressions.
*/
- ExecClearTuple(slot);
econtext = GetPerTupleExprContext(estate);
+retry:
+
/*
* DEFAULT expressions need to be evaluated in a per-tuple context, so
* switch in case we are doing that.
*/
- oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
- found = NextCopyFrom(festate->cstate, econtext,
- slot->tts_values, slot->tts_isnull);
- if (found)
+ MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
+ /*
+ * The protocol for loading a virtual tuple into a slot is first
+ * ExecClearTuple, then fill the values/isnull arrays, then
+ * ExecStoreVirtualTuple. If we don't find another row in the file, we
+ * just skip the last step, leaving the slot empty as required.
+ *
+ */
+ ExecClearTuple(slot);
+
+ if (NextCopyFrom(cstate, econtext, slot->tts_values, slot->tts_isnull))
+ {
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->escontext->error_occurred)
+ {
+ /*
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
+ */
+ cstate->escontext->error_occurred = false;
+
+ /* Switch back to original memory context */
+ MemoryContextSwitchTo(oldcontext);
+
+ /*
+ * Make sure we are interruptible while repeatedly calling
+ * NextCopyFrom() until no soft error occurs.
+ */
+ CHECK_FOR_INTERRUPTS();
+
+ /*
+ * Reset the per-tuple exprcontext, to clean-up after expression
+ * evaluations etc.
+ */
+ ResetPerTupleExprContext(estate);
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ goto retry;
+ }
+
ExecStoreVirtualTuple(slot);
+ }
/* Switch back to original memory context */
MemoryContextSwitchTo(oldcontext);
@@ -796,8 +835,19 @@ fileEndForeignScan(ForeignScanState *node)
FileFdwExecutionState *festate = (FileFdwExecutionState *) node->fdw_state;
/* if festate is NULL, we are in EXPLAIN; nothing to do */
- if (festate)
- EndCopyFrom(festate->cstate);
+ if (!festate)
+ return;
+
+ if (festate->cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ festate->cstate->num_errors > 0 &&
+ festate->cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
+ ereport(NOTICE,
+ errmsg_plural("%llu row was skipped due to data type incompatibility",
+ "%llu rows were skipped due to data type incompatibility",
+ (unsigned long long) festate->cstate->num_errors,
+ (unsigned long long) festate->cstate->num_errors));
+
+ EndCopyFrom(festate->cstate);
}
/*
@@ -1113,7 +1163,8 @@ estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
* which must have at least targrows entries.
* The actual number of rows selected is returned as the function result.
* We also count the total number of rows in the file and return it into
- * *totalrows. Note that *totaldeadrows is always set to 0.
+ * *totalrows. Rows skipped due to on_error = 'ignore' are not included
+ * in this count. Note that *totaldeadrows is always set to 0.
*
* Note that the returned list of rows is not always in order by physical
* position in the file. Therefore, correlation estimates derived later
@@ -1191,6 +1242,21 @@ file_acquire_sample_rows(Relation onerel, int elevel,
if (!found)
break;
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->escontext->error_occurred)
+ {
+ /*
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
+ */
+ cstate->escontext->error_occurred = false;
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ continue;
+ }
+
/*
* The first targrows sample rows are simply copied into the
* reservoir. Then we start replacing tuples in the sample until we
@@ -1236,6 +1302,15 @@ file_acquire_sample_rows(Relation onerel, int elevel,
/* Clean up. */
MemoryContextDelete(tupcontext);
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
+ cstate->num_errors > 0 &&
+ cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)
+ ereport(NOTICE,
+ errmsg_plural("%llu row was skipped due to data type incompatibility",
+ "%llu rows were skipped due to data type incompatibility",
+ (unsigned long long) cstate->num_errors,
+ (unsigned long long) cstate->num_errors));
+
EndCopyFrom(cstate);
pfree(values);
diff --git a/contrib/file_fdw/sql/file_fdw.sql b/contrib/file_fdw/sql/file_fdw.sql
index f0548e14e1..edd77c5cd2 100644
--- a/contrib/file_fdw/sql/file_fdw.sql
+++ b/contrib/file_fdw/sql/file_fdw.sql
@@ -150,6 +150,13 @@ SELECT * FROM agg_csv c JOIN agg_text t ON (t.a = c.a) ORDER BY c.a;
-- error context report tests
SELECT * FROM agg_bad; -- ERROR
+-- on_error and log_verbosity tests
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD on_error 'ignore');
+SELECT * FROM agg_bad;
+ALTER FOREIGN TABLE agg_bad OPTIONS (ADD log_verbosity 'silent');
+SELECT * FROM agg_bad;
+ANALYZE agg_bad;
+
-- misc query tests
\t on
SELECT explain_filter('EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv');
diff --git a/doc/src/sgml/file-fdw.sgml b/doc/src/sgml/file-fdw.sgml
index f2f2af9a59..bb3579b077 100644
--- a/doc/src/sgml/file-fdw.sgml
+++ b/doc/src/sgml/file-fdw.sgml
@@ -126,6 +126,29 @@
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>on_error</literal></term>
+
+ <listitem>
+ <para>
+ Specifies how to behave when encountering an error converting a column's
+ input value into its data type,
+ the same as <command>COPY</command>'s <literal>ON_ERROR</literal> option.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>log_verbosity</literal></term>
+
+ <listitem>
+ <para>
+ Specifies the amount of messages emitted by <literal>file_fdw</literal>,
+ the same as <command>COPY</command>'s <literal>LOG_VERBOSITY</literal> option.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
<para>
--
2.45.2
[text/plain] v7-0003-Refactor-CopyFrom-in-copyfrom.c.patch (3.0K, ../../[email protected]/4-v7-0003-Refactor-CopyFrom-in-copyfrom.c.patch)
download | inline diff:
From 6c132dc601d979ed8ffa4fadc0af9a8c73f88fce Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email protected]>
Date: Wed, 25 Sep 2024 21:30:26 +0900
Subject: [PATCH v7 3/3] Refactor CopyFrom() in copyfrom.c.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This commit simplifies CopyFrom() by removing the unnecessary local variable
'skipped', which tracked the number of rows skipped due to on_error = 'ignore'.
That count is already handled by cstate->num_errors, so the 'skipped' variable
was redundant.
Additionally, the condition on_error != COPY_ON_ERROR_STOP is removed.
Since on_error == COPY_ON_ERROR_IGNORE is already checked, and on_error
only has two values (ignore and stop), the additional check was redundant
and made the logic harder to read. Seemingly this was introduced
in preparation for a future patch, but the current checks don’t offer
clear value and have been removed to improve readability.
Author: Atsushi Torikoshi
Reviewed-by: Masahiko Sawada, Fujii Masao
Discussion: https://postgr.es/m/[email protected]
---
src/backend/commands/copyfrom.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 47879994f7..9139a40785 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -657,7 +657,6 @@ CopyFrom(CopyFromState cstate)
CopyMultiInsertInfo multiInsertInfo = {0}; /* pacify compiler */
int64 processed = 0;
int64 excluded = 0;
- int64 skipped = 0;
bool has_before_insert_row_trig;
bool has_instead_insert_row_trig;
bool leafpart_use_multi_insert = false;
@@ -1004,26 +1003,22 @@ CopyFrom(CopyFromState cstate)
if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
break;
- if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
+ if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE &&
cstate->escontext->error_occurred)
{
/*
- * Soft error occurred, skip this tuple and deal with error
- * information according to ON_ERROR.
+ * Soft error occurred, skip this tuple and just make
+ * ErrorSaveContext ready for the next NextCopyFrom. Since we
+ * don't set details_wanted and error_data is not to be filled,
+ * just resetting error_occurred is enough.
*/
- if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)
-
- /*
- * Just make ErrorSaveContext ready for the next NextCopyFrom.
- * Since we don't set details_wanted and error_data is not to
- * be filled, just resetting error_occurred is enough.
- */
- cstate->escontext->error_occurred = false;
+ cstate->escontext->error_occurred = false;
/* Report that this tuple was skipped by the ON_ERROR clause */
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
- ++skipped);
+ cstate->num_errors);
+ /* Repeat NextCopyFrom() until no soft error occurs */
continue;
}
--
2.45.2
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add on_error and log_verbosity options to file_fdw
2024-09-30 15:36 Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
2024-10-02 00:27 ` Re: Add on_error and log_verbosity options to file_fdw Masahiko Sawada <[email protected]>
2024-10-02 06:34 ` Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
@ 2024-10-03 04:23 ` Masahiko Sawada <[email protected]>
2024-10-03 09:03 ` Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Masahiko Sawada @ 2024-10-03 04:23 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: torikoshia <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers
On Tue, Oct 1, 2024 at 11:34 PM Fujii Masao <[email protected]> wrote:
>
>
>
> On 2024/10/02 9:27, Masahiko Sawada wrote:
> > Sorry for being late in joining the review of this patch. Both 0001
> > and 0003 look good to me. I have two comments on the 0002 patch:
>
> Thanks for the review!
>
> > I think that while scanning a file_fdw foreign table with
> > log_verbosity='silent' the query is not interruptible.
>
> You're right. I added CHECK_FOR_INTERRUPTS() in the retry loop.
>
> > Also, we don't switch to the per-tuple memory context when retrying
> > due to a soft error. I'm not sure it's okay as in CopyFrom(), a
> > similar function for COPY command, we switch to the per-tuple memory
> > context every time before parsing an input line. Would it be
> > problematic if we switch to another memory context while parsing an
> > input line? In CopyFrom() we also call ResetPerTupleExprContext() and
> > ExecClearTuple() for every input, so we might want to consider calling
> > them for every input.
>
> Yes, I've updated the patch based on your comment.
> Could you please review the latest version?
Thank you for updating the patch! All patches look good to me.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add on_error and log_verbosity options to file_fdw
2024-09-30 15:36 Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
2024-10-02 00:27 ` Re: Add on_error and log_verbosity options to file_fdw Masahiko Sawada <[email protected]>
2024-10-02 06:34 ` Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
2024-10-03 04:23 ` Re: Add on_error and log_verbosity options to file_fdw Masahiko Sawada <[email protected]>
@ 2024-10-03 09:03 ` Fujii Masao <[email protected]>
2024-10-03 14:17 ` Re: Add on_error and log_verbosity options to file_fdw torikoshia <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Fujii Masao @ 2024-10-03 09:03 UTC (permalink / raw)
To: Masahiko Sawada <[email protected]>; +Cc: torikoshia <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers
On 2024/10/03 13:23, Masahiko Sawada wrote:
> On Tue, Oct 1, 2024 at 11:34 PM Fujii Masao <[email protected]> wrote:
>>
>>
>>
>> On 2024/10/02 9:27, Masahiko Sawada wrote:
>>> Sorry for being late in joining the review of this patch. Both 0001
>>> and 0003 look good to me. I have two comments on the 0002 patch:
>>
>> Thanks for the review!
>>
>>> I think that while scanning a file_fdw foreign table with
>>> log_verbosity='silent' the query is not interruptible.
>>
>> You're right. I added CHECK_FOR_INTERRUPTS() in the retry loop.
>>
>>> Also, we don't switch to the per-tuple memory context when retrying
>>> due to a soft error. I'm not sure it's okay as in CopyFrom(), a
>>> similar function for COPY command, we switch to the per-tuple memory
>>> context every time before parsing an input line. Would it be
>>> problematic if we switch to another memory context while parsing an
>>> input line? In CopyFrom() we also call ResetPerTupleExprContext() and
>>> ExecClearTuple() for every input, so we might want to consider calling
>>> them for every input.
>>
>> Yes, I've updated the patch based on your comment.
>> Could you please review the latest version?
>
> Thank you for updating the patch! All patches look good to me.
Thanks for the review! Pushed.
BTW, regarding the issue I mentioned earlier about file_fdw not reporting
the number of tuples processed and skipped in the pg_stat_progress_copy view,
I'll start a new thread to discuss this further.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add on_error and log_verbosity options to file_fdw
2024-09-30 15:36 Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
2024-10-02 00:27 ` Re: Add on_error and log_verbosity options to file_fdw Masahiko Sawada <[email protected]>
2024-10-02 06:34 ` Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
2024-10-03 04:23 ` Re: Add on_error and log_verbosity options to file_fdw Masahiko Sawada <[email protected]>
2024-10-03 09:03 ` Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
@ 2024-10-03 14:17 ` torikoshia <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: torikoshia @ 2024-10-03 14:17 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; [email protected]; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
On 2024-10-03 18:03, Fujii Masao wrote:
> On 2024/10/03 13:23, Masahiko Sawada wrote:
>> On Tue, Oct 1, 2024 at 11:34 PM Fujii Masao
>> <[email protected]> wrote:
>>>
>>>
>>> On 2024/10/02 9:27, Masahiko Sawada wrote:
>>>> Sorry for being late in joining the review of this patch. Both 0001
>>>> and 0003 look good to me. I have two comments on the 0002 patch:
>>>
>>> Thanks for the review!
>>>
>>>> I think that while scanning a file_fdw foreign table with
>>>> log_verbosity='silent' the query is not interruptible.
>>>
>>> You're right. I added CHECK_FOR_INTERRUPTS() in the retry loop.
>>>
>>>> Also, we don't switch to the per-tuple memory context when retrying
>>>> due to a soft error. I'm not sure it's okay as in CopyFrom(), a
>>>> similar function for COPY command, we switch to the per-tuple memory
>>>> context every time before parsing an input line. Would it be
>>>> problematic if we switch to another memory context while parsing an
>>>> input line? In CopyFrom() we also call ResetPerTupleExprContext()
>>>> and
>>>> ExecClearTuple() for every input, so we might want to consider
>>>> calling
>>>> them for every input.
>>>
>>> Yes, I've updated the patch based on your comment.
>>> Could you please review the latest version?
>>
>> Thank you for updating the patch! All patches look good to me.
>
> Thanks for the review! Pushed.
Fujii-san, Sawada-san,
Thanks for your review and modifications!
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v23 6/8] Row pattern recognition patch (docs).
@ 2024-10-25 03:56 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Tatsuo Ishii @ 2024-10-25 03:56 UTC (permalink / raw)
---
doc/src/sgml/advanced.sgml | 82 ++++++++++++++++++++++++++++++++++++
doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++
doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++-
3 files changed, 172 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml
index 755c9f1485..b0b1d1c51e 100644
--- a/doc/src/sgml/advanced.sgml
+++ b/doc/src/sgml/advanced.sgml
@@ -537,6 +537,88 @@ WHERE pos < 3;
<literal>rank</literal> less than 3.
</para>
+ <para>
+ Row pattern common syntax can be used to perform row pattern recognition
+ in a query. The row pattern common syntax includes two sub
+ clauses: <literal>DEFINE</literal>
+ and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines
+ definition variables along with an expression. The expression must be a
+ logical expression, which means it must
+ return <literal>TRUE</literal>, <literal>FALSE</literal>
+ or <literal>NULL</literal>. The expression may comprise column references
+ and functions. Window functions, aggregate functions and subqueries are
+ not allowed. An example of <literal>DEFINE</literal> is as follows.
+
+<programlisting>
+DEFINE
+ LOWPRICE AS price <= 100,
+ UP AS price > PREV(price),
+ DOWN AS price < PREV(price)
+</programlisting>
+
+ Note that <function>PREV</function> returns the price column in the
+ previous row if it's called in a context of row pattern recognition. Thus in
+ the second line the definition variable "UP" is <literal>TRUE</literal>
+ when the price column in the current row is greater than the price column
+ in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when
+ the price column in the current row is lower than the price column in the
+ previous row.
+ </para>
+ <para>
+ Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be
+ used. <literal>PATTERN</literal> defines a sequence of rows that satisfies
+ certain conditions. For example following <literal>PATTERN</literal>
+ defines that a row starts with the condition "LOWPRICE", then one or more
+ rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that
+ "+" means one or more matches. Also you can use "*", which means zero or
+ more matches. If a sequence of rows which satisfies the PATTERN is found,
+ in the starting row of the sequence of rows all window functions and
+ aggregates are shown in the target list. Note that aggregations only look
+ into the matched rows, rather than whole frame. On the second or
+ subsequent rows all window functions are NULL. Aggregates are NULL or 0
+ (count case) depending on its aggregation definition. For rows that do not
+ match on the PATTERN, all window functions and aggregates are shown AS
+ NULL too, except count showing 0. This is because the rows do not match,
+ thus they are in an empty frame. Example of a <literal>SELECT</literal>
+ using the <literal>DEFINE</literal> and <literal>PATTERN</literal> clause
+ is as follows.
+
+<programlisting>
+SELECT company, tdate, price,
+ first_value(price) OVER w,
+ max(price) OVER w,
+ count(price) OVER w
+FROM stock
+ WINDOW w AS (
+ PARTITION BY company
+ ORDER BY tdate
+ ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
+ AFTER MATCH SKIP PAST LAST ROW
+ INITIAL
+ PATTERN (LOWPRICE UP+ DOWN+)
+ DEFINE
+ LOWPRICE AS price <= 100,
+ UP AS price > PREV(price),
+ DOWN AS price < PREV(price)
+);
+</programlisting>
+<screen>
+ company | tdate | price | first_value | max | count
+----------+------------+-------+-------------+-----+-------
+ company1 | 2023-07-01 | 100 | 100 | 200 | 4
+ company1 | 2023-07-02 | 200 | | | 0
+ company1 | 2023-07-03 | 150 | | | 0
+ company1 | 2023-07-04 | 140 | | | 0
+ company1 | 2023-07-05 | 150 | | | 0
+ company1 | 2023-07-06 | 90 | 90 | 130 | 4
+ company1 | 2023-07-07 | 110 | | | 0
+ company1 | 2023-07-08 | 130 | | | 0
+ company1 | 2023-07-09 | 120 | | | 0
+ company1 | 2023-07-10 | 130 | | | 0
+(10 rows)
+</screen>
+ </para>
+
<para>
When a query involves multiple window functions, it is possible to write
out each one with a separate <literal>OVER</literal> clause, but this is
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 58dc06b68b..ef4fa7144f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23263,6 +23263,7 @@ SELECT count(*) FROM sometable;
returns <literal>NULL</literal> if there is no such row.
</para></entry>
</row>
+
</tbody>
</tgroup>
</table>
@@ -23302,6 +23303,59 @@ SELECT count(*) FROM sometable;
Other frame specifications can be used to obtain other effects.
</para>
+ <para>
+ Row pattern recognition navigation functions are listed in
+ <xref linkend="functions-rpr-navigation-table"/>. These functions
+ can be used to describe DEFINE clause of Row pattern recognition.
+ </para>
+
+ <table id="functions-rpr-navigation-table">
+ <title>Row Pattern Navigation Functions</title>
+ <tgroup cols="1">
+ <thead>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ Function
+ </para>
+ <para>
+ Description
+ </para></entry>
+ </row>
+ </thead>
+
+ <tbody>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>prev</primary>
+ </indexterm>
+ <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> )
+ <returnvalue>anyelement</returnvalue>
+ </para>
+ <para>
+ Returns the column value at the previous row;
+ returns NULL if there is no previous row in the window frame.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>next</primary>
+ </indexterm>
+ <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> )
+ <returnvalue>anyelement</returnvalue>
+ </para>
+ <para>
+ Returns the column value at the next row;
+ returns NULL if there is no next row in the window frame.
+ </para></entry>
+ </row>
+
+ </tbody>
+ </tgroup>
+ </table>
+
<note>
<para>
The SQL standard defines a <literal>RESPECT NULLS</literal> or
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index d7089eac0b..7e1c9989ba 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl
The <replaceable class="parameter">frame_clause</replaceable> can be one of
<synopsis>
-{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
-{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
+{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
+{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
</synopsis>
where <replaceable>frame_start</replaceable>
@@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS
a given peer group will be in the frame or excluded from it.
</para>
+ <para>
+ The
+ optional <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+ defines the <firstterm>row pattern recognition condition</firstterm> for
+ this
+ window. <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+ includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST
+ ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls
+ how to proceed to next row position after a match
+ found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the
+ default) next row position is next to the last row of previous match. On
+ the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next
+ row position is always next to the last row of previous
+ match. <literal>DEFINE</literal> defines definition variables along with a
+ boolean expression. <literal>PATTERN</literal> defines a sequence of rows
+ that satisfies certain conditions using variables defined
+ in <literal>DEFINE</literal> clause. If the variable is not defined in
+ the <literal>DEFINE</literal> clause, it is implicitly assumed
+ following is defined in the <literal>DEFINE</literal> clause.
+
+<synopsis>
+<literal>variable_name</literal> AS TRUE
+</synopsis>
+
+ Note that the maximu number of variables defined
+ in <literal>DEFINE</literal> clause is 26.
+
+<synopsis>
+[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ]
+PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...]
+DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...]
+</synopsis>
+ </para>
+
<para>
The purpose of a <literal>WINDOW</literal> clause is to specify the
behavior of <firstterm>window functions</firstterm> appearing in the query's
--
2.25.1
----Next_Part(Fri_Oct_25_13_04_53_2024_648)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v23-0007-Row-pattern-recognition-patch-tests.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-10-25 03:56 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-09-30 15:36 Re: Add on_error and log_verbosity options to file_fdw Fujii Masao <[email protected]>
2024-10-02 00:27 ` Masahiko Sawada <[email protected]>
2024-10-02 06:34 ` Fujii Masao <[email protected]>
2024-10-03 04:23 ` Masahiko Sawada <[email protected]>
2024-10-03 09:03 ` Fujii Masao <[email protected]>
2024-10-03 14:17 ` torikoshia <[email protected]>
2024-10-25 03:56 [PATCH v23 6/8] Row pattern recognition patch (docs). Tatsuo Ishii <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox