Re: Add new error_action COPY ON_ERROR "log"

On 2024-02-20 17:22, torikoshia wrote:
> On 2024-02-17 15:00, Bharath Rupireddy wrote:
>> On Fri, Feb 16, 2024 at 8:17 PM torikoshia 
>> <torikoshia@oss.nttdata.com> wrote:
>>> 
>>> I may be wrong since I seldom do data loading tasks, but I greed with
>>> you.
>>> 
>>> I also a little concerned about the case where there are many 
>>> malformed
>>> data and it causes lots of messages, but the information is usually
>>> valuable and if users don't need it, they can suppress it by changing
>>> client_min_messages.
>>> 
>>> Currently both summary of failures and individual information is 
>>> logged
>>> in NOTICE level.
>>> If we should assume that there are cases where only summary 
>>> information
>>> is required, it'd be useful to set lower log level, i.e. LOG to the
>>> individual information.
>> 
>> How about we emit the summary at INFO level and individual information
>> at NOTICE level? With this, the summary is given a different priority
>> than the individual info. With SET client_min_messages = WARNING; one
>> can still get the summary but not the individual info. Also, to get
>> all of these into server log, one can SET log_min_messages = INFO; or
>> SET log_min_messages = NOTICE;.
>> 
>> Thoughts?
> 
> It looks good to me.

Here are comments on the v2 patch.

+               if (cstate->opts.on_error != COPY_ON_ERROR_STOP)
+               {
+                   ereport(NOTICE,

I think cstate->opts.on_error is not COPY_ON_ERROR_STOP here, since if 
it is COPY_ON_ERROR_STOP, InputFunctionCallSafe() should already have 
errored out.

Should it be something like "Assert(cstate->opts.on_error != 
COPY_ON_ERROR_STOP)"?


Should below manual also be updated?

> A NOTICE message containing the ignored row count is emitted at the end 
> of the COPY FROM if at least one row was discarded.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation