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

[torikoshia]

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