public inbox for [email protected]help / color / mirror / Atom feed
Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 19+ messages / 6 participants [nested] [flat]
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) @ 2023-02-05 23:37 Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Andres Freund @ 2023-02-05 23:37 UTC (permalink / raw) To: Damir Belyalov <[email protected]>; +Cc: Danil Anisimow <[email protected]>; [email protected] <[email protected]>; torikoshia <[email protected]>; [email protected] <[email protected]>; pgsql-hackers H, On 2023-02-03 13:27:24 +0300, Damir Belyalov wrote: > @@ -625,6 +628,173 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri, > miinfo->bufferedBytes += tuplen; > } > > +/* > + * Safely reads source data, converts to a tuple and fills tuple buffer. > + * Skips some data in the case of failed conversion if data source for > + * a next tuple can be surely read without a danger. > + */ > +static bool > +SafeCopying(CopyFromState cstate, ExprContext *econtext, TupleTableSlot *myslot) > + BeginInternalSubTransaction(NULL); > + CurrentResourceOwner = sfcstate->oldowner; I don't think this is the right approach. Creating a subtransaction for each row will cause substantial performance issues. We now can call data type input functions without throwing errors, see InputFunctionCallSafe(). Use that to avoid throwing an error instead of catching it. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> @ 2023-02-06 05:00 ` Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Damir Belyalov @ 2023-02-06 05:00 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Danil Anisimow <[email protected]>; [email protected] <[email protected]>; torikoshia <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Hi, Andres! Thank you for reviewing. > I don't think this is the right approach. Creating a subtransaction for > each row will cause substantial performance issues. > Subtransactions aren't created for each row. The block of rows in one subtransaction is 1000 (SAFE_BUFFER_SIZE) and can be changed. There is also a constraint for the number of bytes MAX_SAFE_BUFFER_BYTES in safe_buffer: while (sfcstate->saved_tuples < SAFE_BUFFER_SIZE && sfcstate->safeBufferBytes < MAX_SAFE_BUFFER_BYTES) We now can call data type input functions without throwing errors, see > InputFunctionCallSafe(). Use that to avoid throwing an error instead of > catching it. > InputFunctionCallSafe() is good for detecting errors from input-functions but there are such errors from NextCopyFrom () that can not be detected with InputFunctionCallSafe(), e.g. "wrong number of columns in row''. Do you offer to process input-function errors separately from other errors? Now all errors are processed in one "switch" loop in PG_CATCH, so this change can complicate code. Regards, Damir Belyalov Postgres Professional ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> @ 2023-02-06 05:12 ` Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Tom Lane @ 2023-02-06 05:12 UTC (permalink / raw) To: Damir Belyalov <[email protected]>; +Cc: Andres Freund <[email protected]>; Danil Anisimow <[email protected]>; [email protected] <[email protected]>; torikoshia <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Damir Belyalov <[email protected]> writes: >> I don't think this is the right approach. Creating a subtransaction for >> each row will cause substantial performance issues. > Subtransactions aren't created for each row. The block of rows in one > subtransaction is 1000 (SAFE_BUFFER_SIZE) and can be changed. I think that at this point, any patch that involves adding subtransactions to COPY is dead on arrival; whether it's batched or not is irrelevant. (It's not like batching has no downsides.) > InputFunctionCallSafe() is good for detecting errors from input-functions > but there are such errors from NextCopyFrom () that can not be detected > with InputFunctionCallSafe(), e.g. "wrong number of columns in row''. If you want to deal with those, then there's more work to be done to make those bits non-error-throwing. But there's a very finite amount of code involved and no obvious reason why it couldn't be done. The major problem here has always been the indefinite amount of code implicated by calling datatype input functions, and we have now created a plausible answer to that problem. regards, tom lane ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> @ 2023-02-06 05:52 ` Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Andres Freund @ 2023-02-06 05:52 UTC (permalink / raw) To: Tom Lane <[email protected]>; Damir Belyalov <[email protected]>; +Cc: Danil Anisimow <[email protected]>; [email protected] <[email protected]>; torikoshia <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Hi, On February 5, 2023 9:12:17 PM PST, Tom Lane <[email protected]> wrote: >Damir Belyalov <[email protected]> writes: >>> I don't think this is the right approach. Creating a subtransaction for >>> each row will cause substantial performance issues. > >> Subtransactions aren't created for each row. The block of rows in one >> subtransaction is 1000 (SAFE_BUFFER_SIZE) and can be changed. > >I think that at this point, any patch that involves adding subtransactions >to COPY is dead on arrival; whether it's batched or not is irrelevant. >(It's not like batching has no downsides.) Indeed. >> InputFunctionCallSafe() is good for detecting errors from input-functions >> but there are such errors from NextCopyFrom () that can not be detected >> with InputFunctionCallSafe(), e.g. "wrong number of columns in row''. > >If you want to deal with those, then there's more work to be done to make >those bits non-error-throwing. But there's a very finite amount of code >involved and no obvious reason why it couldn't be done. The major problem >here has always been the indefinite amount of code implicated by calling >datatype input functions, and we have now created a plausible answer to >that problem. I'm not even sure it makes sense to avoid that kind of error. And invalid column count or such is something quite different than failing some data type input routine, or falling a constraint. -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> @ 2023-02-06 06:00 ` Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Tom Lane @ 2023-02-06 06:00 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Damir Belyalov <[email protected]>; Danil Anisimow <[email protected]>; [email protected] <[email protected]>; torikoshia <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: > On February 5, 2023 9:12:17 PM PST, Tom Lane <[email protected]> wrote: >> Damir Belyalov <[email protected]> writes: >>> InputFunctionCallSafe() is good for detecting errors from input-functions >>> but there are such errors from NextCopyFrom () that can not be detected >>> with InputFunctionCallSafe(), e.g. "wrong number of columns in row''. >> If you want to deal with those, then there's more work to be done to make >> those bits non-error-throwing. But there's a very finite amount of code >> involved and no obvious reason why it couldn't be done. > I'm not even sure it makes sense to avoid that kind of error. And > invalid column count or such is something quite different than failing > some data type input routine, or falling a constraint. I think it could be reasonable to put COPY's overall-line-format requirements on the same level as datatype input format violations. I agree that trying to trap every kind of error is a bad idea, for largely the same reason that the soft-input-errors patches only trap certain kinds of errors: it's too hard to tell whether an error is an "internal" error that it's scary to continue past. regards, tom lane ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> @ 2023-02-27 05:58 ` torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: torikoshia @ 2023-02-27 05:58 UTC (permalink / raw) To: pgsql-hackers; +Cc: Andres Freund <[email protected]>; Damir Belyalov <[email protected]>; Danil Anisimow <[email protected]>; [email protected]; [email protected]; [email protected] On 2023-02-06 15:00, Tom Lane wrote: > Andres Freund <[email protected]> writes: >> On February 5, 2023 9:12:17 PM PST, Tom Lane <[email protected]> >> wrote: >>> Damir Belyalov <[email protected]> writes: >>>> InputFunctionCallSafe() is good for detecting errors from >>>> input-functions >>>> but there are such errors from NextCopyFrom () that can not be >>>> detected >>>> with InputFunctionCallSafe(), e.g. "wrong number of columns in >>>> row''. > >>> If you want to deal with those, then there's more work to be done to >>> make >>> those bits non-error-throwing. But there's a very finite amount of >>> code >>> involved and no obvious reason why it couldn't be done. > >> I'm not even sure it makes sense to avoid that kind of error. And >> invalid column count or such is something quite different than failing >> some data type input routine, or falling a constraint. > > I think it could be reasonable to put COPY's overall-line-format > requirements on the same level as datatype input format violations. > I agree that trying to trap every kind of error is a bad idea, > for largely the same reason that the soft-input-errors patches > only trap certain kinds of errors: it's too hard to tell whether > an error is an "internal" error that it's scary to continue past. Is it a bad idea to limit the scope of allowing errors to 'soft' errors in InputFunctionCallSafe()? I think it could be still useful for some usecases. diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +1 {1} 1 +a {2} 2 +3 {3} 3333333333 +4 {a, 4} 4 + +5 {5} 5 +\. +SELECT * FROM check_ign_err; diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index 090ef6c7a8..08e8056fc1 100644 +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +WARNING: invalid input syntax for type integer: "a" +WARNING: value "3333333333" is out of range for type integer +WARNING: invalid input syntax for type integer: "a" +WARNING: invalid input syntax for type integer: "" +SELECT * FROM check_ign_err; + n | m | k +---+-----+--- + 1 | {1} | 1 + 5 | {5} | 5 +(2 rows) -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION Attachments: [text/x-diff] v1-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch (9.2K, ../../[email protected]/2-v1-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch) download | inline diff: From 16877d4cdd64db5f85bed9cd559e618d8211e598 Mon Sep 17 00:00:00 2001 From: Atsushi Torikoshi <[email protected]> Date: Mon, 27 Feb 2023 12:02:16 +0900 Subject: [PATCH v1] Add COPY option IGNORE_DATATYPE_ERRORS --- src/backend/commands/copy.c | 8 ++++++++ src/backend/commands/copyfrom.c | 11 +++++++++++ src/backend/commands/copyfromparse.c | 12 ++++++++++-- src/backend/parser/gram.y | 8 +++++++- src/bin/psql/tab-complete.c | 3 ++- src/include/commands/copy.h | 1 + src/include/commands/copyfrom_internal.h | 2 ++ src/include/parser/kwlist.h | 1 + src/test/regress/expected/copy2.out | 14 ++++++++++++++ src/test/regress/sql/copy2.sql | 12 ++++++++++++ 10 files changed, 68 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index e34f583ea7..2f1cfb3f4d 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -410,6 +410,7 @@ ProcessCopyOptions(ParseState *pstate, bool format_specified = false; bool freeze_specified = false; bool header_specified = false; + bool ignore_datatype_errors_specified= false; ListCell *option; /* Support external use for option sanity checking */ @@ -449,6 +450,13 @@ ProcessCopyOptions(ParseState *pstate, freeze_specified = true; opts_out->freeze = defGetBoolean(defel); } + else if (strcmp(defel->defname, "ignore_datatype_errors") == 0) + { + if (ignore_datatype_errors_specified) + errorConflictingDefElem(defel, pstate); + ignore_datatype_errors_specified= true; + opts_out->ignore_datatype_errors = defGetBoolean(defel); + } else if (strcmp(defel->defname, "delimiter") == 0) { if (opts_out->delim) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index af52faca6d..24eec6a27d 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -959,6 +959,7 @@ CopyFrom(CopyFromState cstate) { TupleTableSlot *myslot; bool skip_tuple; + ErrorSaveContext escontext = {T_ErrorSaveContext}; CHECK_FOR_INTERRUPTS(); @@ -991,10 +992,20 @@ CopyFrom(CopyFromState cstate) ExecClearTuple(myslot); + if (cstate->opts.ignore_datatype_errors) + { + escontext.details_wanted = true; + cstate->escontext = escontext; + } + /* Directly store the values/nulls array in the slot */ if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull)) break; + /* Soft error occured, skip this tuple */ + if(cstate->escontext.error_occurred) + continue; + ExecStoreVirtualTuple(myslot); /* diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 91b564c2bc..12b1780fd6 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -70,6 +70,7 @@ #include "libpq/pqformat.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/miscnodes.h" #include "pgstat.h" #include "port/pg_bswap.h" #include "utils/builtins.h" @@ -938,10 +939,17 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext, cstate->cur_attname = NameStr(att->attname); cstate->cur_attval = string; - values[m] = InputFunctionCall(&in_functions[m], + if (!InputFunctionCallSafe(&in_functions[m], string, typioparams[m], - att->atttypmod); + att->atttypmod, + (Node *) &cstate->escontext, + &values[m])) + { + ereport(WARNING, + errmsg("%s", cstate->escontext.error_data->message)); + return true; + } if (string != NULL) nulls[m] = false; cstate->cur_attname = NULL; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index a0138382a1..d79d293c0d 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -701,7 +701,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); HANDLER HAVING HEADER_P HOLD HOUR_P - IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE + IDENTITY_P IF_P IGNORE_DATATYPE_ERRORS ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE INCLUDING INCREMENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION @@ -3378,6 +3378,10 @@ copy_opt_item: { $$ = makeDefElem("freeze", (Node *) makeBoolean(true), @1); } + | IGNORE_DATATYPE_ERRORS + { + $$ = makeDefElem("ignore_datatype_errors", (Node *)makeBoolean(true), @1); + } | DELIMITER opt_as Sconst { $$ = makeDefElem("delimiter", (Node *) makeString($3), @1); @@ -16821,6 +16825,7 @@ unreserved_keyword: | HOUR_P | IDENTITY_P | IF_P + | IGNORE_DATATYPE_ERRORS | IMMEDIATE | IMMUTABLE | IMPLICIT_P @@ -17375,6 +17380,7 @@ bare_label_keyword: | HOLD | IDENTITY_P | IF_P + | IGNORE_DATATYPE_ERRORS | ILIKE | IMMEDIATE | IMMUTABLE diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 5e1882eaea..a363351d3b 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2857,7 +2857,8 @@ psql_completion(const char *text, int start, int end) else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(")) COMPLETE_WITH("FORMAT", "FREEZE", "DELIMITER", "NULL", "HEADER", "QUOTE", "ESCAPE", "FORCE_QUOTE", - "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING"); + "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING", + "IGNORE_DATATYPE_ERRORS"); /* Complete COPY <sth> FROM|TO filename WITH (FORMAT */ else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "FORMAT")) diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index 8e5f6ff148..a7eb0f8883 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -42,6 +42,7 @@ typedef struct CopyFormatOptions * -1 if not specified */ bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ + bool ignore_datatype_errors; /* ignore rows with datatype errors */ bool csv_mode; /* Comma Separated Value format? */ CopyHeaderChoice header_line; /* header line? */ char *null_print; /* NULL marker string (server encoding!) */ diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h index 7b1c4327bd..d74c633481 100644 --- a/src/include/commands/copyfrom_internal.h +++ b/src/include/commands/copyfrom_internal.h @@ -16,6 +16,7 @@ #include "commands/copy.h" #include "commands/trigger.h" +#include "nodes/miscnodes.h" /* * Represents the different source cases we need to worry about at @@ -94,6 +95,7 @@ typedef struct CopyFromStateData AttrNumber num_defaults; FmgrInfo *in_functions; /* array of input functions for each attrs */ Oid *typioparams; /* array of element types for in_functions */ + ErrorSaveContext escontext; /* soft error trapper during in_functions execution */ int *defmap; /* array of default att numbers */ ExprState **defexprs; /* array of default att expressions */ bool volatile_defexprs; /* is any of defexprs volatile? */ diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h index bb36213e6f..1d7f9efbc0 100644 --- a/src/include/parser/kwlist.h +++ b/src/include/parser/kwlist.h @@ -196,6 +196,7 @@ PG_KEYWORD("hold", HOLD, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("hour", HOUR_P, UNRESERVED_KEYWORD, AS_LABEL) PG_KEYWORD("identity", IDENTITY_P, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("if", IF_P, UNRESERVED_KEYWORD, BARE_LABEL) +PG_KEYWORD("ignore_datatype_errors", IGNORE_DATATYPE_ERRORS, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("ilike", ILIKE, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL) PG_KEYWORD("immediate", IMMEDIATE, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("immutable", IMMUTABLE, UNRESERVED_KEYWORD, BARE_LABEL) diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index 090ef6c7a8..08e8056fc1 100644 --- a/src/test/regress/expected/copy2.out +++ b/src/test/regress/expected/copy2.out @@ -666,6 +666,20 @@ SELECT * FROM instead_of_insert_tbl; (2 rows) COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +WARNING: invalid input syntax for type integer: "a" +WARNING: value "3333333333" is out of range for type integer +WARNING: invalid input syntax for type integer: "a" +WARNING: invalid input syntax for type integer: "" +SELECT * FROM check_ign_err; + n | m | k +---+-----+--- + 1 | {1} | 1 + 5 | {5} | 5 +(2 rows) + -- clean up DROP TABLE forcetest; DROP TABLE vistest; diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql index b0de82c3aa..380adfce96 100644 --- a/src/test/regress/sql/copy2.sql +++ b/src/test/regress/sql/copy2.sql @@ -464,6 +464,18 @@ test1 SELECT * FROM instead_of_insert_tbl; COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +1 {1} 1 +a {2} 2 +3 {3} 3333333333 +4 {a, 4} 4 + +5 {5} 5 +\. +SELECT * FROM check_ign_err; + -- clean up DROP TABLE forcetest; DROP TABLE vistest; -- 2.25.1 ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> @ 2023-02-28 14:28 ` Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Damir Belyalov @ 2023-02-28 14:28 UTC (permalink / raw) To: torikoshia <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Danil Anisimow <[email protected]>; [email protected]; [email protected]; [email protected] Hello Tested patch on all cases: CIM_SINGLE, CIM_MULTI, CIM_MULTI_CONDITION. As expected it works. Also added a description to copy.sgml and made a review on patch. I added 'ignored_errors' integer parameter that should be output after the option is finished. All errors were added to the system logfile with full detailed context. Maybe it's better to log only error message. file:///home/abc13/Documents/todo_copy/postgres/v2-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch Regards, Damir Belyalov Postgres Professional Attachments: [application/x-patch] v2-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch (10.1K, ../../CALH1LgvQ5eRMi3zO7RSmod4ucru=9ekASpReahR7zDcCabANZw@mail.gmail.com/3-v2-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch) download | inline diff: diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index c25b52d0cb..706b929947 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -34,6 +34,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable FORMAT <replaceable class="parameter">format_name</replaceable> FREEZE [ <replaceable class="parameter">boolean</replaceable> ] + IGNORE_DATATYPE_ERRORS [ <replaceable class="parameter">boolean</replaceable> ] DELIMITER '<replaceable class="parameter">delimiter_character</replaceable>' NULL '<replaceable class="parameter">null_string</replaceable>' HEADER [ <replaceable class="parameter">boolean</replaceable> | MATCH ] @@ -233,6 +234,17 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable </listitem> </varlistentry> + <varlistentry> + <term><literal>IGNORE_DATATYPE_ERRORS</literal></term> + <listitem> + <para> + Drops rows that contain malformed data while copying. These are rows + with columns where the data type's input-function raises an error. + Outputs warnings about rows with incorrect data to system logfile. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>DELIMITER</literal></term> <listitem> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index e34f583ea7..0334894014 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -410,6 +410,7 @@ ProcessCopyOptions(ParseState *pstate, bool format_specified = false; bool freeze_specified = false; bool header_specified = false; + bool ignore_datatype_errors_specified= false; ListCell *option; /* Support external use for option sanity checking */ @@ -449,6 +450,13 @@ ProcessCopyOptions(ParseState *pstate, freeze_specified = true; opts_out->freeze = defGetBoolean(defel); } + else if (strcmp(defel->defname, "ignore_datatype_errors") == 0) + { + if (ignore_datatype_errors_specified) + errorConflictingDefElem(defel, pstate); + ignore_datatype_errors_specified = true; + opts_out->ignore_datatype_errors = defGetBoolean(defel); + } else if (strcmp(defel->defname, "delimiter") == 0) { if (opts_out->delim) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index af52faca6d..ecaa750568 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -955,10 +955,14 @@ CopyFrom(CopyFromState cstate) errcallback.previous = error_context_stack; error_context_stack = &errcallback; + if (cstate->opts.ignore_datatype_errors) + cstate->ignored_errors = 0; + for (;;) { TupleTableSlot *myslot; bool skip_tuple; + ErrorSaveContext escontext = {T_ErrorSaveContext}; CHECK_FOR_INTERRUPTS(); @@ -991,9 +995,26 @@ CopyFrom(CopyFromState cstate) ExecClearTuple(myslot); + if (cstate->opts.ignore_datatype_errors) + { + escontext.details_wanted = true; + cstate->escontext = escontext; + } + /* Directly store the values/nulls array in the slot */ if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull)) + { + if (cstate->opts.ignore_datatype_errors && cstate->ignored_errors > 0) + ereport(WARNING, errmsg("Errors: %ld", cstate->ignored_errors)); break; + } + + /* Soft error occured, skip this tuple */ + if (cstate->escontext.error_occurred) + { + ExecClearTuple(myslot); + continue; + } ExecStoreVirtualTuple(myslot); diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 91b564c2bc..9c36b0dc8b 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -70,6 +70,7 @@ #include "libpq/pqformat.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/miscnodes.h" #include "pgstat.h" #include "port/pg_bswap.h" #include "utils/builtins.h" @@ -938,10 +939,23 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext, cstate->cur_attname = NameStr(att->attname); cstate->cur_attval = string; - values[m] = InputFunctionCall(&in_functions[m], - string, - typioparams[m], - att->atttypmod); + + /* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype errors */ + if (!InputFunctionCallSafe(&in_functions[m], + string, + typioparams[m], + att->atttypmod, + (Node *) &cstate->escontext, + &values[m])) + { + cstate->ignored_errors++; + + ereport(LOG, + errmsg("%s", cstate->escontext.error_data->message)); + + return true; + } + if (string != NULL) nulls[m] = false; cstate->cur_attname = NULL; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index a0138382a1..d79d293c0d 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -701,7 +701,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); HANDLER HAVING HEADER_P HOLD HOUR_P - IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE + IDENTITY_P IF_P IGNORE_DATATYPE_ERRORS ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE INCLUDING INCREMENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION @@ -3378,6 +3378,10 @@ copy_opt_item: { $$ = makeDefElem("freeze", (Node *) makeBoolean(true), @1); } + | IGNORE_DATATYPE_ERRORS + { + $$ = makeDefElem("ignore_datatype_errors", (Node *)makeBoolean(true), @1); + } | DELIMITER opt_as Sconst { $$ = makeDefElem("delimiter", (Node *) makeString($3), @1); @@ -16821,6 +16825,7 @@ unreserved_keyword: | HOUR_P | IDENTITY_P | IF_P + | IGNORE_DATATYPE_ERRORS | IMMEDIATE | IMMUTABLE | IMPLICIT_P @@ -17375,6 +17380,7 @@ bare_label_keyword: | HOLD | IDENTITY_P | IF_P + | IGNORE_DATATYPE_ERRORS | ILIKE | IMMEDIATE | IMMUTABLE diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 5e1882eaea..a363351d3b 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2857,7 +2857,8 @@ psql_completion(const char *text, int start, int end) else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(")) COMPLETE_WITH("FORMAT", "FREEZE", "DELIMITER", "NULL", "HEADER", "QUOTE", "ESCAPE", "FORCE_QUOTE", - "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING"); + "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING", + "IGNORE_DATATYPE_ERRORS"); /* Complete COPY <sth> FROM|TO filename WITH (FORMAT */ else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "FORMAT")) diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index 8e5f6ff148..a7eb0f8883 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -42,6 +42,7 @@ typedef struct CopyFormatOptions * -1 if not specified */ bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ + bool ignore_datatype_errors; /* ignore rows with datatype errors */ bool csv_mode; /* Comma Separated Value format? */ CopyHeaderChoice header_line; /* header line? */ char *null_print; /* NULL marker string (server encoding!) */ diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h index 7b1c4327bd..4724fca195 100644 --- a/src/include/commands/copyfrom_internal.h +++ b/src/include/commands/copyfrom_internal.h @@ -16,6 +16,7 @@ #include "commands/copy.h" #include "commands/trigger.h" +#include "nodes/miscnodes.h" /* * Represents the different source cases we need to worry about at @@ -94,6 +95,8 @@ typedef struct CopyFromStateData AttrNumber num_defaults; FmgrInfo *in_functions; /* array of input functions for each attrs */ Oid *typioparams; /* array of element types for in_functions */ + ErrorSaveContext escontext; /* soft error trapper during in_functions execution */ + uint64 ignored_errors; /* total number of ignored errors */ int *defmap; /* array of default att numbers */ ExprState **defexprs; /* array of default att expressions */ bool volatile_defexprs; /* is any of defexprs volatile? */ diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h index bb36213e6f..1d7f9efbc0 100644 --- a/src/include/parser/kwlist.h +++ b/src/include/parser/kwlist.h @@ -196,6 +196,7 @@ PG_KEYWORD("hold", HOLD, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("hour", HOUR_P, UNRESERVED_KEYWORD, AS_LABEL) PG_KEYWORD("identity", IDENTITY_P, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("if", IF_P, UNRESERVED_KEYWORD, BARE_LABEL) +PG_KEYWORD("ignore_datatype_errors", IGNORE_DATATYPE_ERRORS, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("ilike", ILIKE, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL) PG_KEYWORD("immediate", IMMEDIATE, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("immutable", IMMUTABLE, UNRESERVED_KEYWORD, BARE_LABEL) diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index 090ef6c7a8..b4dadbf7a9 100644 --- a/src/test/regress/expected/copy2.out +++ b/src/test/regress/expected/copy2.out @@ -666,6 +666,17 @@ SELECT * FROM instead_of_insert_tbl; (2 rows) COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +WARNING: Errors: 4 +SELECT * FROM check_ign_err; + n | m | k +---+-----+--- + 1 | {1} | 1 + 5 | {5} | 5 +(2 rows) + -- clean up DROP TABLE forcetest; DROP TABLE vistest; diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql index b0de82c3aa..380adfce96 100644 --- a/src/test/regress/sql/copy2.sql +++ b/src/test/regress/sql/copy2.sql @@ -464,6 +464,18 @@ test1 SELECT * FROM instead_of_insert_tbl; COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +1 {1} 1 +a {2} 2 +3 {3} 3333333333 +4 {a, 4} 4 + +5 {5} 5 +\. +SELECT * FROM check_ign_err; + -- clean up DROP TABLE forcetest; DROP TABLE vistest; ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> @ 2023-03-06 14:03 ` Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Daniel Gustafsson @ 2023-03-06 14:03 UTC (permalink / raw) To: Damir Belyalov <[email protected]>; +Cc: torikoshia <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Danil Anisimow <[email protected]>; [email protected]; [email protected]; [email protected] > On 28 Feb 2023, at 15:28, Damir Belyalov <[email protected]> wrote: > Tested patch on all cases: CIM_SINGLE, CIM_MULTI, CIM_MULTI_CONDITION. As expected it works. > Also added a description to copy.sgml and made a review on patch. > > I added 'ignored_errors' integer parameter that should be output after the option is finished. > All errors were added to the system logfile with full detailed context. Maybe it's better to log only error message. FWIW, Greenplum has a similar construct (but which also logs the errors in the db) where data type errors are skipped as long as the number of errors don't exceed a reject limit. If the reject limit is reached then the COPY fails: LOG ERRORS [ SEGMENT REJECT LIMIT <count> [ ROWS | PERCENT ]] IIRC the gist of this was to catch then the user copies the wrong input data or plain has a broken file. Rather than finding out after copying n rows which are likely to be garbage the process can be restarted. This version of the patch has a compiler error in the error message: copyfrom.c: In function ‘CopyFrom’: copyfrom.c:1008:29: error: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘uint64’ {aka ‘long long unsigned int’} [-Werror=format=] 1008 | ereport(WARNING, errmsg("Errors: %ld", cstate->ignored_errors)); | ^~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~ | | | uint64 {aka long long unsigned int} On that note though, it seems to me that this error message leaves a bit to be desired with regards to the level of detail. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> @ 2023-03-07 02:07 ` torikoshia <[email protected]> 2023-03-07 08:35 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: torikoshia @ 2023-03-07 02:07 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; [email protected]; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Danil Anisimow <[email protected]>; [email protected]; [email protected]; [email protected] On 2023-03-06 23:03, Daniel Gustafsson wrote: >> On 28 Feb 2023, at 15:28, Damir Belyalov <[email protected]> wrote: > >> Tested patch on all cases: CIM_SINGLE, CIM_MULTI, CIM_MULTI_CONDITION. >> As expected it works. >> Also added a description to copy.sgml and made a review on patch. Thanks for your tests and improvements! >> I added 'ignored_errors' integer parameter that should be output after >> the option is finished. >> All errors were added to the system logfile with full detailed >> context. Maybe it's better to log only error message. Certainly. > FWIW, Greenplum has a similar construct (but which also logs the errors > in the > db) where data type errors are skipped as long as the number of errors > don't > exceed a reject limit. If the reject limit is reached then the COPY > fails: > > LOG ERRORS [ SEGMENT REJECT LIMIT <count> [ ROWS | PERCENT ]] > > IIRC the gist of this was to catch then the user copies the wrong input > data or > plain has a broken file. Rather than finding out after copying n rows > which > are likely to be garbage the process can be restarted. > > This version of the patch has a compiler error in the error message: > > copyfrom.c: In function ‘CopyFrom’: > copyfrom.c:1008:29: error: format ‘%ld’ expects argument of type ‘long > int’, but argument 2 has type ‘uint64’ {aka ‘long long unsigned int’} > [-Werror=format=] > 1008 | ereport(WARNING, errmsg("Errors: %ld", cstate->ignored_errors)); > | ^~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~ > | | > | uint64 {aka long > long unsigned int} > > > On that note though, it seems to me that this error message leaves a > bit to be > desired with regards to the level of detail. +1. I felt just logging "Error: %ld" would make people wonder the meaning of the %ld. Logging something like ""Error: %ld data type errors were found" might be clearer. -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> @ 2023-03-07 08:35 ` Damir Belyalov <[email protected]> 2023-03-07 09:09 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Damir Belyalov @ 2023-03-07 08:35 UTC (permalink / raw) To: torikoshia <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Danil Anisimow <[email protected]>; [email protected]; [email protected]; [email protected] > > FWIW, Greenplum has a similar construct (but which also logs the errors > in the > db) where data type errors are skipped as long as the number of errors > don't > exceed a reject limit. If the reject limit is reached then the COPY > fails: > > > > LOG ERRORS [ SEGMENT REJECT LIMIT <count> [ ROWS | PERCENT ]] > > > IIRC the gist of this was to catch then the user copies the wrong input > data or > plain has a broken file. Rather than finding out after copying n rows > which > are likely to be garbage the process can be restarted. > I think this is a matter for discussion. The same question is: "Where to log errors to separate files or to the system logfile?". IMO it's better for users to log short-detailed error message to system logfile and not output errors to the terminal. This version of the patch has a compiler error in the error message: > Yes, corrected it. Changed "ignored_errors" to int64 because "processed" (used for counting copy rows) is int64. I felt just logging "Error: %ld" would make people wonder the meaning of > the %ld. Logging something like ""Error: %ld data type errors were > found" might be clearer. > Thanks. For more clearance change the message to: "Errors were found: %". Regards, Damir Belyalov Postgres Professional Attachments: [text/x-patch] v3-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch (10.1K, ../../CALH1LguAEsoTYJTCsXNB-7z2Hu9UGEpsXA4kj0FOTmoP=6Wp3Q@mail.gmail.com/3-v3-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch) download | inline diff: diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index c25b52d0cb..706b929947 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -34,6 +34,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable FORMAT <replaceable class="parameter">format_name</replaceable> FREEZE [ <replaceable class="parameter">boolean</replaceable> ] + IGNORE_DATATYPE_ERRORS [ <replaceable class="parameter">boolean</replaceable> ] DELIMITER '<replaceable class="parameter">delimiter_character</replaceable>' NULL '<replaceable class="parameter">null_string</replaceable>' HEADER [ <replaceable class="parameter">boolean</replaceable> | MATCH ] @@ -233,6 +234,17 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable </listitem> </varlistentry> + <varlistentry> + <term><literal>IGNORE_DATATYPE_ERRORS</literal></term> + <listitem> + <para> + Drops rows that contain malformed data while copying. These are rows + with columns where the data type's input-function raises an error. + Outputs warnings about rows with incorrect data to system logfile. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>DELIMITER</literal></term> <listitem> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index e34f583ea7..0334894014 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -410,6 +410,7 @@ ProcessCopyOptions(ParseState *pstate, bool format_specified = false; bool freeze_specified = false; bool header_specified = false; + bool ignore_datatype_errors_specified= false; ListCell *option; /* Support external use for option sanity checking */ @@ -449,6 +450,13 @@ ProcessCopyOptions(ParseState *pstate, freeze_specified = true; opts_out->freeze = defGetBoolean(defel); } + else if (strcmp(defel->defname, "ignore_datatype_errors") == 0) + { + if (ignore_datatype_errors_specified) + errorConflictingDefElem(defel, pstate); + ignore_datatype_errors_specified = true; + opts_out->ignore_datatype_errors = defGetBoolean(defel); + } else if (strcmp(defel->defname, "delimiter") == 0) { if (opts_out->delim) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 29cd1cf4a6..facfc44def 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -949,10 +949,14 @@ CopyFrom(CopyFromState cstate) errcallback.previous = error_context_stack; error_context_stack = &errcallback; + if (cstate->opts.ignore_datatype_errors) + cstate->ignored_errors = 0; + for (;;) { TupleTableSlot *myslot; bool skip_tuple; + ErrorSaveContext escontext = {T_ErrorSaveContext}; CHECK_FOR_INTERRUPTS(); @@ -985,9 +989,26 @@ CopyFrom(CopyFromState cstate) ExecClearTuple(myslot); + if (cstate->opts.ignore_datatype_errors) + { + escontext.details_wanted = true; + cstate->escontext = escontext; + } + /* Directly store the values/nulls array in the slot */ if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull)) + { + if (cstate->opts.ignore_datatype_errors && cstate->ignored_errors > 0) + ereport(WARNING, errmsg("Errors were found: %lld", (long long) cstate->ignored_errors)); break; + } + + /* Soft error occured, skip this tuple */ + if (cstate->escontext.error_occurred) + { + ExecClearTuple(myslot); + continue; + } ExecStoreVirtualTuple(myslot); diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 91b564c2bc..9c36b0dc8b 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -70,6 +70,7 @@ #include "libpq/pqformat.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/miscnodes.h" #include "pgstat.h" #include "port/pg_bswap.h" #include "utils/builtins.h" @@ -938,10 +939,23 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext, cstate->cur_attname = NameStr(att->attname); cstate->cur_attval = string; - values[m] = InputFunctionCall(&in_functions[m], - string, - typioparams[m], - att->atttypmod); + + /* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype errors */ + if (!InputFunctionCallSafe(&in_functions[m], + string, + typioparams[m], + att->atttypmod, + (Node *) &cstate->escontext, + &values[m])) + { + cstate->ignored_errors++; + + ereport(LOG, + errmsg("%s", cstate->escontext.error_data->message)); + + return true; + } + if (string != NULL) nulls[m] = false; cstate->cur_attname = NULL; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index a0138382a1..d79d293c0d 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -701,7 +701,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); HANDLER HAVING HEADER_P HOLD HOUR_P - IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE + IDENTITY_P IF_P IGNORE_DATATYPE_ERRORS ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE INCLUDING INCREMENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION @@ -3378,6 +3378,10 @@ copy_opt_item: { $$ = makeDefElem("freeze", (Node *) makeBoolean(true), @1); } + | IGNORE_DATATYPE_ERRORS + { + $$ = makeDefElem("ignore_datatype_errors", (Node *)makeBoolean(true), @1); + } | DELIMITER opt_as Sconst { $$ = makeDefElem("delimiter", (Node *) makeString($3), @1); @@ -16821,6 +16825,7 @@ unreserved_keyword: | HOUR_P | IDENTITY_P | IF_P + | IGNORE_DATATYPE_ERRORS | IMMEDIATE | IMMUTABLE | IMPLICIT_P @@ -17375,6 +17380,7 @@ bare_label_keyword: | HOLD | IDENTITY_P | IF_P + | IGNORE_DATATYPE_ERRORS | ILIKE | IMMEDIATE | IMMUTABLE diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 8f12af799b..0f290cd6ff 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2857,7 +2857,8 @@ psql_completion(const char *text, int start, int end) else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(")) COMPLETE_WITH("FORMAT", "FREEZE", "DELIMITER", "NULL", "HEADER", "QUOTE", "ESCAPE", "FORCE_QUOTE", - "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING"); + "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING", + "IGNORE_DATATYPE_ERRORS"); /* Complete COPY <sth> FROM|TO filename WITH (FORMAT */ else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "FORMAT")) diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index 8e5f6ff148..a7eb0f8883 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -42,6 +42,7 @@ typedef struct CopyFormatOptions * -1 if not specified */ bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ + bool ignore_datatype_errors; /* ignore rows with datatype errors */ bool csv_mode; /* Comma Separated Value format? */ CopyHeaderChoice header_line; /* header line? */ char *null_print; /* NULL marker string (server encoding!) */ diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h index 7b1c4327bd..b9ce636f7b 100644 --- a/src/include/commands/copyfrom_internal.h +++ b/src/include/commands/copyfrom_internal.h @@ -16,6 +16,7 @@ #include "commands/copy.h" #include "commands/trigger.h" +#include "nodes/miscnodes.h" /* * Represents the different source cases we need to worry about at @@ -94,6 +95,8 @@ typedef struct CopyFromStateData AttrNumber num_defaults; FmgrInfo *in_functions; /* array of input functions for each attrs */ Oid *typioparams; /* array of element types for in_functions */ + ErrorSaveContext escontext; /* soft error trapper during in_functions execution */ + int64 ignored_errors; /* total number of ignored errors */ int *defmap; /* array of default att numbers */ ExprState **defexprs; /* array of default att expressions */ bool volatile_defexprs; /* is any of defexprs volatile? */ diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h index bb36213e6f..1d7f9efbc0 100644 --- a/src/include/parser/kwlist.h +++ b/src/include/parser/kwlist.h @@ -196,6 +196,7 @@ PG_KEYWORD("hold", HOLD, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("hour", HOUR_P, UNRESERVED_KEYWORD, AS_LABEL) PG_KEYWORD("identity", IDENTITY_P, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("if", IF_P, UNRESERVED_KEYWORD, BARE_LABEL) +PG_KEYWORD("ignore_datatype_errors", IGNORE_DATATYPE_ERRORS, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("ilike", ILIKE, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL) PG_KEYWORD("immediate", IMMEDIATE, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("immutable", IMMUTABLE, UNRESERVED_KEYWORD, BARE_LABEL) diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index 090ef6c7a8..525e3bc454 100644 --- a/src/test/regress/expected/copy2.out +++ b/src/test/regress/expected/copy2.out @@ -666,6 +666,17 @@ SELECT * FROM instead_of_insert_tbl; (2 rows) COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +WARNING: Errors were found: 4 +SELECT * FROM check_ign_err; + n | m | k +---+-----+--- + 1 | {1} | 1 + 5 | {5} | 5 +(2 rows) + -- clean up DROP TABLE forcetest; DROP TABLE vistest; diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql index b0de82c3aa..380adfce96 100644 --- a/src/test/regress/sql/copy2.sql +++ b/src/test/regress/sql/copy2.sql @@ -464,6 +464,18 @@ test1 SELECT * FROM instead_of_insert_tbl; COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +1 {1} 1 +a {2} 2 +3 {3} 3333333333 +4 {a, 4} 4 + +5 {5} 5 +\. +SELECT * FROM check_ign_err; + -- clean up DROP TABLE forcetest; DROP TABLE vistest; ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-07 08:35 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> @ 2023-03-07 09:09 ` Daniel Gustafsson <[email protected]> 2023-03-17 12:23 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Daniel Gustafsson @ 2023-03-07 09:09 UTC (permalink / raw) To: Damir Belyalov <[email protected]>; +Cc: torikoshia <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Danil Anisimow <[email protected]>; Nikita Malakhov <[email protected]>; [email protected]; [email protected] > On 7 Mar 2023, at 09:35, Damir Belyalov <[email protected]> wrote: > I felt just logging "Error: %ld" would make people wonder the meaning of > the %ld. Logging something like ""Error: %ld data type errors were > found" might be clearer. > > Thanks. For more clearance change the message to: "Errors were found: %". I'm not convinced that this adds enough clarity to assist the user. We also shouldn't use "error" in a WARNING log since the user has explicitly asked to skip rows on error, so it's not an error per se. How about something like: ereport(WARNING, (errmsg("%ld rows were skipped due to data type incompatibility", cstate->ignored_errors), errhint("Skipped rows can be inspected in the database log for reprocessing."))); -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-07 08:35 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-07 09:09 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> @ 2023-03-17 12:23 ` torikoshia <[email protected]> 2023-03-22 13:34 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: torikoshia @ 2023-03-17 12:23 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; Damir Belyalov <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Danil Anisimow <[email protected]>; Nikita Malakhov <[email protected]>; [email protected]; [email protected] On 2023-03-07 18:09, Daniel Gustafsson wrote: >> On 7 Mar 2023, at 09:35, Damir Belyalov <[email protected]> wrote: > >> I felt just logging "Error: %ld" would make people wonder the meaning >> of >> the %ld. Logging something like ""Error: %ld data type errors were >> found" might be clearer. >> >> Thanks. For more clearance change the message to: "Errors were found: >> %". > > I'm not convinced that this adds enough clarity to assist the user. We > also > shouldn't use "error" in a WARNING log since the user has explicitly > asked to > skip rows on error, so it's not an error per se. +1 > How about something like: > > ereport(WARNING, > (errmsg("%ld rows were skipped due to data type > incompatibility", cstate->ignored_errors), > errhint("Skipped rows can be inspected in the database log > for reprocessing."))); Since skipped rows cannot be inspected in the log when log_error_verbosity is set to terse, it might be better without this errhint. -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-07 08:35 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-07 09:09 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-17 12:23 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> @ 2023-03-22 13:34 ` torikoshia <[email protected]> 2023-03-22 17:50 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: torikoshia @ 2023-03-22 13:34 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; Damir Belyalov <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Danil Anisimow <[email protected]>; Nikita Malakhov <[email protected]>; [email protected]; [email protected] On 2023-03-17 21:23, torikoshia wrote: > On 2023-03-07 18:09, Daniel Gustafsson wrote: >>> On 7 Mar 2023, at 09:35, Damir Belyalov <[email protected]> wrote: >> >>> I felt just logging "Error: %ld" would make people wonder the meaning >>> of >>> the %ld. Logging something like ""Error: %ld data type errors were >>> found" might be clearer. >>> >>> Thanks. For more clearance change the message to: "Errors were found: >>> %". >> >> I'm not convinced that this adds enough clarity to assist the user. >> We also >> shouldn't use "error" in a WARNING log since the user has explicitly >> asked to >> skip rows on error, so it's not an error per se. > +1 > >> How about something like: >> >> ereport(WARNING, >> (errmsg("%ld rows were skipped due to data type >> incompatibility", cstate->ignored_errors), >> errhint("Skipped rows can be inspected in the database log >> for reprocessing."))); > Since skipped rows cannot be inspected in the log when > log_error_verbosity is set to terse, > it might be better without this errhint. Removed errhint. Modified some codes since v3 couldn't be applied HEAD anymore. Also modified v3 patch as below: > 65 + if (cstate->opts.ignore_datatype_errors) > 66 + cstate->ignored_errors = 0; > 67 + It seems not necessary since cstate is initialized by palloc0() in BeginCopyFrom(). > 134 + ereport(LOG, > 135 + errmsg("%s", > cstate->escontext.error_data->message)); > 136 + > 137 + return true; Since LOG means 'Reports information of interest to administrators' according to the manual[1], datatype error should not be logged as LOG. I put it back in WARNING. [1] https://www.postgresql.org/docs/current/runtime-config-logging.html#RUNTIME-CONFIG-SEVERITY-LEVELS -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION Attachments: [text/x-diff] v4-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch (10.8K, ../../[email protected]/2-v4-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch) download | inline diff: From 6764d7e0f21ca266d7426cb922fd00e5138ec857 Mon Sep 17 00:00:00 2001 From: Atsushi Torikoshi <[email protected]> Date: Wed, 22 Mar 2023 22:00:15 +0900 Subject: [PATCH v4] Add new COPY option IGNORE_DATATYPE_ERRORS Add new COPY option IGNORE_DATATYPE_ERRORS. Currently entire COPY fails even when there is one unexpected data regarding data type or range. IGNORE_DATATYPE_ERRORS ignores these errors and skips them and COPY data which don't contain problem. This patch uses the soft error handling infrastructure, which is introduced by d9f7f5d32f20. Author: Damir Belyalov, Atsushi Torikoshi --- doc/src/sgml/ref/copy.sgml | 12 ++++++++++++ src/backend/commands/copy.c | 8 ++++++++ src/backend/commands/copyfrom.c | 20 ++++++++++++++++++++ src/backend/commands/copyfromparse.c | 19 +++++++++++++++---- src/backend/parser/gram.y | 8 +++++++- src/include/commands/copy.h | 1 + src/include/commands/copyfrom_internal.h | 3 +++ src/include/parser/kwlist.h | 1 + src/test/regress/expected/copy2.out | 15 +++++++++++++++ src/test/regress/sql/copy2.sql | 12 ++++++++++++ 10 files changed, 94 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 5e591ed2e6..168b1c05d9 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -34,6 +34,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable FORMAT <replaceable class="parameter">format_name</replaceable> FREEZE [ <replaceable class="parameter">boolean</replaceable> ] + IGNORE_DATATYPE_ERRORS [ <replaceable class="parameter">boolean</replaceable> ] DELIMITER '<replaceable class="parameter">delimiter_character</replaceable>' NULL '<replaceable class="parameter">null_string</replaceable>' HEADER [ <replaceable class="parameter">boolean</replaceable> | MATCH ] @@ -234,6 +235,17 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable </listitem> </varlistentry> + <varlistentry> + <term><literal>IGNORE_DATATYPE_ERRORS</literal></term> + <listitem> + <para> + Drops rows that contain malformed data while copying. These are rows + with columns where the data type's input-function raises an error. + Outputs warnings about rows with incorrect data to system logfile. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>DELIMITER</literal></term> <listitem> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index f14fae3308..02d911abbe 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -419,6 +419,7 @@ ProcessCopyOptions(ParseState *pstate, bool format_specified = false; bool freeze_specified = false; bool header_specified = false; + bool ignore_datatype_errors_specified = false; ListCell *option; /* Support external use for option sanity checking */ @@ -458,6 +459,13 @@ ProcessCopyOptions(ParseState *pstate, freeze_specified = true; opts_out->freeze = defGetBoolean(defel); } + else if (strcmp(defel->defname, "ignore_datatype_errors") == 0) + { + if (ignore_datatype_errors_specified) + errorConflictingDefElem(defel, pstate); + ignore_datatype_errors_specified = true; + opts_out->ignore_datatype_errors = defGetBoolean(defel); + } else if (strcmp(defel->defname, "delimiter") == 0) { if (opts_out->delim) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 80bca79cd0..85c47f54b2 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -953,6 +953,7 @@ CopyFrom(CopyFromState cstate) { TupleTableSlot *myslot; bool skip_tuple; + ErrorSaveContext escontext = {T_ErrorSaveContext}; CHECK_FOR_INTERRUPTS(); @@ -985,9 +986,28 @@ CopyFrom(CopyFromState cstate) ExecClearTuple(myslot); + if (cstate->opts.ignore_datatype_errors) + { + escontext.details_wanted = true; + cstate->escontext = escontext; + } + /* Directly store the values/nulls array in the slot */ if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull)) + { + if (cstate->opts.ignore_datatype_errors && cstate->ignored_errors > 0) + ereport(WARNING, + errmsg("%zd rows were skipped due to data type incompatibility", + cstate->ignored_errors)); break; + } + + /* Soft error occured, skip this tuple */ + if (cstate->escontext.error_occurred) + { + ExecClearTuple(myslot); + continue; + } ExecStoreVirtualTuple(myslot); diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 3853902a16..b06c44e298 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -70,6 +70,7 @@ #include "libpq/pqformat.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/miscnodes.h" #include "pgstat.h" #include "port/pg_bswap.h" #include "utils/builtins.h" @@ -956,10 +957,20 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext, values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]); } else - values[m] = InputFunctionCall(&in_functions[m], - string, - typioparams[m], - att->atttypmod); + /* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype errors */ + if (!InputFunctionCallSafe(&in_functions[m], + string, + typioparams[m], + att->atttypmod, + (Node *) &cstate->escontext, + &values[m])) + { + cstate->ignored_errors++; + + ereport(WARNING, + errmsg("%s", cstate->escontext.error_data->message)); + return true; + } cstate->cur_attname = NULL; cstate->cur_attval = NULL; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index efe88ccf9d..22bf63b42b 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -701,7 +701,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); HANDLER HAVING HEADER_P HOLD HOUR_P - IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE + IDENTITY_P IF_P IGNORE_DATATYPE_ERRORS ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION @@ -3378,6 +3378,10 @@ copy_opt_item: { $$ = makeDefElem("freeze", (Node *) makeBoolean(true), @1); } + | IGNORE_DATATYPE_ERRORS + { + $$ = makeDefElem("ignore_datatype_errors", (Node *)makeBoolean(true), @1); + } | DELIMITER opt_as Sconst { $$ = makeDefElem("delimiter", (Node *) makeString($3), @1); @@ -16827,6 +16831,7 @@ unreserved_keyword: | HOUR_P | IDENTITY_P | IF_P + | IGNORE_DATATYPE_ERRORS | IMMEDIATE | IMMUTABLE | IMPLICIT_P @@ -17382,6 +17387,7 @@ bare_label_keyword: | HOLD | IDENTITY_P | IF_P + | IGNORE_DATATYPE_ERRORS | ILIKE | IMMEDIATE | IMMUTABLE diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index 33175868f6..c2e55ac21f 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -42,6 +42,7 @@ typedef struct CopyFormatOptions * -1 if not specified */ bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ + bool ignore_datatype_errors; /* ignore rows with datatype errors */ bool csv_mode; /* Comma Separated Value format? */ CopyHeaderChoice header_line; /* header line? */ char *null_print; /* NULL marker string (server encoding!) */ diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h index ac2c16f8b8..1164c71631 100644 --- a/src/include/commands/copyfrom_internal.h +++ b/src/include/commands/copyfrom_internal.h @@ -16,6 +16,7 @@ #include "commands/copy.h" #include "commands/trigger.h" +#include "nodes/miscnodes.h" /* * Represents the different source cases we need to worry about at @@ -94,6 +95,8 @@ typedef struct CopyFromStateData * default value */ FmgrInfo *in_functions; /* array of input functions for each attrs */ Oid *typioparams; /* array of element types for in_functions */ + ErrorSaveContext escontext; /* soft error trapper during in_functions execution */ + int64 ignored_errors; /* total number of ignored errors */ int *defmap; /* array of default att numbers related to * missing att */ ExprState **defexprs; /* array of default att expressions for all diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h index 753e9ee174..5ea159e879 100644 --- a/src/include/parser/kwlist.h +++ b/src/include/parser/kwlist.h @@ -196,6 +196,7 @@ PG_KEYWORD("hold", HOLD, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("hour", HOUR_P, UNRESERVED_KEYWORD, AS_LABEL) PG_KEYWORD("identity", IDENTITY_P, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("if", IF_P, UNRESERVED_KEYWORD, BARE_LABEL) +PG_KEYWORD("ignore_datatype_errors", IGNORE_DATATYPE_ERRORS, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("ilike", ILIKE, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL) PG_KEYWORD("immediate", IMMEDIATE, UNRESERVED_KEYWORD, BARE_LABEL) PG_KEYWORD("immutable", IMMUTABLE, UNRESERVED_KEYWORD, BARE_LABEL) diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index 8e33eee719..a6bf3d66a4 100644 --- a/src/test/regress/expected/copy2.out +++ b/src/test/regress/expected/copy2.out @@ -666,6 +666,21 @@ SELECT * FROM instead_of_insert_tbl; (2 rows) COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +WARNING: invalid input syntax for type integer: "a" +WARNING: value "3333333333" is out of range for type integer +WARNING: invalid input syntax for type integer: "a" +WARNING: invalid input syntax for type integer: "" +WARNING: 4 rows were skipped due to data type incompatibility +SELECT * FROM check_ign_err; + n | m | k +---+-----+--- + 1 | {1} | 1 + 5 | {5} | 5 +(2 rows) + -- clean up DROP TABLE forcetest; DROP TABLE vistest; diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql index d759635068..c934029314 100644 --- a/src/test/regress/sql/copy2.sql +++ b/src/test/regress/sql/copy2.sql @@ -464,6 +464,18 @@ test1 SELECT * FROM instead_of_insert_tbl; COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; +1 {1} 1 +a {2} 2 +3 {3} 3333333333 +4 {a, 4} 4 + +5 {5} 5 +\. +SELECT * FROM check_ign_err; + -- clean up DROP TABLE forcetest; DROP TABLE vistest; base-commit: d69c404c4cc5985d8ae5b5ed38bed3400b317f82 -- 2.25.1 ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-07 08:35 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-07 09:09 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-17 12:23 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-22 13:34 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> @ 2023-03-22 17:50 ` Andres Freund <[email protected]> 2023-03-24 09:27 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Andres Freund @ 2023-03-22 17:50 UTC (permalink / raw) To: torikoshia <[email protected]>; [email protected]; +Cc: Daniel Gustafsson <[email protected]>; Damir Belyalov <[email protected]>; pgsql-hackers; Danil Anisimow <[email protected]>; Nikita Malakhov <[email protected]>; [email protected] Hi, Tom, see below - I wonder if should provide one more piece of infrastructure around the saved error stuff... Have you measured whether this has negative performance effects when *NOT* using the new option? As-is this does not work with FORMAT BINARY - and converting the binary input functions to support soft errors won't happen for 16. So I think you need to raise an error if BINARY and IGNORE_DATATYPE_ERRORS are specified. On 2023-03-22 22:34:20 +0900, torikoshia wrote: > @@ -985,9 +986,28 @@ CopyFrom(CopyFromState cstate) > > ExecClearTuple(myslot); > > + if (cstate->opts.ignore_datatype_errors) > + { > + escontext.details_wanted = true; > + cstate->escontext = escontext; > + } I think it might be worth pulling this out of the loop. That does mean you'd have to reset escontext.error_occurred after an error, but that doesn't seem too bad, you need to do other cleanup anyway. > @@ -956,10 +957,20 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext, > values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]); > } > else > - values[m] = InputFunctionCall(&in_functions[m], > - string, > - typioparams[m], > - att->atttypmod); > + /* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype errors */ > + if (!InputFunctionCallSafe(&in_functions[m], > + string, > + typioparams[m], > + att->atttypmod, > + (Node *) &cstate->escontext, > + &values[m])) > + { > + cstate->ignored_errors++; > + > + ereport(WARNING, > + errmsg("%s", cstate->escontext.error_data->message)); That isn't right - you loose all the details of the message. As is you'd also leak the error context. I think the best bet for now is to do something like /* adjust elevel so we don't jump out */ cstate->escontext.error_data->elevel = WARNING; /* despite the name, this won't raise an error if elevel < ERROR */ ThrowErrorData(cstate->escontext.error_data); I wonder if we ought to provide a wrapper for this? It could e.g. know to mention the original elevel and such? I don't think NextCopyFrom() is the right place to emit this warning - it e.g. is also called from file_fdw.c, which might want to do something else with the error. From a layering POV it seems cleaner to do this in CopyFrom(). You already have a check for escontext.error_occurred there anyway. > @@ -3378,6 +3378,10 @@ copy_opt_item: > { > $$ = makeDefElem("freeze", (Node *) makeBoolean(true), @1); > } > + | IGNORE_DATATYPE_ERRORS > + { > + $$ = makeDefElem("ignore_datatype_errors", (Node *)makeBoolean(true), @1); > + } > | DELIMITER opt_as Sconst > { > $$ = makeDefElem("delimiter", (Node *) makeString($3), @1); I think we shouldn't add a new keyword for this, but only support this via /* new COPY option syntax */ copy_generic_opt_list: copy_generic_opt_elem Further increasing the size of the grammar with random keywords when we have more generic ways to represent them seems unnecessary. > +-- tests for IGNORE_DATATYPE_ERRORS option > +CREATE TABLE check_ign_err (n int, m int[], k int); > +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; > +1 {1} 1 > +a {2} 2 > +3 {3} 3333333333 > +4 {a, 4} 4 > + > +5 {5} 5 > +\. > +SELECT * FROM check_ign_err; > + I suggest adding a few more tests: - COPY with a datatype error that can't be handled as a soft error - test documenting that COPY FORMAT BINARY is incompatible with IGNORE_DATATYPE_ERRORS - a soft error showing the error context - although that will require some care to avoid the function name + line in the output Greetings, Andres Freund ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-07 08:35 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-07 09:09 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-17 12:23 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-22 13:34 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-22 17:50 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> @ 2023-03-24 09:27 ` torikoshia <[email protected]> 2023-03-27 13:51 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: torikoshia @ 2023-03-24 09:27 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: [email protected]; Daniel Gustafsson <[email protected]>; Damir Belyalov <[email protected]>; pgsql-hackers; Danil Anisimow <[email protected]>; Nikita Malakhov <[email protected]>; [email protected] On 2023-03-23 02:50, Andres Freund wrote: > Hi, > > Tom, see below - I wonder if should provide one more piece of > infrastructure > around the saved error stuff... > > > Have you measured whether this has negative performance effects when > *NOT* > using the new option? > > > As-is this does not work with FORMAT BINARY - and converting the binary > input > functions to support soft errors won't happen for 16. So I think you > need to > raise an error if BINARY and IGNORE_DATATYPE_ERRORS are specified. > > > On 2023-03-22 22:34:20 +0900, torikoshia wrote: >> @@ -985,9 +986,28 @@ CopyFrom(CopyFromState cstate) >> >> ExecClearTuple(myslot); >> >> + if (cstate->opts.ignore_datatype_errors) >> + { >> + escontext.details_wanted = true; >> + cstate->escontext = escontext; >> + } > > I think it might be worth pulling this out of the loop. That does mean > you'd > have to reset escontext.error_occurred after an error, but that doesn't > seem > too bad, you need to do other cleanup anyway. > > >> @@ -956,10 +957,20 @@ NextCopyFrom(CopyFromState cstate, ExprContext >> *econtext, >> values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]); >> } >> else >> - values[m] = InputFunctionCall(&in_functions[m], >> - string, >> - typioparams[m], >> - att->atttypmod); >> + /* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype >> errors */ >> + if (!InputFunctionCallSafe(&in_functions[m], >> + string, >> + typioparams[m], >> + att->atttypmod, >> + (Node *) &cstate->escontext, >> + &values[m])) >> + { >> + cstate->ignored_errors++; >> + >> + ereport(WARNING, >> + errmsg("%s", cstate->escontext.error_data->message)); > > That isn't right - you loose all the details of the message. As is > you'd also > leak the error context. > > I think the best bet for now is to do something like > /* adjust elevel so we don't jump out */ > cstate->escontext.error_data->elevel = WARNING; > /* despite the name, this won't raise an error if elevel < ERROR */ > ThrowErrorData(cstate->escontext.error_data); Thanks for your reviewing! I'll try to fix it this way for the time being. > I wonder if we ought to provide a wrapper for this? It could e.g. know > to > mention the original elevel and such? -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-07 08:35 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-07 09:09 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-17 12:23 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-22 13:34 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-22 17:50 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-03-24 09:27 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> @ 2023-03-27 13:51 ` torikoshia <[email protected]> 2023-03-27 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: torikoshia @ 2023-03-27 13:51 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: [email protected]; Daniel Gustafsson <[email protected]>; Damir Belyalov <[email protected]>; pgsql-hackers; Danil Anisimow <[email protected]>; Nikita Malakhov <[email protected]>; [email protected] On 2023-03-23 02:50, Andres Freund wrote: Thanks again for your review. Attached v5 patch. > Have you measured whether this has negative performance effects when > *NOT* > using the new option? I loaded 10000000 rows of pgbench_accounts on my laptop and compared the elapsed time. GUCs changed from the default are logging_collector = on, log_error_verbosity = verbose. Three tests were run under each condition and the middle of them is listed below: - patch NOT applied(36f40ce2dc66): 35299ms - patch applied, without IGNORE_DATATYPE_ERRORS: 34409ms - patch applied, with IGNORE_DATATYPE_ERRORS: 35510ms It seems there are no significant degradation. Also tested the elapsed time when loading data which has some datatype error with IGNORE_DATATYPE_ERRORS: - data has 100 rows of error: 35269ms - data has 1000 rows of error: 34577ms - data has 5000000 rows of error: 48925ms 5000000 rows of error consumes much time, but it seems to be influenced by logging time. Here are test results under log_min_messages and client_min_messages are 'error': - data has 5000000 data type error: 23972ms - data has 0 data type error: 34320ms Now conversely, when there were many data type errors, it consumes less time. This seems like a reasonable result since the amount of skipped data is increasing. > As-is this does not work with FORMAT BINARY - and converting the binary > input > functions to support soft errors won't happen for 16. So I think you > need to > raise an error if BINARY and IGNORE_DATATYPE_ERRORS are specified. Added the option check. > On 2023-03-22 22:34:20 +0900, torikoshia wrote: > > @@ -985,9 +986,28 @@ CopyFrom(CopyFromState cstate) > > > > ExecClearTuple(myslot); > > > > + if (cstate->opts.ignore_datatype_errors) > > + { > > + escontext.details_wanted = true; > > + cstate->escontext = escontext; > > + } > > I think it might be worth pulling this out of the loop. That does mean > you'd > have to reset escontext.error_occurred after an error, but that doesn't > seem > too bad, you need to do other cleanup anyway. Pull this out of the loop and added process for resetting escontext. > > @@ -956,10 +957,20 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext, > > values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]); > > } > > else > > - values[m] = InputFunctionCall(&in_functions[m], > > - string, > > - typioparams[m], > > - att->atttypmod); > > + /* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype errors */ > > + if (!InputFunctionCallSafe(&in_functions[m], > > + string, > > + typioparams[m], > > + att->atttypmod, > > + (Node *) &cstate->escontext, > > + &values[m])) > > + { > > + cstate->ignored_errors++; > > + > > + ereport(WARNING, > > + errmsg("%s", cstate->escontext.error_data->message)); > > That isn't right - you loose all the details of the message. As is > you'd also > leak the error context. > > I think the best bet for now is to do something like > /* adjust elevel so we don't jump out */ > cstate->escontext.error_data->elevel = WARNING; > /* despite the name, this won't raise an error if elevel < ERROR */ > ThrowErrorData(cstate->escontext.error_data); As I mentioned in one previous email, added above codes for now. > I wonder if we ought to provide a wrapper for this? It could e.g. know > to > mention the original elevel and such? > > > I don't think NextCopyFrom() is the right place to emit this warning - > it > e.g. is also called from file_fdw.c, which might want to do something > else > with the error. From a layering POV it seems cleaner to do this in > CopyFrom(). You already have a check for escontext.error_occurred there > anyway. Agreed. > > @@ -3378,6 +3378,10 @@ copy_opt_item: > > { > > $$ = makeDefElem("freeze", (Node *) makeBoolean(true), @1); > > } > > + | IGNORE_DATATYPE_ERRORS > > + { > > + $$ = makeDefElem("ignore_datatype_errors", (Node *)makeBoolean(true), @1); > > + } > > | DELIMITER opt_as Sconst > > { > > $$ = makeDefElem("delimiter", (Node *) makeString($3), @1); > > > I think we shouldn't add a new keyword for this, but only support this > via > /* new COPY option syntax */ > copy_generic_opt_list: > copy_generic_opt_elem > > Further increasing the size of the grammar with random keywords when we > have > more generic ways to represent them seems unnecessary. Agreed. > > +-- tests for IGNORE_DATATYPE_ERRORS option > > +CREATE TABLE check_ign_err (n int, m int[], k int); > > +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS; > > +1 {1} 1 > > +a {2} 2 > > +3 {3} 3333333333 > > +4 {a, 4} 4 > > + > > +5 {5} 5 > > +\. > > +SELECT * FROM check_ign_err; > > + > > I suggest adding a few more tests: > > - COPY with a datatype error that can't be handled as a soft error Added a test for cases with missing columns. However it's not datatype error and not what you expected, is it? > - test documenting that COPY FORMAT BINARY is incompatible with > IGNORE_DATATYPE_ERRORS Added it. > - a soft error showing the error context - although that will require > some > care to avoid the function name + line in the output I assume you mean a test to check the server log, but I haven't come up with a way to do it. Adding a TAP test might do it, but I think it would be overkill to add one just for this. -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION Attachments: [text/x-diff] v5-0001-Add-new-COPY-option-IGNORE_DATATYPE_ERRORS.patch (12.5K, ../../[email protected]/2-v5-0001-Add-new-COPY-option-IGNORE_DATATYPE_ERRORS.patch) download | inline diff: From 6b646d96a9c2a310836693452deb2128636d1beb Mon Sep 17 00:00:00 2001 From: Atsushi Torikoshi <[email protected]> Date: Mon, 27 Mar 2023 22:15:49 +0900 Subject: [PATCH v5] Add new COPY option IGNORE_DATATYPE_ERRORS Currently entire COPY fails when there exists unexpected data regarding data type or range. In some cases, it would be useful to skip copying such data and continue copying and IGNORE_DATATYPE_ERRORS does this. This patch uses the soft error handling infrastructure, which is introduced by d9f7f5d32f20. Author: Damir Belyalov, Atsushi Torikoshi --- doc/src/sgml/ref/copy.sgml | 13 ++++++++++ src/backend/commands/copy.c | 15 ++++++++++- src/backend/commands/copyfrom.c | 32 ++++++++++++++++++++++++ src/backend/commands/copyfromparse.c | 17 ++++++++++--- src/bin/psql/tab-complete.c | 3 ++- src/include/commands/copy.h | 1 + src/include/commands/copyfrom_internal.h | 3 +++ src/test/regress/expected/copy2.out | 22 ++++++++++++++++ src/test/regress/sql/copy2.sql | 19 ++++++++++++++ 9 files changed, 119 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 5e591ed2e6..cea56d65eb 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -34,6 +34,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable FORMAT <replaceable class="parameter">format_name</replaceable> FREEZE [ <replaceable class="parameter">boolean</replaceable> ] + IGNORE_DATATYPE_ERRORS [ <replaceable class="parameter">boolean</replaceable> ] DELIMITER '<replaceable class="parameter">delimiter_character</replaceable>' NULL '<replaceable class="parameter">null_string</replaceable>' HEADER [ <replaceable class="parameter">boolean</replaceable> | MATCH ] @@ -234,6 +235,18 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable </listitem> </varlistentry> + <varlistentry> + <term><literal>IGNORE_DATATYPE_ERRORS</literal></term> + <listitem> + <para> + Drops rows that contain malformed data while copying. These are rows + with columns where the data type's input-function raises an error. + This option is not allowed when using binary format. Note that this + is only supported in current <command>COPY</command> syntax. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>DELIMITER</literal></term> <listitem> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index f14fae3308..aa50cc1ee1 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -419,6 +419,7 @@ ProcessCopyOptions(ParseState *pstate, bool format_specified = false; bool freeze_specified = false; bool header_specified = false; + bool ignore_datatype_errors_specified = false; ListCell *option; /* Support external use for option sanity checking */ @@ -458,6 +459,13 @@ ProcessCopyOptions(ParseState *pstate, freeze_specified = true; opts_out->freeze = defGetBoolean(defel); } + else if (strcmp(defel->defname, "ignore_datatype_errors") == 0) + { + if (ignore_datatype_errors_specified) + errorConflictingDefElem(defel, pstate); + ignore_datatype_errors_specified = true; + opts_out->ignore_datatype_errors = defGetBoolean(defel); + } else if (strcmp(defel->defname, "delimiter") == 0) { if (opts_out->delim) @@ -576,7 +584,7 @@ ProcessCopyOptions(ParseState *pstate, } /* - * Check for incompatible options (must do these two before inserting + * Check for incompatible options (must do these before inserting * defaults) */ if (opts_out->binary && opts_out->delim) @@ -594,6 +602,11 @@ ProcessCopyOptions(ParseState *pstate, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("cannot specify DEFAULT in BINARY mode"))); + if (opts_out->binary && opts_out->ignore_datatype_errors) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("cannot specify IGNORE_DATATYPE_ERRORS in BINARY mode"))); + /* Set defaults for omitted options */ if (!opts_out->delim) opts_out->delim = opts_out->csv_mode ? "," : "\t"; diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 80bca79cd0..9e23cd45d5 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -652,6 +652,7 @@ CopyFrom(CopyFromState cstate) bool has_before_insert_row_trig; bool has_instead_insert_row_trig; bool leafpart_use_multi_insert = false; + ErrorSaveContext escontext = {T_ErrorSaveContext}; Assert(cstate->rel); Assert(list_length(cstate->range_table) == 1); @@ -752,6 +753,13 @@ CopyFrom(CopyFromState cstate) ti_options |= TABLE_INSERT_FROZEN; } + /* Set up soft error handler for IGNORE_DATATYPE_ERRORS */ + if (cstate->opts.ignore_datatype_errors) + { + escontext.details_wanted = true; + cstate->escontext = escontext; + } + /* * We need a ResultRelInfo so we can use the regular executor's * index-entry-making machinery. (There used to be a huge amount of code @@ -987,7 +995,31 @@ CopyFrom(CopyFromState cstate) /* Directly store the values/nulls array in the slot */ if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull)) + { + if (cstate->opts.ignore_datatype_errors && cstate->ignored_errors > 0) + ereport(WARNING, + errmsg("%zd rows were skipped due to data type incompatibility", + cstate->ignored_errors)); break; + } + + /* Soft error occured, skip this tuple and cleanup the escontext */ + if (cstate->escontext.error_occurred) + { + ErrorSaveContext new_escontext = {T_ErrorSaveContext}; + + /* adjust elevel so we don't jump out */ + cstate->escontext.error_data->elevel = WARNING; + /* despite the name, this won't raise an error since elevel is WARNING now */ + ThrowErrorData(cstate->escontext.error_data); + + ExecClearTuple(myslot); + + new_escontext.details_wanted = true; + cstate->escontext = new_escontext; + + continue; + } ExecStoreVirtualTuple(myslot); diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 3853902a16..6b0447782d 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -70,6 +70,7 @@ #include "libpq/pqformat.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/miscnodes.h" #include "pgstat.h" #include "port/pg_bswap.h" #include "utils/builtins.h" @@ -956,10 +957,18 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext, values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]); } else - values[m] = InputFunctionCall(&in_functions[m], - string, - typioparams[m], - att->atttypmod); + /* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype errors */ + if (!InputFunctionCallSafe(&in_functions[m], + string, + typioparams[m], + att->atttypmod, + (Node *) &cstate->escontext, + &values[m])) + { + cstate->ignored_errors++; + + return true; + } cstate->cur_attname = NULL; cstate->cur_attval = NULL; diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 42e87b9e49..4b443d4ea7 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2857,7 +2857,8 @@ psql_completion(const char *text, int start, int end) else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(")) COMPLETE_WITH("FORMAT", "FREEZE", "DELIMITER", "NULL", "HEADER", "QUOTE", "ESCAPE", "FORCE_QUOTE", - "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING", "DEFAULT"); + "FORCE_NOT_NULL", "FORCE_NULL", "ENCODING", "DEFAULT", + "IGNORE_DATATYPE_ERRORS"); /* Complete COPY <sth> FROM|TO filename WITH (FORMAT */ else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAny, "WITH", "(", "FORMAT")) diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index 33175868f6..c2e55ac21f 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -42,6 +42,7 @@ typedef struct CopyFormatOptions * -1 if not specified */ bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ + bool ignore_datatype_errors; /* ignore rows with datatype errors */ bool csv_mode; /* Comma Separated Value format? */ CopyHeaderChoice header_line; /* header line? */ char *null_print; /* NULL marker string (server encoding!) */ diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h index ac2c16f8b8..1164c71631 100644 --- a/src/include/commands/copyfrom_internal.h +++ b/src/include/commands/copyfrom_internal.h @@ -16,6 +16,7 @@ #include "commands/copy.h" #include "commands/trigger.h" +#include "nodes/miscnodes.h" /* * Represents the different source cases we need to worry about at @@ -94,6 +95,8 @@ typedef struct CopyFromStateData * default value */ FmgrInfo *in_functions; /* array of input functions for each attrs */ Oid *typioparams; /* array of element types for in_functions */ + ErrorSaveContext escontext; /* soft error trapper during in_functions execution */ + int64 ignored_errors; /* total number of ignored errors */ int *defmap; /* array of default att numbers related to * missing att */ ExprState **defexprs; /* array of default att expressions for all diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index 8e33eee719..99f70f44ed 100644 --- a/src/test/regress/expected/copy2.out +++ b/src/test/regress/expected/copy2.out @@ -82,6 +82,8 @@ COPY x to stdin (format BINARY, delimiter ','); ERROR: cannot specify DELIMITER in BINARY mode COPY x to stdin (format BINARY, null 'x'); ERROR: cannot specify NULL in BINARY mode +COPY x to stdin (format BINARY, ignore_datatype_errors); +ERROR: cannot specify IGNORE_DATATYPE_ERRORS in BINARY mode COPY x to stdin (format TEXT, force_quote(a)); ERROR: COPY force quote available only in CSV mode COPY x from stdin (format CSV, force_quote(a)); @@ -666,6 +668,25 @@ SELECT * FROM instead_of_insert_tbl; (2 rows) COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH (IGNORE_DATATYPE_ERRORS); +WARNING: invalid input syntax for type integer: "a" +WARNING: value "3333333333" is out of range for type integer +WARNING: invalid input syntax for type integer: "a" +WARNING: invalid input syntax for type integer: "" +WARNING: 4 rows were skipped due to data type incompatibility +SELECT * FROM check_ign_err; + n | m | k +---+-----+--- + 1 | {1} | 1 + 5 | {5} | 5 +(2 rows) + +-- test missing data: should fail +COPY check_ign_err FROM STDIN WITH (IGNORE_DATATYPE_ERRORS); +ERROR: missing data for column "k" +CONTEXT: COPY check_ign_err, line 1: "1 {1}" -- clean up DROP TABLE forcetest; DROP TABLE vistest; @@ -680,6 +701,7 @@ DROP TABLE instead_of_insert_tbl; DROP VIEW instead_of_insert_tbl_view; DROP VIEW instead_of_insert_tbl_view_2; DROP FUNCTION fun_instead_of_insert_tbl(); +DROP TABLE check_ign_err; -- -- COPY FROM ... DEFAULT -- diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql index d759635068..e88f27bbfd 100644 --- a/src/test/regress/sql/copy2.sql +++ b/src/test/regress/sql/copy2.sql @@ -70,6 +70,7 @@ COPY x from stdin (encoding 'sql_ascii', encoding 'sql_ascii'); -- incorrect options COPY x to stdin (format BINARY, delimiter ','); COPY x to stdin (format BINARY, null 'x'); +COPY x to stdin (format BINARY, ignore_datatype_errors); COPY x to stdin (format TEXT, force_quote(a)); COPY x from stdin (format CSV, force_quote(a)); COPY x to stdout (format TEXT, force_not_null(a)); @@ -464,6 +465,23 @@ test1 SELECT * FROM instead_of_insert_tbl; COMMIT; +-- tests for IGNORE_DATATYPE_ERRORS option +CREATE TABLE check_ign_err (n int, m int[], k int); +COPY check_ign_err FROM STDIN WITH (IGNORE_DATATYPE_ERRORS); +1 {1} 1 +a {2} 2 +3 {3} 3333333333 +4 {a, 4} 4 + +5 {5} 5 +\. +SELECT * FROM check_ign_err; + +-- test missing data: should fail +COPY check_ign_err FROM STDIN WITH (IGNORE_DATATYPE_ERRORS); +1 {1} +\. + -- clean up DROP TABLE forcetest; DROP TABLE vistest; @@ -478,6 +496,7 @@ DROP TABLE instead_of_insert_tbl; DROP VIEW instead_of_insert_tbl_view; DROP VIEW instead_of_insert_tbl_view_2; DROP FUNCTION fun_instead_of_insert_tbl(); +DROP TABLE check_ign_err; -- -- COPY FROM ... DEFAULT base-commit: 36f40ce2dc66f1a36d6a12f7a0352e1c5bf1063e -- 2.25.1 ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-07 08:35 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-07 09:09 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-17 12:23 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-22 13:34 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-22 17:50 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-03-24 09:27 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-27 13:51 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> @ 2023-03-27 14:28 ` Damir Belyalov <[email protected]> 2023-03-28 12:16 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Damir Belyalov @ 2023-03-27 14:28 UTC (permalink / raw) To: torikoshia <[email protected]>; +Cc: Andres Freund <[email protected]>; [email protected]; Daniel Gustafsson <[email protected]>; pgsql-hackers; Danil Anisimow <[email protected]>; Nikita Malakhov <[email protected]>; [email protected] Hi! I made the specified changes and my patch turned out the same as yours. The performance measurements were the same too. The only thing left to do is how not to add IGNORE_DATATYPE_ERRORS as a keyword. See how this is done for parameters such as FORCE_NOT_NULL, FORCE_NULL, FORCE_QUOTE. They are not in kwlist.h and are not as keywords in gram.y. Regards, Damir Belyalov Postgres Professional ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-06 05:52 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 06:00 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Tom Lane <[email protected]> 2023-02-27 05:58 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-02-28 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-07 08:35 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> 2023-03-07 09:09 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Daniel Gustafsson <[email protected]> 2023-03-17 12:23 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-22 13:34 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-22 17:50 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-03-24 09:27 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-27 13:51 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) torikoshia <[email protected]> 2023-03-27 14:28 ` Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Damir Belyalov <[email protected]> @ 2023-03-28 12:16 ` torikoshia <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: torikoshia @ 2023-03-28 12:16 UTC (permalink / raw) To: Damir Belyalov <[email protected]>; +Cc: Andres Freund <[email protected]>; [email protected]; Daniel Gustafsson <[email protected]>; pgsql-hackers; Danil Anisimow <[email protected]>; Nikita Malakhov <[email protected]>; [email protected] On 2023-03-27 23:28, Damir Belyalov wrote: > Hi! > > I made the specified changes and my patch turned out the same as > yours. The performance measurements were the same too. Thanks for your review and measurements. > The only thing left to do is how not to add IGNORE_DATATYPE_ERRORS as > a keyword. See how this is done for parameters such as FORCE_NOT_NULL, > FORCE_NULL, FORCE_QUOTE. They are not in kwlist.h and are not as > keywords in gram.y. I might misunderstand something, but I believe the v5 patch uses copy_generic_opt_list and it does not add IGNORE_DATATYPE_ERRORS as a keyword. It modifies neither kwlist.h nor gram.y. -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v10 5/7] Row pattern recognition patch (docs). @ 2023-10-22 02:22 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: Tatsuo Ishii @ 2023-10-22 02:22 UTC (permalink / raw) --- doc/src/sgml/advanced.sgml | 80 ++++++++++++++++++++++++++++++++++++ doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++ doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++- 3 files changed, 170 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml index 755c9f1485..cf18dd887e 100644 --- a/doc/src/sgml/advanced.sgml +++ b/doc/src/sgml/advanced.sgml @@ -537,6 +537,86 @@ 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. 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. So 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. In the second or + subsequent rows all window functions and aggregates are NULL. For rows + that do not match the PATTERN, all window functions and aggregates are + shown AS NULL too, except count which shows 0. This is because the + unmatched rows 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 | | | + company1 | 2023-07-03 | 150 | | | + company1 | 2023-07-04 | 140 | | | + company1 | 2023-07-05 | 150 | | | 0 + company1 | 2023-07-06 | 90 | 90 | 130 | 4 + company1 | 2023-07-07 | 110 | | | + company1 | 2023-07-08 | 130 | | | + company1 | 2023-07-09 | 120 | | | + company1 | 2023-07-10 | 130 | | | 0 +</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 7c3e940afe..1d835af15a 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -21878,6 +21878,7 @@ SELECT count(*) FROM sometable; returns <literal>NULL</literal> if there is no such row. </para></entry> </row> + </tbody> </tgroup> </table> @@ -21917,6 +21918,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 42d78913cf..522ad9dd70 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(Sun_Oct_22_11_39_20_2023_140)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v10-0006-Row-pattern-recognition-patch-tests.patch" ^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2023-10-22 02:22 UTC | newest] Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-02-05 23:37 Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) Andres Freund <[email protected]> 2023-02-06 05:00 ` Damir Belyalov <[email protected]> 2023-02-06 05:12 ` Tom Lane <[email protected]> 2023-02-06 05:52 ` Andres Freund <[email protected]> 2023-02-06 06:00 ` Tom Lane <[email protected]> 2023-02-27 05:58 ` torikoshia <[email protected]> 2023-02-28 14:28 ` Damir Belyalov <[email protected]> 2023-03-06 14:03 ` Daniel Gustafsson <[email protected]> 2023-03-07 02:07 ` torikoshia <[email protected]> 2023-03-07 08:35 ` Damir Belyalov <[email protected]> 2023-03-07 09:09 ` Daniel Gustafsson <[email protected]> 2023-03-17 12:23 ` torikoshia <[email protected]> 2023-03-22 13:34 ` torikoshia <[email protected]> 2023-03-22 17:50 ` Andres Freund <[email protected]> 2023-03-24 09:27 ` torikoshia <[email protected]> 2023-03-27 13:51 ` torikoshia <[email protected]> 2023-03-27 14:28 ` Damir Belyalov <[email protected]> 2023-03-28 12:16 ` torikoshia <[email protected]> 2023-10-22 02:22 [PATCH v10 5/7] 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