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

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