Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

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