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

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