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

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 <dam.bel07@gmail.com> 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